1925701 - Sidebar extension icons that are svg's don't have consistent fill color based on the selected theme

Status: NEW

(Firefox :: Sidebar, defect, P3)

Description

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Attachment: Screenshot 2024-10-18 at 4.09.42 PM.png

Add-on icons in the vertical tabs sidebar seem to have the wrong fill color applied in latest nightly on Mac. Version: 133.0a1 (2024-10-18) (aarch64)

Steps to reproduce:

1. Set sidebar.revamp and sidebar.verticalTabs prefs to true
2. Install some addons (I'm using side-view and notes--these are pretty old and maybe that has something to do with it?)
3. Activate the alpenglow built-in theme

Expected: the icons in the sidebar all have the theme-based fill color

Actual: all the icons have the theme-based fill except the addon icons, which have the regular grey fill. See screenshot.

Comment 1

[Sarah Clements [:sclements]]

hrm, this is really strange because inspecting any of these svg's for the extensions I can see they have the should have the same color/fill applied by moz-button styles as other icons in the launcher (bookmarks, history, etc). I installed TST, Sideberry and Side View and only Side View was unaffected by this. Sideberry even kept its light grey with dark theme.

I think I need someone from the extensions team to speak to whether there is some code that lets extension authors override icon color regardless of theme, which is not great in this case since it looks inconsistent.

Comment 2

[William Durand [:willdurand]]

I am not entirely clear on what's being asked here, sorry if I am off in the reply below.

Extensions can specify any icon, and not necessarily SVGs per https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/manifest.json/sidebar_action (like we have icons for extensions in the nav-bar and the rest of the UI). You can try with https://addons.mozilla.org/en-US/firefox/addon/whatsapp-in-sidebar/, which will add a PNG icon to the sidebar.

That said, I think the intent is to override the colors of SVG files, and the fact that it works for Side View and not Sidebery or another sidebar-friendly extension that uses an SVG icon makes me think about: -moz-context-properties and therefore the svg.context-properties.content.enabled pref. Side View is a special Mozilla-owned line extension. It has some more capabilities than other regular extensions like Sidebery. I think it's this: https://searchfox.org/mozilla-central/rev/dfc3050503739883ce6d17e8365bc35410106aba/toolkit/components/extensions/Extension.sys.mjs#3791-3799

Comment 3

[Sarah Clements [:sclements]]

(In reply to William Durand [:willdurand] from comment #2)

I am not entirely clear on what's being asked here, sorry if I am off in the reply below.

Extensions can specify any icon, and not necessarily SVGs per https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/manifest.json/sidebar_action (like we have icons for extensions in the nav-bar and the rest of the UI). You can try with https://addons.mozilla.org/en-US/firefox/addon/whatsapp-in-sidebar/, which will add a PNG icon to the sidebar.

That said, I think the intent is to override the colors of SVG files, and the fact that it works for Side View and not Sidebery or another sidebar-friendly extension that uses an SVG icon makes me think about: -moz-context-properties and therefore the svg.context-properties.content.enabled pref. Side View is a special Mozilla-owned line extension. It has some more capabilities than other regular extensions like Sidebery. I think it's this: https://searchfox.org/mozilla-central/rev/dfc3050503739883ce6d17e8365bc35410106aba/toolkit/components/extensions/Extension.sys.mjs#3791-3799

Thanks Will, this does help clarify my question around icons supplied by extension authors. Can you elaborate more on the -moz-context-properties enabling svgs with theme-based fill color - is this something that can be applied to all sidebar extensions without needing to enable the pref?

Comment 4

[William Durand [:willdurand]]

(In reply to Sarah Clements [:sclements] from comment #3)

is this something that can be applied to all sidebar extensions without needing to enable the pref?

No, this is only for Mozilla extensions (see: Bug 1710917).

Comment 5

[Sarah Clements [:sclements]]

(In reply to William Durand [:willdurand] from comment #4)

(In reply to Sarah Clements [:sclements] from comment #3)

is this something that can be applied to all sidebar extensions without needing to enable the pref?

No, this is only for Mozilla extensions (see: Bug 1710917).

You mean the context-fill only applies by default to Mozilla extensions, but other sidebar extensions like Sidebery requires flipping the pref for that to apply? I don't understand why that's behind a pref though if it has to do with an extension having certain priviledges. It'd be nice for me to give the UX team more context on this rather than "nope, won't fix" as this will likely be reported as a bug again.

Comment 6

[William Durand [:willdurand]]

No other extension "requires" flipping the pref because we shouldn't expect this feature/pref to be ever set when designing Firefox UI. This is a non-standard feature that only Mozilla-owned extensions can leverage, and other extensions cannot use that SVG feature. Why? I don't know. See https://bugzilla.mozilla.org/show_bug.cgi?id=1394579#c18 and https://bugzilla.mozilla.org/show_bug.cgi?id=1388907.

It'd be nice for me to give the UX team more context on this rather than "nope, won't fix" as this will likely be reported as a bug again.

The sidebar is a CUI area like the nav-bar, tabstrip, bookmarks, etc. Even if we were to allow this feature for all extensions with SVG icons, what would we do for PNG icons?

Comment 7

[Sarah Clements [:sclements]]

Attachment: Screenshot 2024-10-23 at 10.33.39 am.png

Comment 8

[Sarah Clements [:sclements]]

(In reply to William Durand [:willdurand] from comment #6)

No other extension "requires" flipping the pref because we shouldn't expect this feature/pref to be ever set when designing Firefox UI. This is a non-standard feature that only Mozilla-owned extensions can leverage, and other extensions cannot use that SVG feature. Why? I don't know. See https://bugzilla.mozilla.org/show_bug.cgi?id=1394579#c18 and https://bugzilla.mozilla.org/show_bug.cgi?id=1388907.

I might be misunderstanding you so bear with me... only Side View is mozilla owned right? That's what I see from looking at a few sidebar extensions in about:addons. But when I flip that pref, the context-fill seems to be applied to any svg with a moz-extension:// source - so you'll see from this screenshot in comment 7 that we now have context-fill applied to most of the sidebar extensions except for TST which seems to have an SVG/xml src and whatsapp which is a png.

Since :rpl worked on some of those bugs, perhaps he has a bit more context on that work that was done and why there is a pref? Or is there someone else who might know?

The sidebar is a CUI area like the nav-bar, tabstrip, bookmarks, etc. Even if we were to allow this feature for all extensions with SVG icons, what would we do for PNG icons?

It's not customizable in the same way (as in we haven't extended the CUI palette to include the sidebar if that's what you mean). And yeah, fair question about PNG icons. I saw in that bug you linked that a bit further down there was a proposal to recommend svg's as the icon for this (and many requests for context-fill of extension icons) but it sounds like that's not the case currently. Is it worth considering any changes to this (if we can) as we think about further sidebar extension support once the revamped sidebar is released to all users?

Comment 9

[Sarah Clements [:sclements]]

Attachment: Screenshot 2024-10-23 at 11.00.52 am.png

Dark theme with svg.context-properties.content.enabled = false. You'll notice the last extension icon is barely perceptible.

Comment 10

[Sarah Clements [:sclements]]

Attachment: Screenshot 2024-10-23 at 11.01.14 am.png

Dark theme with svg.context-properties.content.enabled = true

Comment 11

[Emilio Cobos Álvarez (:emilio)]

The dark theme issue is fixable without context properties, in the svg, by using the color-scheme property or meta tag

Comment 12

[Sarah Clements [:sclements]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #11)

The dark theme issue is fixable without context properties, in the svg, by using the color-scheme property or meta tag

If I add color-scheme: light dark to the img element it doesn't seem to work (for that one extension icon that does not have the correct fill).

Comment 13

[Emilio Cobos Álvarez (:emilio)]

You need to add :root { color-scheme: light dark } to the SVG.

Comment 14

[Sarah Clements [:sclements]]

Will and I talked about this on slack and he gave me some more context. It sounds like there was a request years ago to allow this svg context-fill feature enabled for (regular - non-Mozilla-owned) extension icons, and the answer was "no". Seems like perhaps somewhat documented in this bug and this bug.

Since this was many years ago, it'd be great to find out if this decision can be revisited and if not for it to be clearly documented here (and for me to pass on to my PM).

:jwatt and :dholbert, could either of you help give some insight on this past decision and if it could be revisited?

Comment 15

[Daniel Holbert [:dholbert]]

(In reply to Sarah Clements [:sclements] from comment #14)

Will and I talked about this on slack and he gave me some more context. It sounds like there was a request years ago to allow this svg context-fill feature enabled for (regular - non-Mozilla-owned) extension icons, and the answer was "no"

Yup, see e.g. https://bugzilla.mozilla.org/show_bug.cgi?id=1388193 and maybe/probably other bugs where this was proposed & wontfix'ed.

Since this was many years ago, it'd be great to find out if this decision can be revisited and if not for it to be clearly documented here (and for me to pass on to my PM).

The decision still stands; this is a non-standard feature that we're using as a hackaround until we have cycles to implement the standardized replacement (in bug 1812163, on our radar but not a high-priority at the moment). Once we've got the standardized replacement implemented, we'd like to rapidly switch over all usages of context-fill/context-paint in our frontend icons to use the new thing, and then remove support for the current hacky context-fill/context-paint solution. If there are arbitrary 3rd-party extensions using & depending on this feature, then we could ask them nicely to remove/modernize their usages but likely many of them wouldn't, and hence they may break if we removed support, and we'll be doomed to supporting this as a legacy feature forever.

We do have a way to opt-in extensions to be able to use this, though, if they're under our control (so we're confident that we'll be able to rip out the context-fill/context-paint usage in the future). See https://searchfox.org/mozilla-central/rev/abe6e3de6850eca664e968484f8ecfc53487ea42/layout/svg/SVGContextPaint.cpp#78-86

It looks like that should already Just Work for any extension that we recognize as being "mozilla-owned" per https://searchfox.org/mozilla-central/rev/abe6e3de6850eca664e968484f8ecfc53487ea42/toolkit/components/extensions/Extension.sys.mjs#296-305 . So the most direct fix here is to be sure your Sidebar extension falls into that category, I think.

Comment 16

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #15)

It looks like that should already Just Work for any extension that we recognize as being "mozilla-owned" per https://searchfox.org/mozilla-central/rev/abe6e3de6850eca664e968484f8ecfc53487ea42/toolkit/components/extensions/Extension.sys.mjs#296-305 . So the most direct fix here is to be sure your Sidebar extension falls into that category, I think.

Ah, sorry, I didn't read the backscroll here -- if this something we're wanting to get working for 3rd-party extensions that plug into the sidebar, then it's not as simple the mechanism that I linked to; and I think it may be gated on implementing the standardized thing in bug 1812163.