The new about:performance UI is missing proper RTL support

VERIFIED FIXED in Firefox 64

Status

()

defect
VERIFIED FIXED
6 months ago
5 months ago

People

(Reporter: itiel_yn8, Assigned: florian)

Tracking

(Blocks 2 bugs, {rtl})

unspecified
mozilla65
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 verified, firefox65 verified)

Details

(URL)

Attachments

(2 attachments)

(Reporter)

Description

6 months ago
Posted image 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
(Assignee)

Comment 1

6 months ago
(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.
(Assignee)

Updated

6 months ago
Duplicate of this bug: 1504236
(Assignee)

Updated

6 months ago
(Reporter)

Comment 3

6 months ago
(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.
(Reporter)

Comment 4

6 months ago
(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.
(Assignee)

Comment 5

6 months ago
Posted patch PatchSplinter Review
Attachment #9023458 - Flags: review?(felipc)
(Assignee)

Updated

6 months ago
Assignee: nobody → florian
Status: NEW → ASSIGNED
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`?
Attachment #9023458 - Flags: review?(felipc) → review+

Comment 7

5 months ago
Pushed by florian@queze.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/45c304d2dd2f
fix about:performance styling to support RTL, r=felipe.
(Assignee)

Comment 8

5 months ago
(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

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/45c304d2dd2f
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
(Assignee)

Comment 10

5 months ago
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
Attachment #9023458 - Flags: approval-mozilla-beta?
(Reporter)

Comment 11

5 months ago
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?
(Reporter)

Comment 12

5 months ago
(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?
(Reporter)

Comment 13

5 months ago
(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.
(Assignee)

Comment 14

5 months ago
(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.
(Reporter)

Comment 15

5 months ago
(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.
(Reporter)

Comment 16

5 months ago
(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)
Flags: qe-verify+
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.
Attachment #9023458 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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?
Flags: needinfo?(florian)
(Assignee)

Comment 20

5 months ago
(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.
Flags: needinfo?(florian)
(Reporter)

Comment 21

5 months ago
Looking great now on Nightly after bug 1507114 was fixed.
Status: RESOLVED → VERIFIED
Verified fixed too on Firefox Beta 64.0b10 under Windows 10 (x64), Ubuntu 18.04 (x64) and macOS 10.12.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.