1322423 - about:debugging lacks RTL

Status: RESOLVED FIXED

(DevTools :: about:debugging, defect)

Description

[Tomer Cohen :tomer]

The page about:debugging is very similar to about:preferences in its design, but doesn't display well on RTL builds.

Comment 1

[Tomer Cohen :tomer]

Attachment: Bug 1322423 about:debugging lacks RTL

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

Comment 2

[Julian Descottes [:jdescottes]]

Comment on attachment 8817280 [details]
Bug 1322423  about:debugging lacks RTL

https://reviewboard.mozilla.org/r/97638/#review99748

Thanks for the patch! Looks almost good on RTL without any css change.

We do have a few `margin` in aboutdebugging.css which should use margin-inline-end/start instead:
- http://searchfox.org/mozilla-central/source/devtools/client/aboutdebugging/aboutdebugging.css#69
- http://searchfox.org/mozilla-central/source/devtools/client/aboutdebugging/aboutdebugging.css#117
- http://searchfox.org/mozilla-central/source/devtools/client/aboutdebugging/aboutdebugging.css#125
- http://searchfox.org/mozilla-central/source/devtools/client/aboutdebugging/aboutdebugging.css#159
- http://searchfox.org/mozilla-central/source/devtools/client/aboutdebugging/aboutdebugging.css#166

We can either deal with it here or log a follow-up issue.

Comment 3

[Tomer Cohen :tomer]

(In reply to Julian Descottes [:jdescottes] from comment #2)
> Comment on attachment 8817280 [details]
> Bug 1322423  about:debugging lacks RTL
> 
> https://reviewboard.mozilla.org/r/97638/#review99748
> 
> Thanks for the patch! Looks almost good on RTL without any css change.
> 
> We do have a few `margin` in aboutdebugging.css which should use
> margin-inline-end/start instead:

Currently these properties use margins with four values. Splitting to margin-inline-start/end will require to split these tuples into four different ones - margin-{block,inline}-{start,end} or margin-{top,bottom,inline-{start,end}}, which will make everything longer and less readable. Another option will be to use #nodeId:dir(rtl) selector to specify the opposite four-values tuples, which may be more readable still duplicates code. The third option is to replace the four-values tuples with three-values tuples (top left/right bottom) or even two-values tuples (top/bottom left/right) which may change the values of these properties when not fully symmetric, but is the most readable and future proof.


p.s., Please note that we can set multiple markers on the same file, as in 
http://searchfox.org/mozilla-central/rev/a6a339991dbc650bab3aefe939cd0e5ac302274a/devtools/client/aboutdebugging/aboutdebugging.css#69,117,125,159,166

Comment 4

[Julian Descottes [:jdescottes]]

I think it's not too bad here:
- .target-icon { margin: 0 5px 0 0; } can be replaced with a single margin-inline-end, the other values have no impact at the moment
- .target-status { margin: 4px 5px 0 0; } can be replaced with margin-top + margin-inline-end, no need to set the others to 0
- .addons-install-error { margin: 5px 4px 5px 0px; } can be replaced with { margin: 5px 4px; }, the difference is not really noticeable

The others are already using longhand properties so can be converted directly. Let me know if you prefer to land your changeset as it is or if you'd like to update the CSS first.

(In reply to Tomer Cohen :tomer from comment #3)
> p.s., Please note that we can set multiple markers on the same file, as in 
> http://searchfox.org/mozilla-central/rev/
> a6a339991dbc650bab3aefe939cd0e5ac302274a/devtools/client/aboutdebugging/
> aboutdebugging.css#69,117,125,159,166

Ah thanks, I didn't know that!

Comment 5

[Tomer Cohen :tomer]

Comment on attachment 8817280 [details]
Bug 1322423  about:debugging lacks RTL

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

Comment 6

[Julian Descottes [:jdescottes]]

Comment on attachment 8817280 [details]
Bug 1322423  about:debugging lacks RTL

https://reviewboard.mozilla.org/r/97638/#review99850

::: devtools/client/aboutdebugging/aboutdebugging.css:69
(Diff revision 2)
>    align-items: start;
>  }
>  
>  .target-icon {
>    height: 24px;
> -  margin: 0 5px 0 0;
> +  margin-inline-start: 5px;

should be margin-inline-end

::: devtools/client/aboutdebugging/aboutdebugging.css:126
(Diff revision 2)
>    box-sizing: border-box;
>    display: inline-block;
>  
>    min-width: 50px;
> -  margin: 4px 5px 0 0;
> +  margin-top: 4px;
> +  margin-inline-start: 5px;

should be margin-inline-end

::: devtools/client/aboutdebugging/aboutdebugging.css:161
(Diff revision 2)
>  
>  .addons-install-error {
>    background-color: #f3b0b0;
>    padding: 5px 10px;
> -  margin: 5px 4px 5px 0px;
> +  margin: 5px 0;
> +  margin-inline-start: 4px;

should be margin-inline-end

Comment 7

[Tomer Cohen :tomer]

Comment on attachment 8817280 [details]
Bug 1322423  about:debugging lacks RTL

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

Comment 8

[Julian Descottes [:jdescottes]]

Comment on attachment 8817280 [details]
Bug 1322423  about:debugging lacks RTL

https://reviewboard.mozilla.org/r/97638/#review99870

Great! Thanks a lot for updating the CSS as well.

Pushed to try : https://treeherder.mozilla.org/#/jobs?repo=try&revision=da86b8ed7e23ac6a889e837db350093670fb2764

Comment 9

[Julian Descottes [:jdescottes]]

Try is green, landing !

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f1929c6554f5
about:debugging lacks RTL r=jdescottes

Comment 11

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

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