Closed
Bug 1454607
Opened 6 years ago
Closed 6 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)
Firefox
Theme
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)
59 bytes,
text/x-review-board-request
|
Gijs
:
review+
|
Details |
1.04 KB,
patch
|
ritu
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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)
Assignee | ||
Comment 1•6 years ago
|
||
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)
Updated•6 years ago
|
Priority: -- → P3
Assignee | ||
Comment 2•6 years ago
|
||
Hmm, even when I revert the bug 1409377 patch, the labels still aren't aligned.
Reporter | ||
Comment 3•6 years ago
|
||
(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.
Assignee | ||
Comment 4•6 years ago
|
||
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.)
Assignee | ||
Comment 5•6 years ago
|
||
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.
Assignee | ||
Comment 6•6 years ago
|
||
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.
Assignee | ||
Comment 7•6 years ago
|
||
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 hidden (mozreview-request) |
Reporter | ||
Comment 9•6 years ago
|
||
mozreview-review |
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+
Comment 10•6 years ago
|
||
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
Comment 11•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/67c3eb947f52
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Comment 12•6 years ago
|
||
Please request Beta approval on this when you get a chance.
Assignee | ||
Comment 13•6 years ago
|
||
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+
Comment 15•6 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/37c3ffc007e5
You need to log in
before you can comment on or make changes to this bug.
Description
•