1380345 - App update badge overlaps title bar when using compact themes

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect, P3)

Description

[Molly Howell (she/her) (mostly inactive)]

Attachment: screenshot.png

On Windows (haven't checked elsewhere) nightly, when the default theme is used and the density is set to compact, or either of the separate compact themes are used, the app update hamburger menu badge extends up into the window title bar by a pixel.

Not the worst thing that's ever happened, but I think the change to using the Windows accent color setting for the title bar makes it look just "off" enough that I could notice it.

Comment 1

[Robert Strong (they/them - no direct email)]

Doug, could you take this bug?

Comment 2

[Alex Thayer [:alexical] (she/her)]

Attachment: Bug 1380345 - Use clipped menu icon when showing update badges

As an alternative to using a border colored to match what's behind
it, as the color of that border needs to be maintained to match,
and the border can cut into other things. Since the border was
1px on high DPI displays, this does change how it looks slightly
under those circumstances, but see the images attached to the bug,
which is still visually pleasing.

Review commit: https://reviewboard.mozilla.org/r/179902/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/179902/

Comment 3

[Alex Thayer [:alexical] (she/her)]

Attachment: clippath_badge.png

Comment 4

[Alex Thayer [:alexical] (she/her)]

Attachment: mask_badge_compact_high_dpi.PNG

Comment 5

[:Gijs (he/him)]

I'll review some more tomorrow morning, but one question I have is whether we want to go back to the use:not(:target) stuff (rather than simply having 2 images). I think :jwatt told us to avoid it, but I don't know if the objections to using it apply here. :jwatt ? :-)

Comment 6

[Alex Thayer [:alexical] (she/her)]

Ah - good to know. I thought it seemed the better approach since the update icons might often get overlooked, and someone might update the menu icon without updating the duplicate badged menu icon. But I'm happy to go with whatever the convention is if that's the ruling.

Comment 7

[Jonathan Watt [:jwatt]]

Yeah, if your modifying the hamburger menu icon it would be better to avoid using the :not() thing since that icon is in the startup path (we don't want to slow that down).

That said I don't really understand why the mask is being added here.

Comment 8

[Jonathan Watt [:jwatt]]

Gijs just explained the masking on IRC (to clip the top layer of the hamburger to provide visual space between it and the overlapping badge).

Masking is expensive and in general we should try to avoid runtime work when we can. In this case that could be done by simply modifying the path path that makes up the top layer of the hamburger menu. I'd encourage you to do that instead of using a mask. (It may involve a little math to figure out the correct path data of course.)

Comment 9

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8908245 [details]
Bug 1380345 - Use clipped menu icon when showing update badges

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/179902/diff/1-2/

Comment 10

[:Gijs (he/him)]

Comment on attachment 8908245 [details]
Bug 1380345 - Use clipped menu icon when showing update badges

https://reviewboard.mozilla.org/r/179902/#review185426

::: browser/themes/shared/toolbarbutton-icons.inc.css:278
(Diff revision 2)
> +#PanelUI-menu-button[badge-status="update-available"],
> +#PanelUI-menu-button[badge-status="update-manual"],
> +#PanelUI-menu-button[badge-status="update-restart"] {

Should we do this for other badges as well?

Comment 11

[Alex Thayer [:alexical] (she/her)]

Other badges would need different paths, since they're not circular. I've been wondering for a while if there are plans to redo any of our other badges, since they seem to be a little bit visually out of sync with photon. Do you know of any plans for this?

Comment 12

[:Gijs (he/him)]

(In reply to Doug Thayer [:dthayer] from comment #11)
> Other badges would need different paths, since they're not circular. I've
> been wondering for a while if there are plans to redo any of our other
> badges, since they seem to be a little bit visually out of sync with photon.
> Do you know of any plans for this?

I'm not aware of any. Maybe Stephen knows.

I don't think we need to block this bug on that.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7798872848df
Use clipped menu icon when showing update badges r=Gijs

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7798872848df

Comment 17

[Stephen Horlander [:shorlander]]

(In reply to :Gijs (blocked reviews; no response to ni until Nov 2 (PTO)) from comment #12)
> (In reply to Doug Thayer [:dthayer] from comment #11)
> > Other badges would need different paths, since they're not circular. I've
> > been wondering for a while if there are plans to redo any of our other
> > badges, since they seem to be a little bit visually out of sync with photon.
> > Do you know of any plans for this?
> 
> I'm not aware of any. Maybe Stephen knows.
> 
> I don't think we need to block this bug on that.

Yes we do have plans for that. Will be looking into it post 57.