1503680 - The new about:performance UI is missing proper RTL support

Status: VERIFIED FIXED

(Toolkit :: Performance Monitoring, defect)

Description

[Itiel]

Attachment: Screenshot

See attached for current implementation.

Things to fix in the current implementation:
1. The twisties should point to the left if not expanded
2. Icons should be on the right side of the names
3. Column text is offset to the left and not aligned with headers (covered by bug 1502951)
4. Mirror the 'Show in Add-ons Manager' icon

Things to consider for future work on about:performance (based on the specs):
1. "Link to a details page" (i) icon should be on the left side of the names
2. Energy Impact animation should start from right to left
3. The 'Pause', 'Close tab', 'Show in Add-ons Manager' icons should appear in the revese order

Comment 1

[Florian Quèze [:florian]]

(In reply to Itiel from comment #0)
> Created attachment 9021630 [details]
> Screenshot
> 
> See attached for current implementation.
> 
> Things to fix in the current implementation:
> 1. The twisties should point to the left if not expanded
> 2. Icons should be on the right side of the names
> 3. Column text is offset to the left and not aligned with headers (covered
> by bug 1502951)
> 4. Mirror the 'Show in Add-ons Manager' icon

I locally have a patch that fixes #1 and #2. I'm still working on #4, and I can't reproduce #3.

Comment 3

[Itiel]

(In reply to Florian Quèze [:florian] from comment #1)
... and I can't reproduce #3.

Maybe high DPI has something to do with it? I'm on 150% DPI on Windows 10.
Will test on a 100% DPI when I'll get home.

Comment 4

[Itiel]

(In reply to Itiel from comment #3)
> Maybe high DPI has something to do with it? I'm on 150% DPI on Windows 10.
> Will test on a 100% DPI when I'll get home.

Nope, that's not it. See bug 1502951 comment 5.

Comment 5

[Florian Quèze [:florian]]

Attachment: Patch

Comment 6

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 9023458 [details] [diff] [review]
Patch

Review of attachment 9023458 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/aboutperformance/content/aboutPerformance.xhtml
@@ +194,5 @@
> +      .addon-icon::after {
> +        background-image: url("chrome://global/skin/icons/shortcut.svg");
> +        background-size: 16px;
> +      }
> +      .addon-icon:dir(rtl)::after {

I know it's meaningless for the .close-icon, but if more icons are added in the future, don't you want this :dir(trl) selector to apply for any `.action-icon` instead of just `.addon-icon`?

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/45c304d2dd2f
fix about:performance styling to support RTL, r=felipe.

Comment 8

[Florian Quèze [:florian]]

(In reply to :Felipe Gomes (needinfo me!) from comment #6)

Thanks for the reviews!

> ::: toolkit/components/aboutperformance/content/aboutPerformance.xhtml
> @@ +194,5 @@
> > +      .addon-icon::after {
> > +        background-image: url("chrome://global/skin/icons/shortcut.svg");
> > +        background-size: 16px;
> > +      }
> > +      .addon-icon:dir(rtl)::after {
> 
> I know it's meaningless for the .close-icon, but if more icons are added in
> the future, don't you want this :dir(trl) selector to apply for any
> `.action-icon` instead of just `.addon-icon`?

I went back and forth on this one. I initially had it in the action-icon class, then realized reversing all the close icons was pointless, and I was wondering if that transform has a cost.
The only other icon I'm currently planning to add there is the pause icon, which is also symetrical, so I don't think it matters much. And the rtl css rule is close enough to the rule setting the background-image that it's likely to be seen by someone adding an icon in the future. I don't think it's too much of a footgun.

Comment 9

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/45c304d2dd2f

Comment 10

[Florian Quèze [:florian]]

Comment on attachment 9023458 [details] [diff] [review]
Patch

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: new about:performance

User impact if declined: Icons in the wrong place and/or wrong direction for RTL users

Is this code covered by automated tests?: No

Has the fix been verified in Nightly?: No

Needs manual test from QE?: Yes

If yes, steps to reproduce: Ensure that the display looks good on RTL.

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): CSS only change. I hesitated between "low" and "medium" risk because the patch also affects LTR users, as the icons are moved to pseudo elements. Decided that it's "low" because if it's broken it should be very visible and easy to catch.

String changes made/needed: none

Comment 11

[Itiel]

RTL-wise this looks okay now, but no twisties are shown (and unable to exapnd an item, when relevant), also no Close Tab or Show in Add-ons Manager buttons.
Was that intended?

Comment 12

[Itiel]

(In reply to Itiel from comment #11)
> RTL-wise this looks okay now, but no twisties are shown (and unable to
> exapnd an item, when relevant), also no Close Tab or Show in Add-ons Manager
> buttons.
> Was that intended?

LTR is also affected, so this is likely has something to do with bug 1502951?

Comment 13

[Itiel]

(In reply to Itiel from comment #12)
> (In reply to Itiel from comment #11)
> > RTL-wise this looks okay now, but no twisties are shown (and unable to
> > exapnd an item, when relevant), also no Close Tab or Show in Add-ons Manager
> > buttons.
> > Was that intended?
> 
> LTR is also affected, so this is likely has something to do with bug 1502951?

Nope, that's bug 1196668. I'll be able to verify this bug after bug 1196668 is fixed.

Comment 14

[Florian Quèze [:florian]]

(In reply to Itiel from comment #13)

> Nope, that's bug 1196668. I'll be able to verify this bug after bug 1196668
> is fixed.

Did you mean to include 2 separate bug numbers? If not I'm confused as bug 1196668 is already marked as fixed.

Comment 15

[Itiel]

(In reply to Florian Quèze [:florian] from comment #14)
> Did you mean to include 2 separate bug numbers? If not I'm confused as bug
> 1196668 is already marked as fixed.

Oops, sorry, I meant bug 1506592.

Comment 16

[Itiel]

(In reply to Itiel from comment #15)
> (In reply to Florian Quèze [:florian] from comment #14)
> > Did you mean to include 2 separate bug numbers? If not I'm confused as bug
> > 1196668 is already marked as fixed.
> 
> Oops, sorry, I meant bug 1506592.

(Of course, unless I'm mistaken and this issue is another regression from bug 1196668)

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9023458 [details] [diff] [review]
Patch

[Triage Comment]
Fixes RTL issues with the new about:performance and un-blocks the uplift of bug 1502951. Approved for 64.0b10.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/afb34abc2e8b

Comment 19

[Catalin Sasca, Desktop Test Engineering [:csasca]]

I successfully reproduced the issue on Firefox Nightly 65.0a1 (2018-10-31) under Windows 10 (x64).

On latest Firefox Nightly 65.0a1 (2018-11-13), the icons are properly arranged in RTL mode, but I still encounter the issues stated in Comment 11. Is this right?

Comment 20

[Florian Quèze [:florian]]

(In reply to Sasca Catalin, QA [:csasca] from comment #19)

> On latest Firefox Nightly 65.0a1 (2018-11-13), the icons are properly
> arranged in RTL mode, but I still encounter the issues stated in Comment 11.
> Is this right?

This is a regression on nightly caused by bug 1196668. I filed bug 1507114 to fix it.

It should be possible to verify the fix on beta.

Comment 21

[Itiel]

Looking great now on Nightly after bug 1507114 was fixed.

Comment 22

[Catalin Sasca, Desktop Test Engineering [:csasca]]

Verified fixed too on Firefox Beta 64.0b10 under Windows 10 (x64), Ubuntu 18.04 (x64) and macOS 10.12.