1325539 - [computed style][rtl] RTL support for devtools computed style (un-rtl)

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Tomer Cohen :tomer]

A change introduced by bug 1220839 made the computed style pane RTL. While it is okay to have toolbars in RTL, the main area of the screen is actually the CSS rules, which almost always contains code, not RTL text (with the only exception I can think about is the content() attributes). I think it would make sense to reverse the code section back to LTR.

Comment 1

[Tomer Cohen :tomer]

Attachment: Bug 1325539 [computed style][rtl] RTL support for devtools computed style (un-rtl)

Review commit: https://reviewboard.mozilla.org/r/100732/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/100732/

Comment 2

[Tomer Cohen :tomer]

Comment on attachment 8821439 [details]
Bug 1325539 [computed style][rtl] RTL support for devtools computed style (un-rtl)

https://reviewboard.mozilla.org/r/100732/#review101186

::: devtools/client/themes/computed.css:52
(Diff revision 1)
>  #propertyContainer {
>    -moz-user-select: text;
>    overflow-y: auto;
>    overflow-x: hidden;
>    flex: auto;
> +  direction: ltr;

While this fix the issue by aligning the properties to the left (LTR), it doesn't deal with the expander well enough, so now it appears reversed (pointing to the left). 

The code is `.theme-twisty:dir(rtl), .theme-twisty:-moz-locale-dir(rtl) {
    transform: scaleX(-1);
}` on `common.css` line 746. 

:dir is actually not RTL because its parent is LTR, but it seems the the browser ignore this fact.

Comment 3

[Tomer Cohen :tomer]

Comment on attachment 8821439 [details]
Bug 1325539 [computed style][rtl] RTL support for devtools computed style (un-rtl)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100732/diff/1-2/

Comment 4

[Julian Descottes [:jdescottes]]

Comment on attachment 8821439 [details]
Bug 1325539 [computed style][rtl] RTL support for devtools computed style (un-rtl)

https://reviewboard.mozilla.org/r/100732/#review101588

Thanks for the patch and for the input! 
We've been struggling with our RTL styles because we are always unsure about what should be reversed and what should stay, so your feedback is most welcome!

R+ with my comment addressed.

::: devtools/client/themes/common.css
(Diff revision 2)
>    visibility: hidden;
>  }
>  
>  /* Mirror the twisty for rtl direction */
> -.theme-twisty:dir(rtl),
> +.theme-twisty:dir(rtl) {
> -.theme-twisty:-moz-locale-dir(rtl) {

Removing this selector doesn't fix the arrow icon display and will create display issues in XUL panels such as the performance panel.

:dir() ignores the stylistic "direction" and only relies on the semantic direction (here the dir="rtl" set on the document).

In the webconsole, this is manually overridden :http://searchfox.org/mozilla-central/source/devtools/client/themes/webconsole.css#577-581

We could do the same in computed.css

::: devtools/client/themes/computed.css:52
(Diff revision 2)
>  #propertyContainer {
>    -moz-user-select: text;
>    overflow-y: auto;
>    overflow-x: hidden;
>    flex: auto;
> +  direction: ltr;

Totally agree! Also more consistent with the computed view.

Comment 5

[Julian Descottes [:jdescottes]]

Comment on attachment 8821439 [details]
Bug 1325539 [computed style][rtl] RTL support for devtools computed style (un-rtl)

https://reviewboard.mozilla.org/r/100732/#review101588

> Totally agree! Also more consistent with the computed view.

Meant "more consistent with the rule view" :)

Comment 6

[Tomer Cohen :tomer]

Comment on attachment 8821439 [details]
Bug 1325539 [computed style][rtl] RTL support for devtools computed style (un-rtl)

https://reviewboard.mozilla.org/r/100732/#review101588

> Removing this selector doesn't fix the arrow icon display and will create display issues in XUL panels such as the performance panel.
> 
> :dir() ignores the stylistic "direction" and only relies on the semantic direction (here the dir="rtl" set on the document).
> 
> In the webconsole, this is manually overridden :http://searchfox.org/mozilla-central/source/devtools/client/themes/webconsole.css#577-581
> 
> We could do the same in computed.css

I'll do it, but I still doesn't understand why -moz-locale-dir(rtl) is useful in such cases, because when RTL is required we should always set it on parent. In other words, If list items are LTR, the icon should be LTR as well, and not as the locale direction dictate.

Comment 7

[Julian Descottes [:jdescottes]]

(In reply to Tomer Cohen :tomer from comment #6)
> Comment on attachment 8821439 [details]
> Bug 1325539 [computed style][rtl] RTL support for devtools computed style
> (un-rtl)
> 
> https://reviewboard.mozilla.org/r/100732/#review101588
> 
> > Removing this selector doesn't fix the arrow icon display and will create display issues in XUL panels such as the performance panel.
> > 
> > :dir() ignores the stylistic "direction" and only relies on the semantic direction (here the dir="rtl" set on the document).
> > 
> > In the webconsole, this is manually overridden :http://searchfox.org/mozilla-central/source/devtools/client/themes/webconsole.css#577-581
> > 
> > We could do the same in computed.css
> 
> I'll do it, but I still doesn't understand why -moz-locale-dir(rtl) is
> useful in such cases, because when RTL is required we should always set it
> on parent. In other words, If list items are LTR, the icon should be LTR as
> well, and not as the locale direction dictate.

dir(rtl) doesn't care about the parent's direction style value either. -moz-locale-dir(rtl) allows this selector to work in XUL documents which don't support any dir attribute.

If we want to semantically change the direction of an element, we can add a dir="rtl" directly in the markup for this element.
(here that would be in inspector.xhtml: http://searchfox.org/mozilla-central/source/devtools/client/inspector/inspector.xhtml#191)

Feel free to update your patch with this alternative approach if you want.

Comment 8

[Tomer Cohen :tomer]

Comment on attachment 8821439 [details]
Bug 1325539 [computed style][rtl] RTL support for devtools computed style (un-rtl)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100732/diff/2-3/

Comment 9

[Tomer Cohen :tomer]

Comment on attachment 8821439 [details]
Bug 1325539 [computed style][rtl] RTL support for devtools computed style (un-rtl)

https://reviewboard.mozilla.org/r/100732/#review101186

> While this fix the issue by aligning the properties to the left (LTR), it doesn't deal with the expander well enough, so now it appears reversed (pointing to the left). 
> 
> The code is `.theme-twisty:dir(rtl), .theme-twisty:-moz-locale-dir(rtl) {
>     transform: scaleX(-1);
> }` on `common.css` line 746. 
> 
> :dir is actually not RTL because its parent is LTR, but it seems the the browser ignore this fact.

(answering myself) …Because the -moz-locale-dir is hardcoded to RTL and can't be overriden.

https://dxr.mozilla.org/mozilla-central/rev/5ea0c495d3b2318287bffe1121e0e33d74427143/devtools/client/themes/common.css#745-749

I don't understand the logic here. If the element is LTR, why should we care about the locale direction?

Comment 10

[Julian Descottes [:jdescottes]]

(In reply to Tomer Cohen :tomer from comment #9)
> Comment on attachment 8821439 [details]
> Bug 1325539 [computed style][rtl] RTL support for devtools computed style
> (un-rtl)
> 
> https://reviewboard.mozilla.org/r/100732/#review101186
> 
> > While this fix the issue by aligning the properties to the left (LTR), it doesn't deal with the expander well enough, so now it appears reversed (pointing to the left). 
> > 
> > The code is `.theme-twisty:dir(rtl), .theme-twisty:-moz-locale-dir(rtl) {
> >     transform: scaleX(-1);
> > }` on `common.css` line 746. 
> > 
> > :dir is actually not RTL because its parent is LTR, but it seems the the browser ignore this fact.
> 
> (answering myself) …Because the -moz-locale-dir is hardcoded to RTL and
> can't be overriden.
> 
> https://dxr.mozilla.org/mozilla-central/rev/
> 5ea0c495d3b2318287bffe1121e0e33d74427143/devtools/client/themes/common.
> css#745-749
> 
> I don't understand the logic here. If the element is LTR, why should we care
> about the locale direction?

The element does not really become LTR because of the `direction: ltr;` specified in CSS. You can use dir=ltr if you want to actually force the semantic direction on the element (and if you do so, you don't need to change any CSS, everything should be displayed in LTR).

Comment 11

[Tomer Cohen :tomer]

I am updating the commit with <div id="propertyContainer" dir=ltr">

Comment 12

[Tomer Cohen :tomer]

Comment on attachment 8821439 [details]
Bug 1325539 [computed style][rtl] RTL support for devtools computed style (un-rtl)

https://reviewboard.mozilla.org/r/100732/#review101588

We are, as native RTL readers, are more aware of such issues. We don't need to spend much time thinking on what should be reversed and what not, because we 'feel' it right away. Feel free to use our knowledge when you need second opinion. :)

Comment 13

[Tomer Cohen :tomer]

Comment on attachment 8821439 [details]
Bug 1325539 [computed style][rtl] RTL support for devtools computed style (un-rtl)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100732/diff/3-4/

Comment 14

[Julian Descottes [:jdescottes]]

Thanks for the update! Pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cb07db5d4d1cc21de05bcd95ca29b523348a9e75

Will autoland after we get some results there.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dc649ffb0370
[computed style][rtl] RTL support for devtools computed style (un-rtl) r=jdescottes

Comment 16

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

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

Comment 17

[Itiel]

I can confirm this is fixed on latest Nightly build.

Comment 19

[magicp]

How about uplift?

Comment 20

[Sylvestre Ledru [:Sylvestre]]

Too late for 50 & 51

Comment 21

[Sylvestre Ledru [:Sylvestre]]

Tomer, is it something you want to uplift? If yes, you have to go in the attachment page and fill the details