Closed Bug 1503680 Opened 6 years ago Closed 6 years ago

The new about:performance UI is missing proper RTL support

Categories

(Toolkit :: Performance Monitoring, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla65
Tracking Status
firefox64 --- verified
firefox65 --- verified

People

(Reporter: itiel_yn8, Assigned: florian)

References

()

Details

(Keywords: rtl)

Attachments

(2 files)

Attached 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
(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.
(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.
(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.
Attached patch PatchSplinter Review
Attachment #9023458 - Flags: review?(felipc)
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+
Pushed by florian@queze.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/45c304d2dd2f
fix about:performance styling to support RTL, r=felipe.
(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.
https://hg.mozilla.org/mozilla-central/rev/45c304d2dd2f
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
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?
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?
(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?
(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.
(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.
(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.
(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)
(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)
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.

Attachment

General

Creator:
Created:
Updated:
Size: