1206341 - [RTL] Ruleview toolbar elements should be reversed direction in RTL locales

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P3)

Description

[magicp]

Attachment: rtl-sidebar-tab-ruleview-toolbar.png

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:43.0) Gecko/20100101 Firefox/43.0
Build ID: 20150918030202

Steps to reproduce:

1. Run any version Firefox in Arabic
2. Open Ruleview in Developer Tools


Actual results:

Ruleview toolbar elements have wrong direction in RTL locales.


Expected results:

Ruleview toolbar elements should be reversed direction in RTL locales.

Comment 1

[Tim Nguyen :ntim]

To fix this bug, you need to add this rule:
.devtools-toolbar:-moz-dir(rtl) {
  flex-direction: row-reverse;
}
after https://dxr.mozilla.org/mozilla-central/source/devtools/client/styleinspector/ruleview.css?offset=200#27

Comment 2

[Sanchit Nevgi]

Hi Tim! Can I try fixing this bug? If so, how do I run a version of Firefox in Arabic?
Thanks :)

Comment 3

[Tim Nguyen :ntim]

(In reply to Sanchit Nevgi from comment #2)
> Hi Tim! Can I try fixing this bug? If so, how do I run a version of Firefox
> in Arabic?
> Thanks :)

Sure ! Thanks for your interest in this bug !
I think you can install an Arabic language pack [0] on top of your Firefox build, although I would recommend you to use this extension [1] rather than a language pack. With that extension, you can force RTL text by click on the Tools menu, then "Force RTL Direction".

[0]: https://addons.mozilla.org/en-US/firefox/language-tools/
[1]: https://addons.mozilla.org/en-US/firefox/addon/force-rtl/

Comment 4

[Sanchit Nevgi]

(In reply to Tim Nguyen [:ntim] from comment #3)

Hey Tim! I tried setting the flex-direction to row-reverse, but it doesn't seem to work on my build. The pseudo-selector -moz-dir(rtl) isn't triggered(The layout changes without using the selector). Is it because of the Force RTL extention? I've also tried using -moz-locale-dir(rtl) to no effect. What am I doing wrong? Thanks for your help :)

Comment 5

[Tim Nguyen :ntim]

(In reply to Sanchit Nevgi from comment #4)
> (In reply to Tim Nguyen [:ntim] from comment #3)
> 
> Hey Tim! I tried setting the flex-direction to row-reverse, but it doesn't
> seem to work on my build. The pseudo-selector -moz-dir(rtl) isn't
> triggered(The layout changes without using the selector). Is it because of
> the Force RTL extention? I've also tried using -moz-locale-dir(rtl) to no
> effect. What am I doing wrong? Thanks for your help :)
Sorry for the late reply, missed the comment. Please use the needinfo flag next time so I don't miss your comment :)

Just asked :ehsan on IRC, and looks like you also need to add `dir="&locale.dir;"` to http://mxr.mozilla.org/mozilla-central/source/devtools/client/styleinspector/cssruleview.xhtml#34
and the locale.dir definition that comes with it after line 8:   
  <!ENTITY % globalDTD
    SYSTEM "chrome://global/locale/global.dtd">
  %globalDTD;

Comment 6

[Sanchit Nevgi]

Hey Tim! Sorry for the late reply, my exams were going on. I made the required changes and built it, but it has no effect. After inspecting the code using browser toolbox I've noticed that the dir attribute of body is still ltr(not rtl). Changing it to rtl has the desired effect. I'm confused as to what to do next, any suggestions? I've added a patch of the changes made. Thanks :)

Comment 7

[Sanchit Nevgi]

Attachment: patch_v1

Comment 8

[Brian Grinstead [:bgrins]]

I believe this will be (at least partially) fixed by Bug 1220839, where dir="&locale.dir;" is being set to the rule view.  I'm not sure if the additional styles will be needed.

Comment 9

[Tim Nguyen :ntim]

(In reply to Brian Grinstead [:bgrins] from comment #8)
> I believe this will be (at least partially) fixed by Bug 1220839, where
> dir="&locale.dir;" is being set to the rule view.  I'm not sure if the
> additional styles will be needed.

Alright, let's wait for that to land then. I doubt that bug will fix this one, but it will surely make that easier.

Comment 10

[Tim Nguyen :ntim]

After Bug 1220839, only the CSS changes are needed. Sanchit, do you have enough information to pursue this bug ?

Comment 11

[Sanchit Nevgi]

Hey Tim, Even after making CSS changes there's no effect. The body dir attribute remains ltr. Without making any CSS changes and changing dir to rtl reverses direction(I changed locale.dir in global.dtd to rtl). I can't seem to figure out why the dir attribute isn't changing in rtl locales.

Comment 12

[Tim Nguyen :ntim]

(In reply to Sanchit Nevgi from comment #11)
> Hey Tim, Even after making CSS changes there's no effect. The body dir
> attribute remains ltr. Without making any CSS changes and changing dir to
> rtl reverses direction(I changed locale.dir in global.dtd to rtl). I can't
> seem to figure out why the dir attribute isn't changing in rtl locales.

Can you post your patch so I can investigate ?

Comment 13

[Sanchit Nevgi]

Attachment: ruleview.patch

This patch has the CSS changes to ruleview.css

Comment 14

[Tim Nguyen :ntim]

Brian, any chance you could take a look at this ? I'm lacking time to look at this.

Comment 15

[Brian Grinstead [:bgrins]]

Comment on attachment 8689004 [details] [diff] [review]
ruleview.patch

Review of attachment 8689004 [details] [diff] [review]:
-----------------------------------------------------------------

This seems to work for me locally, did you build the dom/ folder after modifying the dtd?

::: devtools/client/styleinspector/ruleview.css
@@ +30,5 @@
>    transition: visibility 0.25s;
>  }
>  
> +.devtools-toolbar:-moz-dir(rtl),
> +.devtools-toolbar:-moz-locale-dir(rtl) {

You should be able to remove .devtools-toolbar:-moz-locale-dir(rtl) since this isn't a xul doc.  Unless if we want to make this a general rule inside of toolbars.css - although I don't think we are using flex for all toolbars so that'd probably be another bug.

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/d6788a70c97b

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d6788a70c97b

Comment 18

[Tim Nguyen :ntim]

Looks like bug 1224192 landed here accidentally.

Comment 19

[Patrick Brosset <:pbro>]

Inspector bug triage (filter on CLIMBING SHOES).

Comment 20

[[:Towkir] Ahmed]

Hey everyone,
Is this bug still reproducible ? Asked this because I could not reproduce.
Can anyone check this ?

Comment 21

[Patrick Brosset <:pbro>]

[good first bug] whiteboard -> keyword mass change

Comment 22

[Itiel]

Seems to be fixed in recent builds.
Closing as RESOLVED FIXED.
Feel free to reopen if you think otherwise.