As suggested in https://github.com/bwinton/SnoozeTabs/issues/67, but we also ran into this in TabCenter. Fortunately, TabCenter was an SDK-based add-on so it could go digging in the chrome to try and figure out what's up, but that's not available to SnoozeTabs (because it's a WebExtension). Possible options: 1) Add an extra something to the manifest specifying alternate icons. Pros: Very easy for add-on authors to support. Cons: Impossible for add-ons to change other bits of their UI to match the dark theme. 2) Add a new API for themes, with a way to get info on the current theme (id?, name, textColor, accentColor, author, iconURL, whether it's light or dark?) and an event when the theme changes. Pros: Add-ons can make their UI fit in with the theme. We could extend the API to allow switching the theme. Cons: Supporting a dark theme now requires a background script and event listener. 3) Something else?

will check out with Theming if add-ons can determine how to match icon to theme or generic if dark theme use this icon. similar to what we have separate add-ons for dev-tools theme because it's dark

(In reply to Blake Winton (:bwinton) (:☕️) from comment #0) > > 3) Something else? Maybe bug 1207597 would help.

(In reply to Hector Zhao [:hectorz] from comment #2) > (In reply to Blake Winton (:bwinton) (:☕️) from comment #0) > > > > 3) Something else? > > Maybe bug 1207597 would help. That would be part of it, but it wouldn't let stuff like TabCenter darken its UI when the rest of the browser's theme was dark, so it would only get us part of the way there.

> 1) Add an extra something to the manifest specifying alternate icons. > Pros: Very easy for add-on authors to support. > Cons: Impossible for add-ons to change other bits of their UI to match > the dark theme. I think the ":-moz-lwtheme-brighttext" and ":-moz-lwtheme-darktext" psuedoclasses could help us accomplish this for lightweight themes (mozLWThemeBrightText http://searchfox.org/mozilla-central/source/layout/style/nsCSSRuleProcessor.cpp#2042). For example, calling element.matches(":-moz-lwtheme-brighttext") will return true if a bright theme is active. On the other hand, :-moz-lwtheme-darktext can be used for detecting dark themes. So if the author provides alternative icons we could switch between them this way. > 3) Something else? Going off of what I mentioned in 1) -> rather than having the extension author provide additional icons, we could apply our own filters to the icons when light or dark themes are active. In this case, I imagine we'd have a property in the manifest that allows extension authors to opt in to this, with it being off by default.

To be discussed at 2/21 WebExtensions triage meeting. Agenda: https://docs.google.com/document/d/1H4sjnRFc87NZXZsM6XIgwWaoV2bdRFepiG28G7WzgPs/edit

If/when context menus become theme-able, this issue should apply to those icons as well. Although... as mentioned by Jared Wein[1], some platforms (like Ubuntu) might already be changing the background colour of context menus. 1: https://mail.mozilla.org/pipermail/dev-addons/2017-February/002609.html

I would add a "apply_theme_button_tint" manifest field that defaults to false, which allows you to apply the button tint specified by the theming API. Since compact themes will also become WE themes, I suspect it will naturally work.

It would be nice if SVG icons with fill="context-fill" could match the relevant fill.

Calling out that Firefox Screenshots (shipping in 55) would benefit greatly from this: https://github.com/mozilla-services/screenshots/issues/2887

I don't think I'll be able to get this landed in 55, but I'll aim to get it landed early in 56 Nightly.

I would like this to accommodate not just icons, but also general styling: I've been pushed from Tab Center to eoger's Tab Center Redux, and I've realized there's no way to make the UI reflect my dark theme, even by eliminating all CSS color declarations.

(In reply to Tim Nguyen :ntim from comment #11) > It would be nice if SVG icons with fill="context-fill" could match the > relevant fill. We can easily make this apply to all .toolbarbutton-1 rather than only :-moz-any(@primaryToolbarButtons@): http://searchfox.org/mozilla-central/rev/c49a70b53f67dd5550eec8a08793805f2aca8d42/browser/themes/shared/toolbarbutton-icons.inc.css#7-14

I am running into this and tried context-fill before finding this bug. On top of just matching the default color I also need a highlight color though, like the blue color that the download button uses and am not sure how context-fill can solve this.

(In reply to Dão Gottwald [::dao] from comment #15) > (In reply to Tim Nguyen :ntim from comment #11) > > It would be nice if SVG icons with fill="context-fill" could match the > > relevant fill. > > We can easily make this apply to all .toolbarbutton-1 rather than only > :-moz-any(@primaryToolbarButtons@): > > http://searchfox.org/mozilla-central/rev/ > c49a70b53f67dd5550eec8a08793805f2aca8d42/browser/themes/shared/toolbarbutton- > icons.inc.css#7-14 I think we should do this. Using context-fill is clearly opt-in, and it would be nice to be able to integrate with the native styles.

(In reply to :Harald Kirschner :digitarald from comment #16) > I am running into this and tried context-fill before finding this bug. On > top of just matching the default color I also need a highlight color though, > like the blue color that the download button uses and am not sure how > context-fill can solve this. I don't think our SVG icons are designed in this manner, but we can supply a 'fill' color and a 'stroke' color using context-fill. So we could use 'fill' for the default color, and 'stroke' for the highlight color.

Comment on attachment 8879404 [details] Bug 1329242 - Add support for browser_action.theme_icons https://reviewboard.mozilla.org/r/150722/#review157848 ::: toolkit/components/extensions/ExtensionParent.jsm:1158 (Diff revision 2) > } > } > + > + if (themeIcons) { > + themeIcons.forEach(({size, light, dark}) => { > + if (!INTEGER.test(size)) { unecessary now

Comment on attachment 8879405 [details] Bug 1329242 - Add unit tests for browser_action.theme_icons https://reviewboard.mozilla.org/r/150724/#review157854

Pushed by mwein@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/70384deed6a0 Add support for browser_action.theme_icons r=mixedpuppy https://hg.mozilla.org/integration/autoland/rev/5520599fb20a Add unit tests for browser_action.theme_icons r=mixedpuppy

Comment on attachment 8879404 [details] Bug 1329242 - Add support for browser_action.theme_icons just landed, early uplift request lest we forget Approval Request Comment [Feature/Bug causing the regression]: needed for screenshots for dark/light theme buttons [User impact if declined]: poor appearance with themes [Is this code covered by automated tests?]: yes [Has the fix been verified in Nightly?]: in autoland, should hit m-c soon [Needs manual test from QE? If yes, steps to reproduce]: no [List of other uplifts needed for the feature/fix]: both patches in this bug [Is the change risky?]: low to moderate [Why is the change risky/not risky?]: changes are fairly small [String changes made/needed]: none

Comment on attachment 8879405 [details] Bug 1329242 - Add unit tests for browser_action.theme_icons See prior comment request. This is the test patch.

https://hg.mozilla.org/mozilla-central/rev/70384deed6a0 https://hg.mozilla.org/mozilla-central/rev/5520599fb20a

Is the plan for screenshots to start using this in the 55 time frame?

hey :jcristau, I talked to mwien about this patch for a bit. We use the browserAction.setIcon so the solution here does not cover our use cases. I would love to be able to pass the default theme color into my webExtension's icons (which are SVGs). Seems like Dao's suggestion https://bugzilla.mozilla.org/show_bug.cgi?id=1329242#c15 would allow this. I'm not at all knowledgeable about how these SVGs would inherit browser styles, but would setting a class inside a webExtension SVG toolbar icon inherit from a css declaration in toolbarbutton-icons.inc.css? Julien, NIing you since you asked the original question, but feel free to flag someone else for info here.

I talked to mwein last week in SF and he was positive in allowing comment 15 to happen, so I filed bug 1377302 for it.

Removing that ni, https://bugzilla.mozilla.org/show_bug.cgi?id=1377302 is taking care of the case i describe.

Looks like we could uplift the patches from both bugs (for beta 9 later this week). Do they need to land at the same time?

Comment on attachment 8879404 [details] Bug 1329242 - Add support for browser_action.theme_icons Theme fix for screenshot feature, let's uplift for beta 9.

Lets make sure we need this uplifted, it seems like bug 1377302 is all that screenshot needs. Via irc mattw thinks this one is not necessary. jgruen, can you verify you don't need this one uplifted?

Comment on attachment 8879405 [details] Bug 1329242 - Add unit tests for browser_action.theme_icons Let's also uplift the tests!

I didn't realize this was still in doubt. I'll find you on irc to straighten this out.

No, this issue is not required for screenshots since we are using the context-fill option :mixedpuppy refers to.

I think we should consider backing out this change now that support for SVG icons has been added by bug 1377302. I think the SVG solution is simple and elegant, and I'd rather we hold off on supporting other image formats until we see a use case for which the use of SVG icons doesn't make sense.

matt: back out from central right ? does this affect uplifting ?

(In reply to Matthew Wein [:mattw] from comment #41) > I think we should consider backing out this change now that support for SVG > icons has been added by bug 1377302. I think the SVG solution is simple and > elegant, and I'd rather we hold off on supporting other image formats until > we see a use case for which the use of SVG icons doesn't make sense. The SVG solution is only enabled for mozilla signed extensions.

Though I do admit I don't like the solution found in this bug.

Do you know what it would take to get the SVG solution enabled for all WebExtensions, not just for those internal to Mozilla?

I think Jonathan's concerns are not technical -- we could easily allow this for all WebExtensions -- rather that this feature should go through the usual standardization process before being exposed to content. It might be that WebExtensions are sufficiently not-content that it's OK to allow them, but still we would be creating a potential legacy burden for ourselves here if we eventually did standardize this feature but differently, and we have random WebExtensions relying on the old syntax.

Comment on attachment 8879404 [details] Bug 1329242 - Add support for browser_action.theme_icons removing beta approval flag until the future of this change is decided

Too late for 55. Mark 55 won't fix.

I'm resolving this bug again to reflect reality. The patch landed and has not been backed out. Obviously, simply reopening a bug isn't an effective way to get it backed out. Advocate for backing it out, actually do get it backed out, then we'll reopen this.

It doesn't look like an svg alternative is going to happen any time soon, we should document this.

Will, this landed in 56 apparently and I completely missed the ability to do light and dark in the SVG discussion.

I've updated https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/browser_action to talk about this. Let me know if this covers it.

lgtm, thanks

The problem with this solution is that it only applies to the initial icon. It doesn't work for webextensions that change their toolbar icon (e.g. if they're using the icon as a toggle, or to show the status of something). For that reason, I think the SVG method from bug 1377302 is a better approach.

Is there a programmatic way to know the current needed text ? Light/dark ?

Since I recently tried dealing with switching a light/dark browserAction icon programmatically, I thought this might be an interesting FYI: https://github.com/mdn/webextensions-examples/blob/master/theme-switcher/switcher.js While you can't detect literally light vs dark, you *can* query for the currently active theme extension using the browser.management API and then make a judgement as to whether that theme should be considered light or dark https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/management/getAll

Another FYI is bug 1419940: you can do a setIcon({}) or setIcon({ path: null }) to revert the icon to what is described in the manifest.

As a web extension developer: Detecting the toolbar background color requires a lot of logic (parsing various color formats, named colors) and is prone to error (background images) so this is not dev friendly. It would be useful to have a browser.browserSettings.isDarkTheme or a "toolbar_is_dark" returned by `browser.theme.getCurrent()` so we can set the right icon programmatically using browser.pageAction.setIcon.

See bug 1367042 and bug 1416871.