Add RTL support for the Toolbox Options

VERIFIED FIXED in Firefox 53

Status

defect
P3
normal
VERIFIED FIXED
3 years ago
Last year

People

(Reporter: itiel_yn8, Assigned: tomer)

Tracking

(Blocks 1 bug, {rtl})

unspecified
Firefox 53
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

()

Attachments

(2 attachments)

The Toolbox Options panel on RTL builds is currently not aligned to the right, which makes reading strings (containing both Hebrew and English words) unreadable.

Attached current look of the Toolbox Options on a Hebrew build.
Blocks: dt-rtl
Component: Developer Tools → Developer Tools: Framework
Priority: -- → P3
Comment on attachment 8818611 [details]
Bug 1323504 - Add RTL support for the Toolbox Options

https://reviewboard.mozilla.org/r/98624/#review99606

::: devtools/client/framework/toolbox-options.xhtml:11
(Diff revision 1)
>  <!ENTITY % toolboxDTD SYSTEM "chrome://devtools/locale/toolbox.dtd" >
>   %toolboxDTD;
> +<!ENTITY % globalDTD SYSTEM "chrome://global/locale/global.dtd">
> + %globalDTD;
>  ]>
> -<html xmlns="http://www.w3.org/1999/xhtml">
> +<html xmlns="http://www.w3.org/1999/xhtml" dir="&locale.dir;">

Should I replace it with dir="", as suggested on [bug 1323041 comment 6]?

[bug 1323041 comment 6]: https://bugzilla.mozilla.org/show_bug.cgi?id=1323041#c6?
Attachment #8818611 - Flags: review?(jryans)
Comment on attachment 8818611 [details]
Bug 1323504 - Add RTL support for the Toolbox Options

https://reviewboard.mozilla.org/r/98624/#review99606

> Should I replace it with dir="", as suggested on [bug 1323041 comment 6]?
> 
> [bug 1323041 comment 6]: https://bugzilla.mozilla.org/show_bug.cgi?id=1323041#c6?

Yes, I think you should make this change.

I'll redirect your review to :jdescottes, who has worked with our RTL support more recently.
Attachment #8818611 - Flags: review?(jryans) → review?(jdescottes)
Comment on attachment 8818611 [details]
Bug 1323504 - Add RTL support for the Toolbox Options

https://reviewboard.mozilla.org/r/98624/#review100112

r+ with the global.dtd removed.

Note that the "camera" icon overlaps with the text of the "Screenshot Behvior". 
Could be nice to add a #screenshot-icon:dir(rtl) selector to fix this, but not blocking the review on this.
Attachment #8818611 - Flags: review?(jdescottes) → review+
Comment on attachment 8818611 [details]
Bug 1323504 - Add RTL support for the Toolbox Options

https://reviewboard.mozilla.org/r/98624/#review100112

The fix for the camera icon involves changing `left:50%` on `.devtools-button:empty::before, .devtools-toolbarbutton:not([label]):not([disabled]) > image` rule on `common.css`, so I'm considering it unsafe because it could affect other parts of devtools. I'll create another bug to fix it.
Blocks: 1324746
Looks good. 
We can't autoland your changeset until you mark the issue you opened on the review as "fixed" -> https://reviewboard.mozilla.org/r/98624/#comment128690

Can you update it? Thanks!
Flags: needinfo?(tomer.moz.bugs)
(In reply to Julian Descottes [:jdescottes] from comment #7)
> We can't autoland your changeset 
Sorry.
Flags: needinfo?(tomer.moz.bugs)
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c53a4c48e3f8
Add RTL support for the Toolbox Options r=jdescottes
https://hg.mozilla.org/mozilla-central/rev/c53a4c48e3f8
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Depends on: 1334956
QA Whiteboard: [good first verify]
Yup, I can confirm this is fixed in latest Nightly.
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.