See attachment

Since these colors are now used in the browser chrome and needing to render favicons as well as devtools extensions within the toolbox, it may be time to revisit the actual colors used for this selection. See also bug 1094574 about using a slightly less dark base color.

Stephen: do we want to pull this into Photon? Your mocks for Windows/macOS with the dark titlebar show a white squircle behind the dark Github favicon. Did you have a particular approach in mind for determining when how/when to show that?

Talked with Stephen... We'd like to consider this for Photon, but it's not high-priority given that DevTools theme has shipped with this issue for years. But sounds like the fix should be fairly straightforward -- compute the dominant/average/whatever color of the favicon, and if it's below a given contrast threshold, put the white background behind it. (The white background should be slightly larger than 16x16, so that fully-opaque icons that are low contrast will still have some of it extend beyond the edges.) Rumor has it we have/had some of this code floating around for use on Android or Metro, but I don't know where it is.

A bit more discussion: * We don't know what the perf overhead of color detection is, so should be mindful of that. * Nihanth suggested that a clever approach (maybe for v2?) would be to apply an outer-glow type effect to the image. So small icons with lots of transparency (e.g. http://sched.org/) wouldn't be forced to be afloat in a sea of white, but would have a hopefully-pleasing white border/glow to improve contrast.

For future reference, this is a simple demo of an outer glow effect created by stacking the image on top of itself, and inverting and blurring the bottom image. It uses the sched favicon on a background value of #232323.

(In reply to Justin Dolske [:Dolske] from comment #3) > Talked with Stephen... > > We'd like to consider this for Photon, but it's not high-priority given that > DevTools theme has shipped with this issue for years. ... as well as Firefox on Ubuntu without the dark compact/devtools theme.

Some thought following a conversation with bwinton on Slack: - Blurring the actual shape is great, since it leads to fewer problems with oddly-shaped favicons! - Favicons are generally designed for a bright/neutral background, so we shouldn't apply a dark glow when using a light theme - This could potentially look very odd when done on all icons. I imagine there must be a way to find out the contrast ratio of icon to background and only apply it when that falls under a certain threshold?

Comment on attachment 8891715 [details] Bug 1094357 - Implement underglow effect on dark favicons to make them visible on Photon style tabs. So, UX asked if I was interested in implementing this for 57 so I started giving it a shot as a Sunday morning side project. The patch adds another identical image positioned below the favicon, and applies a filter to grayscale, invert, brighten and blur it. It's not quite ready for review yet: - It does not attempt to detect whether the favicon is dark yet (i.e. it actually needs this effect). - I've only tested this on the default theme - The positioning rules could probably be more elegant than a negative margin + z-index; we probably want to use a xul:stack for in the final patch. Requesting feedback for now to gather opinions on the approach. - I wonder if this has perf implications (we are, after all, going to be doing image processing) and/or will regress talos tests. - I already have xul:stack in mind, but is there a cleverer way to do the stacking/positioning? - Any thoughts on the amount of blur or other filter parameters? The main thing I want to improve here is to find a deterministic way to completely whiten the image instead of grayscale+invert+brightness. - Ideally, instead of having two xul:images, I'd like to just use ::before to have a white square under the image, and clip this square using the icon above it as a mask, and blur the resulting shape. I couldn't figure out how to do this with CSS though.

New patch uses contrast(0) to completely black out the image instead of grayscale, and adds an opacity. Blake, the attached screenshot compares glow opacities of 100%, 75%, and 50% from top to bottom. Can you chime in on what you think is the best value?

Comment on attachment 8891715 [details] Bug 1094357 - Implement underglow effect on dark favicons to make them visible on Photon style tabs. See comment 12

Comment on attachment 8891716 [details] glow opacity comparison I'm a fan of the top one, but let's check with Philipp and Stephen to see what they prefer…

(Random thought: it would probably be a good idea to do a Try/Talos run with this, to see if it has impact on performance. I'd expect CSS filters to have some modest cost, and Talos seems to be quite sensitive now. Best to find that out up front, instead of after we've polished the visuals in review!)

Comment on attachment 8891715 [details] Bug 1094357 - Implement underglow effect on dark favicons to make them visible on Photon style tabs. https://reviewboard.mozilla.org/r/162782/#review168232 Concur with dolske on the perf issues here. ::: browser/themes/shared/tabs.inc.css:93 (Diff revision 3) > +#TabsToolbar:not([brighttext]) { > + .tab-icon-image-underglow { > + visibility: hidden; > + } > +} This, um, isn't valid CSS. But also, just set it to visibility: hidden in the simple selector rule you add below this, and use the descendant selector with [brighttext] to set it to visible.

Comment on attachment 8891715 [details] Bug 1094357 - Implement underglow effect on dark favicons to make them visible on Photon style tabs. https://reviewboard.mozilla.org/r/162782/#review168780 ::: browser/base/content/tabbrowser.xml:7380 (Diff revision 3) > <xul:image xbl:inherits="fadein,pinned,busy,progress,selected=visuallyselected" > class="tab-throbber" > role="presentation" > layer="true" /> > <xul:image xbl:inherits="src=image,loadingprincipal=iconLoadingPrincipal,fadein,pinned,selected=visuallyselected,busy,crashed,sharing" > + anonid="tab-icon-image-underglow" This extra <xul:image> is unnecessary. You can achieve the same effect using the drop-shadow() filter. .tabbrowser-tab:not([visuallyselected=true]):not(:-moz-lwtheme) .tab-icon-image { filter: drop-shadow(0 0 2px rgba(255,255,255,0.3)) }

(In reply to Tim Nguyen :ntim from comment #19) > Comment on attachment 8891715 [details] > Bug 1094357 - Implement underglow effect on dark favicons to make them > visible on Photon style tabs. > > https://reviewboard.mozilla.org/r/162782/#review168780 > > ::: browser/base/content/tabbrowser.xml:7380 > (Diff revision 3) > > <xul:image xbl:inherits="fadein,pinned,busy,progress,selected=visuallyselected" > > class="tab-throbber" > > role="presentation" > > layer="true" /> > > <xul:image xbl:inherits="src=image,loadingprincipal=iconLoadingPrincipal,fadein,pinned,selected=visuallyselected,busy,crashed,sharing" > > + anonid="tab-icon-image-underglow" > > This extra <xul:image> is unnecessary. > > You can achieve the same effect using the drop-shadow() filter. > > > .tabbrowser-tab:not([visuallyselected=true]):not(:-moz-lwtheme) > .tab-icon-image { > filter: drop-shadow(0 0 2px rgba(255,255,255,0.3)) > } Nice, thanks! I didn't run into the drop-shadow filter when I was Googling.

So this patch adds the outer glow using the drop-shadow() filter, to all icons with the darkicon attribute (which will be inherited from the tab binding). Now we just have to set this attribute somewhere after detecting if the icon is "dark". I'll ask around and find out who to talk to about the best way to implement such a thing, and then we can do perf experiments.

Nihanth is this something you were thinking about coming back to, or want me to see about getting it landed?

(I suspect Nihanth is still waiting on a ui-review from :shorlander or :phlsa…)

Yeah, I'd like to work on this but a final "yes we should do this" would be nice since this is perf-impacting work. I need to jog my memory on the context of this bug, I'll ping people this week and get back up to speed.

As the Dark Theme is nearly finished now it would be really nice to have this. Is there any way to contribute to this?

61 is in beta now and it'd be real nice to see this land along with the other Dark Theme improvements. Though this does affect both "Default" and "Dark" so you might say it's an even more important improvement.

https://github.com/nt1m/vivaldi-fox does something similar, only it merely puts a white favicon behind.

alternative 1: perhaps icons with a very low detected contrast could just be inverted? alternative 2: perhaps just a solid lighter gray container behind the favicons (squircle, circle, outline...)? alternative 3: if the contrast detection is expensive from a performance perspective, perhaps just make it an option on right-click of the favicon, for the user to choose to invert the favicon of that site (or add a background, or whatever the solution is) just to avoid the glow, which still doesn't seems to increase the contrast that much, and also aesthetically doesn't match the rest of the flat ui style (in my humble opinion). additionally, some favicons glowing and others not-glowing would look inconsistent and seem to highlight some tabs over others. particularly when in 'pinned' form. just some feedback, i hope this is the right place for it.