1253323 - Fix direction of heap-tree-number in RTL

Status: RESOLVED FIXED

(DevTools :: Memory, defect, P3)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

See https://bugzilla.mozilla.org/show_bug.cgi?id=1224192#c4

See attachment 8726122 [details].

Comment 1

[Steve Chung [:steveck]]

Attachment: Bug 1253323 - Fix direction of heap-tree-number in RTL.

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

Comment 2

[Tim Nguyen :ntim]

https://reviewboard.mozilla.org/r/69538/#review66738

I don't see a change locally when applying the patch. It doesn't seem to look like the expected results: https://bug1253324.bmoattachments.org/attachment.cgi?id=8777830

Comment 3

[Tim Nguyen :ntim]

Comment on attachment 8778183 [details]
Bug 1253323 - Fix direction of heap-tree-number in RTL.

https://reviewboard.mozilla.org/r/69538/#review66740

Comment 4

[Steve Chung [:steveck]]

Attachment: rtl.jpeg

Hi Tim, the attached image is the before/after comparison from my side. BTW the force rtl addon does not change the direction in memory/dominator view, so I forced theme-body's direction to rtl via remote debugger. May I know how you verify this issue?

Comment 5

[Tim Nguyen :ntim]

(In reply to Steve Chung [:steveck] from comment #4)
> Created attachment 8778824 [details]
> rtl.jpeg
> 
> Hi Tim, the attached image is the before/after comparison from my side. BTW
> the force rtl addon does not change the direction in memory/dominator view,
> so I forced theme-body's direction to rtl via remote debugger. May I know
> how you verify this issue?

Oh I see the change now, sorry. Reflagging for review.

Comment 6

[Tim Nguyen :ntim]

Comment on attachment 8778183 [details]
Bug 1253323 - Fix direction of heap-tree-number in RTL.

https://reviewboard.mozilla.org/r/69538/#review67486

::: devtools/client/themes/memory.css:439
(Diff revision 1)
>    /**
>     * Flex: contains several subcolumns, which need to be laid out horizontally.
>     * These subcolumns may have specific widths or need to flex.
>     */
>    display: flex;
>    text-align: end;

Maybe it's better to change text-align here to `right` to match the expected results.
Would be worth a comment as well:
/* Make sure units/decimals/... are always vertically aligned even in RTL locales */

or something like that.

::: devtools/client/themes/memory.css:492
(Diff revision 1)
>  
>  .heap-tree-number {
>    padding-inline-start: 3px;
> +  flex: 1;
> +  color: var(--theme-content-color3);
> +  direction: ltr;

Can you add a comment ?
/* Make sure number doesn't appear backwards on RTL locales */

::: devtools/client/themes/memory.css:493
(Diff revision 1)
>  .heap-tree-number {
>    padding-inline-start: 3px;
> +  flex: 1;
> +  color: var(--theme-content-color3);
> +  direction: ltr;
> +  text-align: left;

Why is the text-align rule needed? Now the units/decimals/... are no longer aligned.

Comment 7

[Steve Chung [:steveck]]

Comment on attachment 8778183 [details]
Bug 1253323 - Fix direction of heap-tree-number in RTL.

https://reviewboard.mozilla.org/r/69538/#review67486

> Maybe it's better to change text-align here to `right` to match the expected results.
> Would be worth a comment as well:
> /* Make sure units/decimals/... are always vertically aligned even in RTL locales */
> 
> or something like that.

Should we change the text align to right for the most of tree items here, or just for btyes?

> Why is the text-align rule needed? Now the units/decimals/... are no longer aligned.

If we set the text-align to right at heap-tree-item-total-bytes, then we can remove this part for sure.

Comment 8

[Steve Chung [:steveck]]

Comment on attachment 8778183 [details]
Bug 1253323 - Fix direction of heap-tree-number in RTL.

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

Comment 9

[Tim Nguyen :ntim]

magicp, can you download one if these builds and tell me if the fix works as expected ?
Thanks!

OSX: https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-475b982aa77b45b9dc43479a1aa520fe3a97dd3e/try-macosx64/

Windows: https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-475b982aa77b45b9dc43479a1aa520fe3a97dd3e/try-win32/

Linux: https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-475b982aa77b45b9dc43479a1aa520fe3a97dd3e/try-linux/

Comment 10

[magicp]

(In reply to Tim Nguyen :ntim (use needinfo?) from comment #9)
> magicp, can you download one if these builds and tell me if the fix works as
> expected ?
> Thanks!
> 
> OSX:
> https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-
> 475b982aa77b45b9dc43479a1aa520fe3a97dd3e/try-macosx64/
> 
> Windows:
> https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-
> 475b982aa77b45b9dc43479a1aa520fe3a97dd3e/try-win32/
> 
> Linux:
> https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-
> 475b982aa77b45b9dc43479a1aa520fe3a97dd3e/try-linux/

Can I confirm RTL locale version? (e.g. Arabic) Because I don't believe to use Force RTL. Thank you in advance.

Comment 11

[Tim Nguyen :ntim]

I don't know how to generate localized builds unfortunately. Can you try installing one of these language packs over the build ? https://addons.mozilla.org/en-US/firefox/language-tools/
Or if that doesn't work, can you try setting dir=rtl with the browser toolbox on the mem tool body ?

Comment 12

[magicp]

Attachment: rtl-memory.png

I confirmed them using class="theme-body" dir="ltr"

heap-tree-number should have "padding-inline-end: 3px;"

Comment 13

[Tim Nguyen :ntim]

Comment on attachment 8778183 [details]
Bug 1253323 - Fix direction of heap-tree-number in RTL.

https://reviewboard.mozilla.org/r/69538/#review68594

::: devtools/client/themes/memory.css:490
(Diff revision 2)
>  .heap-tree-item-name {
>    white-space: nowrap;
>  }
>  
>  .heap-tree-number {
>    padding-inline-start: 3px;

Can you replace this line with padding-left: 3px;padding-right: 3px; ?

Comment 14

[Tim Nguyen :ntim]

(In reply to magicp from comment #12)
> Created attachment 8779979 [details]
> rtl-memory.png
> 
> I confirmed them using class="theme-body" dir="ltr"
> 
> heap-tree-number should have "padding-inline-end: 3px;"

Thanks for your help magicp!

Comment 15

[Steve Chung [:steveck]]

Comment on attachment 8778183 [details]
Bug 1253323 - Fix direction of heap-tree-number in RTL.

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

Comment 16

[Steve Chung [:steveck]]

Thanks for the suggestion!

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/35a1712d7f61
Fix direction of heap-tree-number in RTL. r=ntim

Comment 18

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

https://hg.mozilla.org/mozilla-central/rev/35a1712d7f61