Closed Bug 1219479 Opened 4 years ago Closed 4 years ago

Toolbarbuttons in overflow tooltip have increased icons

Categories

(Firefox :: Theme, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 45
Tracking Status
firefox43 --- unaffected
firefox44 + fixed
firefox45 --- fixed

People

(Reporter: arni2033, Assigned: Gijs)

References

Details

(Keywords: regression)

Attachments

(2 files, 2 obsolete files)

STR:   (Win7_64, Nightly 44, 32bit, ID 20151027030239, new profile)
1. Set DPI in your OS to 125% OR set layout.css.devPixelsPerPx -> 1.25 in firefox
2. Switch window to normal mode, reduce window's width to make it as narrow as possible
3. Click overflow button in toolbar

Result:       All items in overflow tooltip have increased icons.
Expectations: Normal sized icons

>   Pushlog_url: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=0733455f90374ddbd5bae81254cd864c955f6114&tochange=f911df85e80d06e3c7f0b2118f1c19417d92a633
Flags: needinfo?(gijskruitbosch+bugs)
Attachment #8681992 - Flags: review?(dao)
[Tracking Requested - why for this release]:
Very visible styling regression
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Flags: needinfo?(gijskruitbosch+bugs)
Comment on attachment 8681992 [details] [diff] [review]
fix hidpi styling of toolbar buttons in overflow menu,

>+.toolbarbutton-1[overflowedItem] > .toolbarbutton-icon,
>+.toolbarbutton-1[overflowedItem] > :-moz-any(.toolbarbutton-menubutton-button, .toolbarbutton-badge-stack) > .toolbarbutton-icon,

>+.toolbarbutton-1:-moz-any(@primaryToolbarButtons@)[overflowedItem] > .toolbarbutton-icon,
>+.toolbarbutton-1:-moz-any(@primaryToolbarButtons@)[overflowedItem] > :-moz-any(.toolbarbutton-menubutton-button, .toolbarbutton-badge-stack) > .toolbarbutton-icon,

These selectors don't work for nested buttons, do they?
Attachment #8681992 - Flags: review?(dao) → review-
Attachment #8683051 - Flags: review?(dao)
Attachment #8681992 - Attachment is obsolete: true
Comment on attachment 8683051 [details] [diff] [review]
fix hidpi styling of toolbar buttons in overflow menu,

>+.widget-overflow-list .toolbarbutton-1 > .toolbarbutton-icon,
>+.widget-overflow-list .toolbarbutton-1 > :-moz-any(.toolbarbutton-menubutton-button, .toolbarbutton-badge-stack) > .toolbarbutton-icon,
> toolbar .toolbarbutton-1 > .toolbarbutton-icon,
> toolbar .toolbarbutton-1 > :-moz-any(.toolbarbutton-menubutton-button, .toolbarbutton-badge-stack) > .toolbarbutton-icon {

At this point we should be using :-moz-any(toolbar, .widget-overflow-list). I also wonder if there's a sane way to utilize [cui-areatype="toolbar"] here which would cover both cases.
Attachment #8683051 - Flags: review?(dao)
(In reply to Dão Gottwald [:dao] from comment #5)
> I also wonder if there's a sane way to utilize [cui-areatype="toolbar"] here
> which would cover both cases.

Not without duplicating the selectors again, because it could be on the toolbarbutton itself, and yet it might not be, so you'd need something like:

[attr] .toolbarbutton-1 > .toolbarbutton-icon,
.toolbarbutton-1[attr] > .toolbarbutton-icon,

etc.

I can't think of a (sane) way to unify those selectors.
Attachment #8683051 - Attachment is obsolete: true
Comment on attachment 8683065 [details] [diff] [review]
fix hidpi styling of toolbar buttons in overflow menu,

I suppose you could now remove the !important that I added in attachment 8673229 [details] [diff] [review].
Attachment #8683065 - Flags: review?(dao) → review+
https://hg.mozilla.org/mozilla-central/rev/b37f781fb591
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
(In reply to Dão Gottwald [:dao] from comment #8)
> Comment on attachment 8683065 [details] [diff] [review]
> fix hidpi styling of toolbar buttons in overflow menu,
> 
> I suppose you could now remove the !important that I added in attachment 8673229 [details] [diff] [review].

This didn't work so well, it's still needed apparently. Could you please revert this? Feel free to land with rs=dao
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Dão Gottwald [:dao] from comment #11)
> (In reply to Dão Gottwald [:dao] from comment #8)
> > Comment on attachment 8683065 [details] [diff] [review]
> > fix hidpi styling of toolbar buttons in overflow menu,
> > 
> > I suppose you could now remove the !important that I added in attachment 8673229 [details] [diff] [review].
> 
> This didn't work so well, it's still needed apparently. Could you please
> revert this? Feel free to land with rs=dao

remote:   https://hg.mozilla.org/integration/fx-team/rev/c88a0ac15fa5
Flags: needinfo?(gijskruitbosch+bugs)
See Also: → 1222021
Depends on: 1222017
Arni2033, could you please verify that this issue is fixed as expected in the latest Nightly build? Thanks!
Flags: needinfo?(arni2033)
Tracked since it's a regression in the UI that will make icons look too big and reduce overall usability. Glad to know this is already fixed.
Dao, does this need to be uplifted to Aurora44? If yes, please nominate.
Flags: needinfo?(dao)
(In reply to Ritu Kothari (:ritu) from comment #15)
> Arni2033, could you please verify that this issue is fixed as expected in
> the latest Nightly build? Thanks!
I confirm that currently icons in overflow menu are of expected size

My Info:   Win7_64, Nightly 45, 32bit, ID 20151118030232
Flags: needinfo?(arni2033)
Flags: needinfo?(dao) → needinfo?(gijskruitbosch+bugs)
Comment on attachment 8683065 [details] [diff] [review]
fix hidpi styling of toolbar buttons in overflow menu,

Approval Request Comment
[Feature/regressing bug #]: bug 1214221
[User impact if declined]: icon sizes in the overflow panel are wrong
[Describe test coverage new/current, TreeHerder]: no, CSS only change, sadly.
[Risks and why]: low, CSS-only change. This regressed for 44 so we should make sure this gets fixed for 44.
[String/UUID change made/needed]: no.
Flags: needinfo?(gijskruitbosch+bugs)
Attachment #8683065 - Flags: approval-mozilla-aurora?
(when uplifting, please make sure to uplift both commits)
(In reply to arni2033 from comment #18)
> (In reply to Ritu Kothari (:ritu) from comment #15)
> > Arni2033, could you please verify that this issue is fixed as expected in
> > the latest Nightly build? Thanks!
> I confirm that currently icons in overflow menu are of expected size
> 
> My Info:   Win7_64, Nightly 45, 32bit, ID 20151118030232

Thanks! Setting status as verified based on comment 18.
Status: RESOLVED → VERIFIED
Comment on attachment 8683065 [details] [diff] [review]
fix hidpi styling of toolbar buttons in overflow menu,

This fix was verified on Nightly 11-18 build. Seems safe to uplift to Aurora44.
Attachment #8683065 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
These appear to have been uplifted to aurora earlier this morning, but never marked in the bug:
http://hg.mozilla.org/releases/mozilla-aurora/rev/cd65ff3817ef
http://hg.mozilla.org/releases/mozilla-aurora/rev/1ce71bdcff0e
Flags: needinfo?(cbook)
Flags: needinfo?(cbook)
> max-width: 32px !important /* bug 561154 */;

FWIW, this !important was breaking our extension, which also uses !important to fix the FF rules for us. Button is something that an extension is particularly likely to want to override.

Please see (again) whether you really can't find another solution. Thanks for understanding.
(In reply to Ben Bucksch (:BenB) from comment #24)
> > max-width: 32px !important /* bug 561154 */;
> 
> FWIW, this !important was breaking our extension, which also uses !important
> to fix the FF rules for us. Button is something that an extension is
> particularly likely to want to override.
> 
> Please see (again) whether you really can't find another solution. Thanks
> for understanding.

You can trivially override this rule with a selector like:

:-moz-any(#TabsToolbar, #nav-bar) #myextensionbuttonid > .toolbarbutton-icon

and/or similar if you're using a menu button. Please file a separate bug blocking bug 1214221 (not this bug, which is in principle not related) *with more details and suggestions about what you want us to do* if that doesn't work.
You need to log in before you can comment on or make changes to this bug.