Closed
Bug 1325539
Opened 8 years ago
Closed 8 years ago
[computed style][rtl] RTL support for devtools computed style (un-rtl)
Categories
(DevTools :: Inspector, defect)
DevTools
Inspector
Tracking
(firefox-esr45 affected, firefox50 wontfix, firefox51 wontfix, firefox52 affected, firefox53 fixed)
RESOLVED
FIXED
Firefox 53
People
(Reporter: tomer, Assigned: tomer)
References
(Blocks 1 open bug, )
Details
Attachments
(1 file)
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 hidden (mozreview-request) |
Assignee | ||
Comment 2•8 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Comment 4•8 years ago
|
||
mozreview-review |
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.
Attachment #8821439 -
Flags: review?(jdescottes) → review+
Comment 5•8 years ago
|
||
mozreview-review-reply |
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" :)
Assignee | ||
Comment 6•8 years ago
|
||
mozreview-review-reply |
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•8 years ago
|
||
(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 hidden (mozreview-request) |
Assignee | ||
Comment 9•8 years ago
|
||
mozreview-review-reply |
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•8 years ago
|
||
(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).
Assignee | ||
Comment 11•8 years ago
|
||
I am updating the commit with <div id="propertyContainer" dir=ltr">
Assignee | ||
Comment 12•8 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Updated•8 years ago
|
Assignee: nobody → tomer.moz.bugs
Status: NEW → ASSIGNED
Comment 14•8 years ago
|
||
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•8 years ago
|
||
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dc649ffb0370
[computed style][rtl] RTL support for devtools computed style (un-rtl) r=jdescottes
Comment 16•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Comment 17•8 years ago
|
||
I can confirm this is fixed on latest Nightly build.
Comment 19•8 years ago
|
||
How about uplift?
Assignee | ||
Updated•8 years ago
|
status-firefox50:
--- → affected
status-firefox51:
--- → affected
status-firefox52:
--- → affected
status-firefox-esr45:
--- → affected
Comment 20•8 years ago
|
||
Too late for 50 & 51
Comment 21•8 years ago
|
||
Tomer, is it something you want to uplift? If yes, you have to go in the attachment page and fill the details
Flags: needinfo?(tomer.moz.bugs)
Updated•7 years ago
|
Flags: needinfo?(tomer.moz.bugs)
Updated•6 years ago
|
Product: Firefox → DevTools
Updated•4 years ago
|
Component: Inspector: Computed → Inspector
You need to log in
before you can comment on or make changes to this bug.
Description
•