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

RESOLVED FIXED in Firefox 55

Status

()

Firefox
Theme
P1
normal
Rank:
28
RESOLVED FIXED
3 years ago
10 months ago

People

(Reporter: designakt, Assigned: daleharvey, Mentored)

Tracking

(Blocks: 2 bugs, {good-first-bug})

unspecified
Firefox 55
All
Unspecified
good-first-bug
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [photon-visual][p3])

Attachments

(3 attachments)

(Reporter)

Description

3 years ago
Created attachment 8660871 [details]
Hello_on_Win10_private_notGrey.PNG

Updated

3 years ago
Rank: 28
Priority: -- → P2
Whiteboard: [win10]
Gijs, any idea if there's specific theme items for Windows 10?
Component: General → Client
Assignee: nobody → fernando.campo
(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
Flags: needinfo?(gijskruitbosch+bugs)

Comment 3

3 years ago
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. :-(
Flags: needinfo?(gijskruitbosch+bugs)
Created attachment 8689488 [details]
hello_private_nav_macOS
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.
Flags: needinfo?(gijskruitbosch+bugs)
(Reporter)

Comment 6

3 years ago
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

3 years ago
(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...).
Flags: needinfo?(gijskruitbosch+bugs)
Summary: Button in Private Mode on Windows 10 is not grey as in other OS → Disabled badged button icons do not dim on Windows and Linux
Status: NEW → ASSIGNED
priority cleaning
Assignee: fernando.campo → nobody
Status: ASSIGNED → NEW
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.
Component: Client → Toolbars and Customization
Product: Hello (Loop) → Firefox

Updated

2 years ago
Component: Toolbars and Customization → Theme
Priority: P2 → P3
Whiteboard: [win10]
Mentor: kmaglione+bmo
Keywords: good-first-bug
OS: Windows 10 → Unspecified
Hardware: x86_64 → All
Blocks: 1311472

Comment 10

a year ago
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.
Blocks: 1325171

Updated

a year ago
Summary: Disabled badged button icons do not dim on Windows and Linux → Disabled badged button icons do not dim (change opacity / grey out)
Whiteboard: [photon-visual][p3]
Flags: qe-verify?
Whiteboard: [photon-visual][p3] → [reserve-photon-visual][p3]
Flags: qe-verify? → qe-verify-
Priority: P3 → P2
Whiteboard: [reserve-photon-visual][p3] → [photon-visual][p3]
(Assignee)

Comment 11

11 months ago
Ill take this
Assignee: nobody → dale

Updated

11 months ago
Status: NEW → ASSIGNED
Iteration: --- → 55.6 - May 29
Priority: P2 → P1

Updated

11 months ago
Iteration: 55.6 - May 29 → 55.7 - Jun 12
(Assignee)

Comment 12

11 months ago
Created attachment 8873381 [details] [diff] [review]
Ensure disabled extension badges are dimmed
Attachment #8873381 - Flags: review?(dao+bmo)

Comment 13

11 months ago
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.
Attachment #8873381 - Flags: review?(dao+bmo) → review+

Comment 14

11 months ago
(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

11 months ago
Pushed by dharvey@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/58119f6aff8f
Ensure disabled extension badges are dimmed. r=dao

Comment 16

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/58119f6aff8f
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
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

11 months ago
(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

11 months ago
(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?
Flags: needinfo?(ionut.goldan)
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.
Flags: needinfo?(ionut.goldan)
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!
You need to log in before you can comment on or make changes to this bug.