I can't seem to get theme_icons to work right and I think it's because of a bug in the feature. When I set a default icon and the light icon to the same black icon and the dark icon to a white icon what I actually see is: default theme: black icon light theme: white icon dark theme: dark icon I think there is a bug in the test for this feature, wouldn't you want the dark icon with the light theme and vice versa? https://dxr.mozilla.org/mozilla-central/source/browser/components/extensions/test/browser/browser_ext_browserAction_theme_icons.js#164

Especially since it defaults to the "light" icon if you don't have a matching default icon and the default Firefox theme would need a dark icon. Else the it would be more of a terminology issue (is the property name the theme or the icon type). See https://dxr.mozilla.org/mozilla-central/rev/11fe0a2895aab26c57bcfe61b3041d7837e954cd/toolkit/components/extensions/ExtensionParent.jsm#1315 for the default icon logic.

(In reply to Dave Townsend [:mossop] from comment #0) > I can't seem to get theme_icons to work right and I think it's because of a > bug in the feature. > > When I set a default icon and the light icon to the same black icon and the > dark icon to a white icon what I actually see is: > > default theme: black icon > light theme: white icon > dark theme: dark icon > > I think there is a bug in the test for this feature, wouldn't you want the > dark icon with the light theme and vice versa? > > https://dxr.mozilla.org/mozilla-central/source/browser/components/extensions/ > test/browser/browser_ext_browserAction_theme_icons.js#164 So there is a bit of confusion as the naming isn't perhaps ideal, but the manifest properties are based on the _text colour_, as documented in the manifest schema. https://hg.mozilla.org/mozilla-central/file/c97190c389c4/toolkit/components/extensions/schemas/manifest.json#l347 so light: icon.png is what will show in Fx's dark theme, since the themes are classed by text colour. But there is a bug: (In reply to Martin Giger [:freaktechnik] from comment #1) > Especially since it defaults to the "light" icon if you don't have a > matching default icon and the default Firefox theme would need a dark icon. > Else the it would be more of a terminology issue (is the property name the > theme or the icon type). > > See > https://dxr.mozilla.org/mozilla-central/rev/ > 11fe0a2895aab26c57bcfe61b3041d7837e954cd/toolkit/components/extensions/ > ExtensionParent.jsm#1315 for the default icon logic. That should definitely be darkURL instead of lightURL as the default theme is dark text. Though I actually think it should try a normal default icon before the darkURL, that seems more intuitive for authors. There is also a second bug with menu panels, where the CSS selectors are all missing the pseudo-classes: https://hg.mozilla.org/mozilla-central/file/c97190c389c4/browser/base/content/browser.css#l368 so the dark icon is always used. And the panels are always dark text even in light text themes, so the light icon should never be used in it. Patches for these two issues incoming.

Comment on attachment 8914981 [details] Bug 1404568 - Improve webext browser_action icon fallbacks. https://reviewboard.mozilla.org/r/186238/#review191566

Comment on attachment 8914982 [details] Bug 1404568 - Use the correct browser_action theme icons when the action is in a menu-panel. https://reviewboard.mozilla.org/r/186240/#review191568

Seems like an uplift candidate.

Can this land?

(In reply to Dave Townsend [:mossop] from comment #8) > Can this land? Sorry for the delay. So I've updated the test for icons to account for the new behaviour, but I was delayed from uploading it because I've been a bit puzzled by try run results. https://treeherder.mozilla.org/#/jobs?repo=try&revision=182e5586223604441c46cec51c506aea794fd0d1&selectedJob=135176109 It keeps hitting bug 1204281, which I don't see how this could have made it go from intermittent from permanent, but it makes me nervous nonetheless.

Hey Shane, just want you to go over test changes in https://reviewboard.mozilla.org/r/186238/diff/1-3/ quickly. The first change is just adjusting the test to match the corrected behaviour for default using the dark icon. The second change, to 16, makes it also test the new using provided default for all sizes behaviour. (I think we could maybe do with a new bug to change the icon selecting logic, to select for type before size) (In reply to Ian Moody [:Kwan] from comment #11) > (In reply to Dave Townsend [:mossop] from comment #8) > > Can this land? > > Sorry for the delay. So I've updated the test for icons to account for the > new behaviour, but I was delayed from uploading it because I've been a bit > puzzled by try run results. > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=182e5586223604441c46cec51c506aea794fd0d1&selectedJob=1 > 35176109 > It keeps hitting bug 1204281, which I don't see how this could have made it > go from intermittent from permanent, but it makes me nervous nonetheless. So that seems like it might have been an artefact of doing a test-paths push, I think it prevents chunking so the suite simply ended up with too many tests in it and timed out, a non-test-path push did fine: https://treeherder.mozilla.org/#/jobs?repo=try&revision=54958174765af3e8ebe1222bccaadb7e705d3d1f

Yeah, that's fine. Icon issues are on my radar for more attention/work.

Thanks Shane! Sheriff/c-n elf: green try end of comment #14

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/ca81275884eb Improve webext browser_action icon fallbacks. r=mixedpuppy https://hg.mozilla.org/integration/autoland/rev/38347dff7799 Use the correct browser_action theme icons when the action is in a menu-panel. r=mixedpuppy

https://hg.mozilla.org/mozilla-central/rev/ca81275884eb https://hg.mozilla.org/mozilla-central/rev/38347dff7799

Comment on attachment 8914981 [details] Bug 1404568 - Improve webext browser_action icon fallbacks. Approval Request Comment [Feature/Bug causing the regression]: bug 1329242, bug in initial implementation [User impact if declined]: Extension's toolbar icon meant for light-text themes shown in default theme, hard to see (also confuses add-on developers) [Is this code covered by automated tests?]: Yes [Has the fix been verified in Nightly?]: Yes, verified in Firefox 58.0a1 (20171010100200) just now [Needs manual test from QE? If yes, steps to reproduce]: No [List of other uplifts needed for the feature/fix]: none [Is the change risky?]: no [Why is the change risky/not risky?]: very minimal code change. Previously used one of two local variables as a fallback, now pick the other one; also add another simple fallback earlier on. [String changes made/needed]: none

Comment on attachment 8914982 [details] Bug 1404568 - Use the correct browser_action theme icons when the action is in a menu-panel. Approval Request Comment [Feature/Bug causing the regression]: bug 1329242, bug in initial implementation [User impact if declined]: Extension's action icon is wrong if the button is placed in the overflow panel [Is this code covered by automated tests?]: no [Has the fix been verified in Nightly?]: Yes, verified in Firefox 58.0a1 (20171010100200) just now [Needs manual test from QE? If yes, steps to reproduce]: No [List of other uplifts needed for the feature/fix]: none [Is the change risky?]: no [Why is the change risky/not risky?]: very minimal CSS change. Removing two selectors that never had any affect, and making two more specific so they only apply when they should. [String changes made/needed]: none

Comment on attachment 8914981 [details] Bug 1404568 - Improve webext browser_action icon fallbacks. Recent regression, beta57+

Is manual testing required on this bug? If yes, please provide some STR.

https://hg.mozilla.org/releases/mozilla-beta/rev/99d96c2d8636 https://hg.mozilla.org/releases/mozilla-beta/rev/bbda3799f0c8

(In reply to marius.santa from comment #22) > Is manual testing required on this bug? If yes, please provide some STR. You could create an example add-on that uses the manifest entry I guess. Not sure though maybe the patch author has opinions.

Marius, I created an example add-on here that tests it out: https://github.com/mdn/webextensions-examples/pull/294 https://github.com/mdn/webextensions-examples/tree/master/beastify You can see the comment from Will about how it used to behave, seems to work much better for me now.

This bug is verified on Firefox 57.0b8 (20171013042429) and Firefox 58.0a1 (20171015220106) under Windows 10 64bit/Mac OS X 10.12.3. Please see the attached screenshot.

I think this issue still happens when we use different sizes for both icons. As an extension developer, it's not clear what the behaviour is, especially when we don't specify verious sizes for default_icon. See https://github.com/mdn/webextensions-examples/compare/master...julienw:icons-different-size as a testcase. Should I file a sepaerate bug ?

(In reply to Julien Wajsberg [:julienw] from comment #27) > I think this issue still happens when we use different sizes for both icons. > As an extension developer, it's not clear what the behaviour is, especially > when we don't specify verious sizes for default_icon. > > See > https://github.com/mdn/webextensions-examples/compare/master...julienw:icons- > different-size as a testcase. Should I file a sepaerate bug ? So that sounds like a different issue. This bug was for inappropriately using the light icon instead of the default icon. I'm guessing in your case it is not using the theme icons at all? That'll be what I alluded to in comment #14. The code checks by size first, then type. And it checks for ideal size, then ideal size*2.[1] So when you had 32 it was looking for 16, not finding it and trying 16*2=32 and succeeding, but now with 48 that fails and falls back to default (because default has an assumed size of 19 [2]). (I think that's what's happening anyway) (I suppose it would have been better had the manifest structure been "theme_icons": { "dark": ("path"|{"16": "path", "32": "path"}), "light": ("path"|{"16": "path", "32": "path"}) } etc. so it matched the default_icon structure) [1] https://hg.mozilla.org/mozilla-central/file/5572465c08a9/toolkit/components/extensions/ExtensionParent.jsm#l1363 [2] https://hg.mozilla.org/mozilla-central/file/5572465c08a9/toolkit/components/extensions/ExtensionParent.jsm#l1289

Yes that's what I guessed too :) Is it worth filing a separate bug, I mean, would we want to change this behavior?

We encountered the exact same bug and the solution we found was to explicitely have two icon size (16px for most desktop and 32 for desktop running on retina displays) I updated the documentation to explicit it: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/browser_action#Syntax I believe that we should fallback to a bigger icon if present (looking for 16px should use the 32px form) I believe that we should be able to use SVG here. The same way that we can use: > "default_icon": "path/to/geo.svg" We should be able to use: > "theme_icons": { > "light": "icons/geo-light.svg", > "dark": "icons/geo-dark.svg" > }

Also here [0] we advertise for 48px and 96px icons while in the theme_icons doc we speak about 16px and 32px sized icons [1] [0] https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/icons [1] https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/browser_action#Syntax

I am adding a bit of the discussion we had with Luca on IRC: > It also interesting that there is a galore of sizes in the internal icon selection logic :-) > > eg. the AddonsManager specify 64 as the default preferred size: https://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIInstall.jsm#1669,1674 > > browserAction defaults to 16 with 18 as a fallback: https://searchfox.org/mozilla-central/source/browser/components/extensions/ext-browserAction.js#480-481,487-488 > > pageAction uses 32 and 16 as preferred icon sizes on desktop: https://searchfox.org/mozilla-central/source/browser/components/extensions/ext-pageAction.js#159,164-165 > > but on mobile it is 18 * devicePixelRatio: https://searchfox.org/mozilla-central/source/mobile/android/components/extensions/ext-pageAction.js#171-172

I don't think I see this issue with this code: https://github.com/devtools-html/Gecko-Profiler-Addon/blob/ef046a4ce85bc1375ec656502eccc1fb61bf996f/manifest.json#L17-L23 "default_icon": "icons/toolbar_off.png", "theme_icons": [ { "dark": "icons/toolbar_off.png", "light": "icons/toolbar_off_light.png", "size": 32 } So I never specify a 16x16 icon. Yet the icon file itself is 64x64. I'm not entirely sure of what this means :) but this works fine for us so far.