browser_action theme_icons default incorrectly falls back to light icon, and they aren't applied properly to menu panels

VERIFIED FIXED in Firefox 57

Status

defect
P2
normal
VERIFIED FIXED
2 years ago
Last year

People

(Reporter: mossop, Assigned: Kwan)

Tracking

unspecified
mozilla58
Dependency tree / graph

Firefox Tracking Flags

(firefox57 verified, firefox58 verified)

Details

()

Attachments

(3 attachments)

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
Reporter

Updated

2 years ago
Blocks: 1329242
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.
Assignee

Comment 2

2 years ago
(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.
Assignee: nobody → moz-ian
Status: NEW → ASSIGNED
Summary: browserAction theme_icons are inverted between dark and light → browser_action theme_icons default incorrectly falls back to light icon, and they aren't applied properly to menu panels
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 5

2 years ago
mozreview-review
Comment on attachment 8914981 [details]
Bug 1404568 - Improve webext browser_action icon fallbacks.

https://reviewboard.mozilla.org/r/186238/#review191566
Attachment #8914981 - Flags: review?(mixedpuppy) → review+

Comment 6

2 years ago
mozreview-review
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
Attachment #8914982 - Flags: review?(mixedpuppy) → review+
Seems like an uplift candidate.

Updated

2 years ago
Priority: -- → P2
Can this land?
Flags: needinfo?(moz-ian)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 11

2 years ago
(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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 14

2 years ago
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
Flags: needinfo?(moz-ian) → needinfo?(mixedpuppy)
Yeah, that's fine.  Icon issues are on my radar for more attention/work.
Flags: needinfo?(mixedpuppy)
Assignee

Comment 16

2 years ago
Thanks Shane!

Sheriff/c-n elf: green try end of comment #14
Keywords: checkin-needed

Comment 17

2 years ago
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ca81275884eb
https://hg.mozilla.org/mozilla-central/rev/38347dff7799
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Assignee

Comment 19

2 years ago
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
Attachment #8914981 - Flags: approval-mozilla-beta?
Assignee

Comment 20

2 years ago
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
Attachment #8914982 - Flags: approval-mozilla-beta?
Comment on attachment 8914981 [details]
Bug 1404568 - Improve webext browser_action icon fallbacks.

Recent regression, beta57+
Attachment #8914981 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8914982 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 22

2 years ago
Is manual testing required on this bug? If yes, please provide some STR.
Flags: needinfo?(dtownsend)
(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.
Flags: needinfo?(dtownsend)

Comment 25

2 years ago
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.

Comment 26

2 years ago
Posted image dark1.PNG
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.

Updated

2 years ago
Status: RESOLVED → VERIFIED
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 ?
Assignee

Comment 28

2 years ago
(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.

Updated

Last year
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.