Closed Bug 1836440 Opened 3 years ago Closed 2 years ago

Firefox black favicons look bad on black/dark backgrounds

Categories

(Firefox :: Theme, defect, P3)

Firefox 114
defect

Tracking

()

RESOLVED FIXED
127 Branch
Tracking Status
firefox127 --- fixed

People

(Reporter: mprops, Assigned: sp, Mentored)

Details

(Keywords: good-first-bug)

Attachments

(2 files)

Attached image darkfavicons.jpg —

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:109.0) Gecko/20100101 Firefox/114.0

Steps to reproduce:

Have a tab manager. Use the favicons from the tab api. This is how they look in a dark background.

Expected results:

Maybe they should have more neutral colors.

The Bugbug bot thinks this bug should belong to the 'Core::Widget: Gtk' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Component: Untriaged → Widget: Gtk
Product: Firefox → Core
Component: Widget: Gtk → Theme
Product: Core → Firefox

Looks like this extension should set -moz-context-properties: fill; fill: currentColor; or so? I don't think we can fix this on the theme side.

Status: UNCONFIRMED → NEW
Component: Theme → Frontend
Ever confirmed: true
Product: Firefox → WebExtensions

Where should that go? I tried adding that css to the container and to the icons themselves.
Doesn't work, even with !important
Also this is an <img> element with an svg src, not sure if that works
The inspector on the icon with -moz-context-properties says it's an invalid property name.

The svg I'm using to test is chrome://global/skin/icons/developer.svg

I suppose the extension page counts as web content, and so -moz-context-properties isn't available; the svg.context-properties.content.enabled pref controls that.

We could use prefers-color-scheme within these SVGs to fill them with white or black, respectively, but I'm not sure how that'd work if we want keep supporting context-fill at the same time, which I think we probably want to for info.svg for instance...

Moving this back to Firefox::Themes for further investigation.

Component: Frontend → Theme
Product: WebExtensions → Firefox
Severity: -- → S3
Flags: needinfo?(sfoster)
Priority: -- → P3

Here's my understanding of the issue: An extension author can use the tabs API to present a list of open tabs, and if the favIconUrl field has a value, show the favicon for that tab in its UI (as a sidebar, or whatever.) This content's color scheme is determined by the extension author, but they can use prefers-color-scheme to better match the rest of the browser UI and user's preferred light/dark color scheme. Sometimes the faviconURL is a chrome:// URL to a icon from mozilla-central. When used in browser UI, these icons get their color from browser CSS, using the -moz-context-fill mechanism. This CSS property is not enabled for web content, so the fallback color is used. The fallback color is black.

I'm not sure we should be doing something special for firefox/toolkit icons here. This problem will exist when showing any tab where the site's favicon has some fill color that is similar to the page's background color. I guess in theory we could have a fallback color in the SVG which is a grey or mid-tone which might mitigate this in some cases. But that doesn't really address the core issue which is that presenting icons you don't own which have unknown colors requires careful treatment. Perhaps an outline, or similar would work in the extension's implementation where favicons are used?

If we are seeing this problem on surfaces owned by Firefox where it is using its own icons and there's no contrast - that is definitely a bug we'll want to fix.

Maybe the best approach here is to start by identifying moz-central icons which are used as favicons. Then we can evaluate them in that context and see if a redesign is warranted. So for hypothetically we might have end up with one svg asset for the info "i" glyph used in browser UI and a separate file with the same glyph but designed for use in a favicon for error pages.

Flags: needinfo?(sfoster)

From what I understand (although I haven't tested it), we could also use prefers-color-scheme from within our SVG icons to set light and dark fill colors. However I don't think we could combine this with optionally supporting context-fill at the same time, but perhaps we could create special favicon versions of a known set of icons that would use prefers-color-scheme instead of context-fill. Does this make sense to you?

Flags: needinfo?(sfoster)

Fill allows specifying a fallback color right? Why can't we use, for example, fill: context-paint light-dark(black, white)?

Flags: needinfo?(dao+bmo)

Huh, I wasn't aware / must have forgotten about that. Seems like we may have found a solution here!

Flags: needinfo?(dao+bmo)

Next steps here are to identify moz-central icons which are used as favicons and add the light-dark(black, white) fallback color to their fill attributes.

Assignee: nobody → sfoster
Status: NEW → ASSIGNED
Flags: needinfo?(sfoster)

I'm not currently working on this but would be happy to mentor on it. Next steps are in comment 10. If you want to work on this, please prepare an initial patch and submit it to phabricator for review. If you have questions, you can need-info me here or find me (or any of the theme, desktop front-end devs) on matrix.

Assignee: sfoster → nobody
Mentor: sfoster
Status: ASSIGNED → NEW
Keywords: good-first-bug
Assignee: nobody → sp

Hi Sam, I'm a new contributor and I'll be working on a patch and submitting it ASAP.

Do you have any advice on the best way of identifying moz-central icons used as favicons in the codebase?

Flags: needinfo?(sfoster)

(In reply to sp from comment #12)

Do you have any advice on the best way of identifying moz-central icons used as favicons in the codebase?

I think a good start would be a search across the codebase for rel="icon" and find references to .svg icons in user-facing browser UI pages.

We're not interested in docs or test pages. Most of our icons are in going to be under chrome://global/skin/icons/ or chrome://browser/skin/, but it would be worth taking a look at any .svg used in this way.

Flags: needinfo?(sfoster) → needinfo?(sp)

Hi Sam, thanks for the info on matrix. It helped and I believe I have been able to make the change on the info.svg icon. Please review the WIP phabricator link below when you have time and let me know if I'm on the right track here. If so, I can continue on to the rest of the .svg's used as icons. Thanks, have a good weekend!

https://phabricator.services.mozilla.com/D208035

Flags: needinfo?(sp)

(In reply to Steve P from comment #15)

Please review the WIP phabricator link below when you have time and let me know if I'm on the right track here. If so, I can continue on to the rest of the .svg's used as icons. Thanks, have a good weekend!

https://phabricator.services.mozilla.com/D208035

So far so good. Thanks!

Attachment #9397661 - Attachment description: WIP: Bug 1836440 - Firefox black favicons look bad on black/dark backgrounds → Bug 1836440 - Firefox black favicons look bad on black/dark backgrounds.r?sfoster
Pushed by sfoster@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/72165b55f3e1 Firefox black favicons look bad on black/dark backgrounds.r=sfoster,desktop-theme-reviewers,omc-reviewers,emcminn
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 127 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: