Closed Bug 1329242 Opened 7 years ago Closed 7 years ago

Ability to specify a different icon for browserActions in dark themes.

Categories

(WebExtensions :: Untriaged, enhancement)

enhancement
Not set
normal

Tracking

(firefox55 wontfix, firefox56 fixed)

RESOLVED FIXED
mozilla56
Tracking Status
firefox55 --- wontfix
firefox56 --- fixed
webextensions +

People

(Reporter: bwinton, Assigned: mattw)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [design-decision-needed], theming, triaged)

Attachments

(2 files)

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
Flags: needinfo?(mwein)
Whiteboard: [design-decision-needed], theming, triaged
(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.
Flags: needinfo?(mwein)
Flags: needinfo?(mixedpuppy)
See Also: → 1341893
See Also: 1341893
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.
webextensions: --- → +
Flags: needinfo?(mixedpuppy)
Assignee: nobody → mwein
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.
See Also: → 1365807
(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.
Attachment #8879404 - Flags: review?(kmaglione+bmo) → review?(mixedpuppy)
Attachment #8879405 - Flags: review?(kmaglione+bmo) → review?(mixedpuppy)
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
Attachment #8879404 - Flags: review?(mixedpuppy) → review+
Comment on attachment 8879405 [details]
Bug 1329242 - Add unit tests for browser_action.theme_icons

https://reviewboard.mozilla.org/r/150724/#review157854
Attachment #8879405 - Flags: review?(mixedpuppy) → review+
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
Attachment #8879404 - Flags: approval-mozilla-beta?
Comment on attachment 8879405 [details]
Bug 1329242 - Add unit tests for browser_action.theme_icons

See prior comment request.  This is the test patch.
Attachment #8879405 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/mozilla-central/rev/70384deed6a0
https://hg.mozilla.org/mozilla-central/rev/5520599fb20a
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
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.
Flags: needinfo?(jcristau)
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.
Flags: needinfo?(jcristau)
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?
Flags: needinfo?(mwein)
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.
Attachment #8879404 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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?
Flags: needinfo?(mwein) → needinfo?(jgruen)
Comment on attachment 8879405 [details]
Bug 1329242 - Add unit tests for browser_action.theme_icons

Let's also uplift the tests!
Attachment #8879405 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
Flags: needinfo?(jgruen)
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
matt: back out from central right ? does this affect uplifting ?
Flags: needinfo?(mwein)
(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?
Flags: needinfo?(mwein) → needinfo?(cam)
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.
Flags: needinfo?(cam)
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
Attachment #8879404 - Flags: approval-mozilla-beta+
Attachment #8879405 - Flags: approval-mozilla-beta+
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.
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
It doesn't look like an svg alternative is going to happen any time soon, we should document this.
Keywords: dev-doc-needed
Depends on: 1404568
Will, this landed in 56 apparently and I completely missed the ability to do light and dark in the SVG discussion.
Flags: needinfo?(wbamberg)
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.
Flags: needinfo?(wbamberg) → needinfo?(amckay)
lgtm, thanks
Flags: needinfo?(amckay)
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.
Product: Toolkit → WebExtensions
See Also: → 1558587
See Also: → 1809958
You need to log in before you can comment on or make changes to this bug.