1204609 - Disabled badged button icons do not dim (change opacity / grey out)

Status: RESOLVED FIXED

(Firefox :: Theme, defect, P1)

Description

[Markus Jaritz [:designakt] (UX)]

Attachment: Hello_on_Win10_private_notGrey.PNG

Comment 1

[Mark Banner (:standard8)]

Gijs, any idea if there's specific theme items for Windows 10?

Comment 2

[Mark Banner (:standard8)]

(In reply to Mark Banner (:standard8) from comment #1)
> Gijs, any idea if there's specific theme items for Windows 10?

and now with the needinfo

Comment 3

[:Gijs (he/him)]

I don't really understand the summary of this bug. There's no description. I'm assuming that this is about the chat bubble icon for the hello button (but there are a lot of buttons in that screenshot) and specifically the color of the outline. These are the relevant image files on Windows for Hello:

https://dxr.mozilla.org/mozilla-central/source/browser/themes/windows/loop/toolbar.png (win10+)

https://dxr.mozilla.org/mozilla-central/source/browser/themes/windows/loop/toolbar-win8.png (win8)

https://dxr.mozilla.org/mozilla-central/source/browser/themes/windows/loop/toolbar-aero.png (win7/vista)

https://dxr.mozilla.org/mozilla-central/source/browser/themes/windows/loop/toolbar-XP.png (win XP)
https://dxr.mozilla.org/mozilla-central/source/browser/themes/windows/loop/toolbar-lunaSilver.png (win XP luna silver special snowflake!)

Seems like the color of the icon without the smilie is always the same as the one with the smilie. So no idea what the issue is. The person who made these assets clearly intended this. Unless the Windows ones weren't updated or something, but there's no dep/blocking bug so I have no idea where that happened.

In general, if filing bugs and/or requesting needinfo, please provide more context. :-(

Comment 4

[Fernando Campo (:fcampo)]

Attachment: hello_private_nav_macOS

Comment 5

[Fernando Campo (:fcampo)]

What I understand from the bug, is that the icon for Hello is lighter for private navigation than the original dark grey for standard navigation (no matter if smiley or not, that's just the specific design for the system). This happens on all platforms except for Win10 (added attachment 8689488 [details] with OSX to compare).

So the question is if you're aware of a specific theme for private navigation in Win10, so UX can create specific lighter grey icon for it.

Sorry for the lack of description, hope this helps.

Comment 6

[Markus Jaritz [:designakt] (UX)]

Thanks Fernando for clarifying and thanks for adding the Mac-Screenshot.
In Private Mode Hello is inactive, but on Win10 the Hello-icon is not greyed out as it is on Mac which seams inconsistent across platforms, and inconsistent in how we show deactivated states of toolbar icons. (usually greyed out)

It seams the greyed out state is done programmatically by setting opacity of the icon to 0.4. At least, this is how the deactivated bookmark-star and share-plane appear on a new tab.
The difference that may cause this is that the XUL in the Hello button looks different than for other buttons (toolbarbutton > **xul:stack** > xul:image.toolbarbutton-icon), which is why the css rule controlling opacity does not apply. 

see chrome://browser/skin/browser.css line 1642-1648:
> #main-window:not([customizing]) .toolbarbutton-1[disabled=true] > .toolbarbutton-icon,
> #main-window:not([customizing]) .toolbarbutton-1[disabled=true] > .toolbarbutton-menu-dropmarker,
> #main-window:not([customizing]) .toolbarbutton-1[disabled=true] > .toolbarbutton-menubutton-dropmarker,
> #main-window:not([customizing]) .toolbarbutton-1[disabled=true] > .toolbarbutton-menubutton-button > .toolbarbutton-icon,
> #main-window:not([customizing]) .toolbarbutton-1 > .toolbarbutton-menubutton-button[disabled=true] > .toolbarbutton-icon {
>   opacity: .4;
> }

And sorry Gijs, reading my report again I realize that I should have added more context and a second screenshot for comparison.

Comment 7

[:Gijs (he/him)]

(In reply to Fernando Campo (:fcampo) from comment #5)
> What I understand from the bug, is that the icon for Hello is lighter for
> private navigation than the original dark grey for standard navigation (no
> matter if smiley or not, that's just the specific design for the system).
> This happens on all platforms except for Win10 (added attachment 8689488 [details]
> [details] with OSX to compare).

Just so we're clear here, "OSX" is not the same as "all platforms except for Win10", especially not when you specifically call out "Windows 10" as opposed to "Windows". The bug summary and initial questions seemed to indicate this was about Windows 10 vs. other versions of Windows and/or everything else, but it turns out that is not true at all. The reality is that this is currently not working anywhere *except* on OS X, because all versions of Windows and Linux use the selector mentioned in comment #6, which as comment #6 notes does not apply to badged buttons because they have a stack:

https://dxr.mozilla.org/mozilla-central/rev/a2f83cbe53ac4009afa4cb2b0b8f549289b23eeb/browser/themes/linux/browser.css#819-825

https://dxr.mozilla.org/mozilla-central/rev/a2f83cbe53ac4009afa4cb2b0b8f549289b23eeb/browser/themes/windows/browser.css#675-681

The correct fix would likely be updating these:

#main-window:not([customizing]) .toolbarbutton-1[disabled=true] > .toolbarbutton-menubutton-button > .toolbarbutton-icon,

to look like:

#main-window:not([customizing]) .toolbarbutton-1[disabled=true] > :-moz-any(.toolbarbutton-menubutton-button, .toolbarbutton-badge-stack) > .toolbarbutton-icon,

(cf. https://dxr.mozilla.org/mozilla-central/rev/a2f83cbe53ac4009afa4cb2b0b8f549289b23eeb/browser/themes/linux/browser.css#576 )

Please doublecheck that that works and then get Dão to review this so we're sure I'm not missing something else here (which I very well could be...).

Comment 8

[Fernando Campo (:fcampo)]

priority cleaning

Comment 9

[Hector Zhao [:hectorz]]

This is not a Loop specific problem. For example, any browserAction button created by WebExtensions is a badged button, and there's no visual indication when one is disabled.

Comment 10

[:Gijs (he/him)]

This selector has now moved and the bug now also applies to OS X. The new location of the selector is:

https://dxr.mozilla.org/mozilla-central/rev/b21b974d60d3075ae24f6fb1bae75d0f122f28fc/browser/themes/shared/toolbarbuttons.inc.css#27-35

Fixing this should be as simple as adding another selector that matches for badged buttons.

Comment 11

[Dale Harvey (:daleharvey)]

Ill take this

Comment 12

[Dale Harvey (:daleharvey)]

Attachment: Ensure disabled extension badges are dimmed

Comment 13

[Dão Gottwald [:dao]]

Comment on attachment 8873381 [details] [diff] [review]
Ensure disabled extension badges are dimmed

Can "#PanelUI-menu-button[disabled=true] > .toolbarbutton-badge-stack > .toolbarbutton-icon," be removed now? I don't understand /* specialcase the overflow and the hamburger button so they show up disabled in customize mode. */ since the hamburger button doesn't get disabled in customize mode.

Comment 14

[:Gijs (he/him)]

(In reply to Dão Gottwald [::dao] from comment #13)
> Comment on attachment 8873381 [details] [diff] [review]
> Ensure disabled extension badges are dimmed
> 
> Can "#PanelUI-menu-button[disabled=true] > .toolbarbutton-badge-stack >
> .toolbarbutton-icon," be removed now?

No, because in customize mode the other selector doesn't match because it starts with #main-window:not([customizing]) .

>  I don't understand /* specialcase the
> overflow and the hamburger button so they show up disabled in customize
> mode. */ since the hamburger button doesn't get disabled in customize mode.

It does in Photon.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/58119f6aff8f
Ensure disabled extension badges are dimmed. r=dao

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/58119f6aff8f

Comment 17

[Ionuț Goldan [:igoldan]]

This got in late, but no improvement should be overlooked.

== Change summary for alert #7305 (as of June 01 2017 14:19 UTC) ==

Improvements:

  3%  damp summary windows10-64 opt e10s     277.92 -> 269.94

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=7305

Comment 18

[Dão Gottwald [:dao]]

(In reply to Ionuț Goldan [:igoldan], Performance Sheriffing from comment #17)
> This got in late, but no improvement should be overlooked.
> 
> == Change summary for alert #7305 (as of June 01 2017 14:19 UTC) ==
> 
> Improvements:
> 
>   3%  damp summary windows10-64 opt e10s     277.92 -> 269.94
> 
> For up to date results, see:
> https://treeherder.mozilla.org/perf.html#/alerts?id=7305

I don't know what damp is, but it makes no sense that this patch would improve performance in any situation.

Comment 19

[:Gijs (he/him)]

(In reply to Dão Gottwald [::dao] from comment #18)
> (In reply to Ionuț Goldan [:igoldan], Performance Sheriffing from comment
> #17)
> > This got in late, but no improvement should be overlooked.
> > 
> > == Change summary for alert #7305 (as of June 01 2017 14:19 UTC) ==
> > 
> > Improvements:
> > 
> >   3%  damp summary windows10-64 opt e10s     277.92 -> 269.94
> > 
> > For up to date results, see:
> > https://treeherder.mozilla.org/perf.html#/alerts?id=7305
> 
> I don't know what damp is, but it makes no sense that this patch would
> improve performance in any situation.

I agree with Dão. I mean, in theory I guess it could be the case that the selector now overrides some other selector that was costly, or that it avoids a second reflow/paint somewhere? But that possibility seems very remote...

From: https://wiki.mozilla.org/Buildbot/Talos/Tests#DAMP

> measuring: Developer Tools toolbox startup, shutdown, and reload performance 

The alert points to:

https://treeherder.mozilla.org/index.html#/jobs?repo=mozilla-inbound&fromchange=9c0c9ed34b0717eccd27a03b839b54258696c959&tochange=20250e1b5b64cbb4ac31ffb2ec253416ec460b6d

which doesn't contain this bug. Also, the subtest listing ( https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-inbound&originalRevision=9c0c9ed34b0717eccd27a03b839b54258696c959&newProject=mozilla-inbound&newRevision=20250e1b5b64cbb4ac31ffb2ec253416ec460b6d&originalSignature=f687ee347bcbdc55828148af029908049de69b7f&newSignature=f687ee347bcbdc55828148af029908049de69b7f&framework=1 ) doesn't show confidence in *any* results. So on the whole, I would expect this improvement either doesn't exist or was caused by a different bug. Ionut, can you have another look?

Comment 20

[Ionuț Goldan [:igoldan]]

Surely. :dao 's comment made me do retriggers and backfills so I get this right.
Indeed, a lot of important data points started to come in.

Comment 21

[Ionuț Goldan [:igoldan]]

The damp improvement is, as ::dao and :Gijs stated, unrelated to this bug. For more details, check https://treeherder.mozilla.org/perf.html#/alerts?id=6992
Thanks for your quick feedback!