1094357 - Dark favicons hard to read on Themes with a dark tabstrip background (like the default theme, or dark lightweight themes)

Status: NEW

(Firefox :: Theme, defect, P5)

Description

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Attachment: favicons-dark-theme.png

See attachment

Comment 1

[Brian Grinstead [:bgrins]]

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.

Comment 2

[Justin Dolske [:Dolske]]

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?

Comment 3

[Justin Dolske [:Dolske]]

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.

Comment 4

[Justin Dolske [:Dolske]]

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.

Comment 5

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Demo of outer glow effect

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.

Comment 8

[Dão Gottwald [:dao]]

(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.

Comment 9

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

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 10

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Bug 1094357 - Implement underglow effect on dark favicons to make them visible on Photon style tabs.

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

Comment 11

[(inactive) Nihanth Subramanya [:nhnt11]]

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

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

Comment 12

[(inactive) Nihanth Subramanya [:nhnt11]]

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.

Comment 13

[(inactive) Nihanth Subramanya [:nhnt11]]

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

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

Comment 14

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: glow opacity comparison

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 15

[(inactive) Nihanth Subramanya [:nhnt11]]

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 16

[Blake Winton (:bwinton) (:☕️)]

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…

Comment 17

[Justin Dolske [:Dolske]]

(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 18

[:Gijs (out for now; he/him)]

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 19

[Tim Nguyen :ntim]

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))
}

Comment 20

[(inactive) Nihanth Subramanya [:nhnt11]]

(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.

Comment 21

[(inactive) Nihanth Subramanya [:nhnt11]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162782/diff/3-4/

Comment 22

[(inactive) Nihanth Subramanya [:nhnt11]]

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.

Comment 27

[Dale Harvey (:daleharvey)]

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

Comment 28

[Blake Winton (:bwinton) (:☕️)]

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

Comment 29

[(inactive) Nihanth Subramanya [:nhnt11]]

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.

Comment 31

[TMart]

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

Comment 32

[danielwatson311]

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.

Comment 33

[danielwatson311]

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

Comment 34

[mco]

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.

Comment 35

[benhaines]

+1

Comment 36

[benhaines]

+1

Comment 37

[mozilla]

My hope 🤞is that now that Chrome also has support for SVG favicons (like FF had for years), people who make websites will use SVG favicons, and ideally embed CSS with a dark-mode version. https://dark-mode-favicon.glitch.me/

There's still a bit of a problem with FF's default theme, where the tab bar is dark, but the content is light. This is a very ignorant thought, but perhaps the tab bar could report its prefers-color-scheme independently from the content? I have no concept of the implementation details.

Relying on sites to start using SVG is less of a "Bang! All sites look good now!" fix, but if interested parties can evangelize sites they use (and improve the sites they control), maybe it will catch on.