1323553 - Add RTL to JSON Viewer

Status: RESOLVED FIXED

(DevTools :: JSON Viewer, defect)

Description

[Tomer Cohen :tomer]

Attachment: example.json

Steps to reproduce: 

a. Launch an RTL Firefox build.
b. Open the attached file in Firefox (example file borrowed from json.org)

Current result: 
No RTL support at all. For example, the searchbox placeholder is "סינון JSON" in Hebrew Nightly. The word "סינון" should be the rightmost and the text should be right-aligned. 

I am planning to provide patch in few days.

Comment 1

[Tomer Cohen :tomer]

Attachment: Bug 1323553 - Add RTL to JSON Viewer

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

Comment 2

[Jan Honza Odvarko [:Honza] (always need-info? me)]

I applied the attached patch and tested using Force RTL extension [1], but I don't see the search-box placeholder aligned to the right (running on Win 10).

Does it work on your machine?

Honza

[1] https://github.com/mikedeboer/ForceRTL

Comment 3

[Tomer Cohen :tomer]

Comment on attachment 8823030 [details]
Bug 1323553 - Add RTL to JSON Viewer

https://reviewboard.mozilla.org/r/101676/#review102148

::: devtools/client/jsonview/css/general.css:27
(Diff revision 1)
>    height: 100%;
>  }
>  
> +#content:-moz-locale-dir(rtl) {
> +  direction: rtl;
> +}

This indeed won't work, because the page is generated by JavaScript. I'll br trying to inject the direction into the page `<body>`.

Comment 4

[Tomer Cohen :tomer]

Comment on attachment 8823030 [details]
Bug 1323553 - Add RTL to JSON Viewer

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

Comment 5

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8823030 [details]
Bug 1323553 - Add RTL to JSON Viewer

https://reviewboard.mozilla.org/r/101678/#review102172

Nice, works for me now!

R+, but please fix one inline comment.

Thanks,
Honza

::: devtools/client/jsonview/converter-child.js:233
(Diff revision 2)
> +    let chromeReg = Cc["@mozilla.org/chrome/chrome-registry;1"]
> +                        .getService(Ci.nsIXULChromeRegistry);
> +    let dir = chromeReg.isLocaleRTL("global") ? "rtl" : "ltr";
> +
>      return "<!DOCTYPE html>\n" +
> -      "<html platform=\"" + os + "\" class=\"" + themeClassName + "\">" +
> +      "<html platform=\"" + os + "\" class=\"" + themeClassName + 

nit: remove whitespace at the end of the line

Comment 6

[Tomer Cohen :tomer]

Comment on attachment 8823030 [details]
Bug 1323553 - Add RTL to JSON Viewer

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

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/afd84580caab
Add RTL to JSON Viewer r=Honza

Comment 8

[Carsten Book [:Tomcat]]

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