Closed Bug 1323504 Opened 4 years ago Closed 4 years ago

Add RTL support for the Toolbox Options

Categories

(DevTools :: Framework, defect, P3)

defect

Tracking

(firefox53 fixed)

VERIFIED FIXED
Firefox 53
Tracking Status
firefox53 --- fixed

People

(Reporter: itiel_yn8, Assigned: tomer)

References

(Blocks 1 open bug, )

Details

(Keywords: rtl)

Attachments

(2 files)

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
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: 4 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.