Closed Bug 1454607 Opened 2 years ago Closed 2 years ago

[macOS] "Zoom" and "Edit" labels are no longer aligned to text in other items in the hamburger panel

Categories

(Firefox :: Theme, defect, P3)

defect

Tracking

()

RESOLVED FIXED
Firefox 61
Tracking Status
firefox-esr52 --- unaffected
firefox59 --- wontfix
firefox60 --- fixed
firefox61 --- fixed

People

(Reporter: Gijs, Assigned: adw)

References

Details

(Keywords: regression)

Attachments

(2 files)

Seems we've shipped 58 and 59 with this and nobody noticed so probably not super awful, but it does look a bit odd, and IMO ideally we should fix it. Drew, seems bug 1409377 broke this (I *think* by accident, as I can't find a reference in the comments in that bug that indicate we're deliberately moving the text to the left) - do you have cycles to fix?
Flags: needinfo?(adw)
Not sure I can look at this soon, so if anyone else wants to take it, please feel free.
Assignee: nobody → adw
Status: NEW → ASSIGNED
Flags: needinfo?(adw)
Priority: -- → P3
Hmm, even when I revert the bug 1409377 patch, the labels still aren't aligned.
(In reply to Drew Willcoxon :adw from comment #2)
> Hmm, even when I revert the bug 1409377 patch, the labels still aren't
> aligned.

Huh. Perhaps other styles changed since then? I reran mozregression and I'm seeing the same result.
It does seem like something else changed too.  The related CSS has changed slightly since then, but I'm still not sure what the difference is.  (But I did a poor-man's bisect by checking the Nightlies before and after bug 1409377 landed, and the problem did start on a subsequent Nightly.)
The "12px toolbarbutton padding" in the comment here seems to be wrong:

> panelmultiview .toolbaritem-combined-buttons > spacer.before-label {
>   width: 36px; /* 12px toolbarbutton padding + 16px icon + 8px label padding start */
> }

It's actually 18px -- at least in the other non-combined buttons, there's 18 pixels of space between the edge of the panel and the left edge of the button icons.  If I revert the patch and change this to `width: 42px`, the text is aligned.  Still trying to work out why/how.  And this line hasn't changed since before the patch.
Oh: https://dxr.mozilla.org/mozilla-central/rev/a0c804993efc599a95e97bea39fa1528fd0195d8/browser/themes/osx/customizableui/panelUI.css#11

> .subviewbutton {
>   padding-inline-start: 18px;
> }

Looks like this rule is only in the osx panelUI.css.
Aha.  I didn't completely revert the patch.  I left the "before-label" class on the spacer before the label.  So this rule in the shared CSS:

> panelmultiview .toolbaritem-combined-buttons > spacer.before-label {
>   width: 36px; /* 12px toolbarbutton padding + 16px icon + 8px label padding start */
> }

Was more specific than this rule in the osx CSS:

> panelmultiview .toolbaritem-combined-buttons > spacer {
>   width: 42px; /* 18px toolbarbutton padding + 16px icon + 8px label padding start */
> }

So this is actually due entirely to that bug.
Comment on attachment 8969418 [details]
Bug 1454607 - [macOS] "Zoom" and "Edit" labels are no longer aligned to text in other items in the hamburger panel.

https://reviewboard.mozilla.org/r/238168/#review244138

Nice, thanks for digging into this!
Attachment #8969418 - Flags: review?(gijskruitbosch+bugs) → review+
Pushed by dwillcoxon@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/67c3eb947f52
[macOS] "Zoom" and "Edit" labels are no longer aligned to text in other items in the hamburger panel. r=Gijs
https://hg.mozilla.org/mozilla-central/rev/67c3eb947f52
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Please request Beta approval on this when you get a chance.
Attached patch Beta patchSplinter Review
Approval Request Comment
[Feature/Bug causing the regression]:
Bug 1409377 regressed this

[User impact if declined]:
On macOS only, the "Zoom" and "Edit" labels in the app menu will remain misaligned with the rest of the labels in the menu

[Is this code covered by automated tests?]:
No

[Has the fix been verified in Nightly?]:
This bug hasn't been marked verified, but I (the patch author) have verified it

[Needs manual test from QE? If yes, steps to reproduce]:
No

[List of other uplifts needed for the feature/fix]:
None

[Is the change risky?]:
No

[Why is the change risky/not risky?]:
It's a simple one-line CSS change

[String changes made/needed]:
None
Attachment #8970261 - Flags: approval-mozilla-beta?
Comment on attachment 8970261 [details] [diff] [review]
Beta patch

Low risk, Beta60+
Attachment #8970261 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.