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

Status: VERIFIED FIXED

(WebExtensions :: Untriaged, defect, P2)

Description

[Dave Townsend [:mossop]]

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

Comment 1

[Martin Giger [:freaktechnik]]

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.

Comment 2

[Ian Moody [:Kwan] (UTC+1)]

(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 3

[Ian Moody [:Kwan] (UTC+1)]

Attachment: Bug 1404568 - Improve webext browser_action icon fallbacks.

Currently if there is no default icon at the specified size, the default icon
falls back to the light text icon at that size. This is wrong in two ways:
First, the default theme uses dark text, so it should fallback to the dark icon
Secondly, authors expect the unsized default_icon to be used if specified

This patch fixes both of these issues, so that the default icon first falls back
to the unsized default_icon, and then only if that is not specified falls back
to the dark icon

Review commit: https://reviewboard.mozilla.org/r/186238/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/186238/

Comment 4

[Ian Moody [:Kwan] (UTC+1)]

Attachment: Bug 1404568 - Use the correct browser_action theme icons when the action is in a menu-panel.

The patch adding support for specifying theme icons had a bug in the CSS: it
added styles for the action in a menu-panel depending on theme, but missed out
the theme pseudo-class selectors.  Therefore the dark text icon was always used
since it was last in the CSS.
Additionally, the menu panels can't be styled, so still have light backgrounds
and dark text even in light text themes. If a light icon is used in the menu
panel in a light text theme it will be hard to see.

Thus, this patch adds the pseudo-class for dark text themes, but removes the
selector entirely for light text themes.

Review commit: https://reviewboard.mozilla.org/r/186240/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/186240/

Comment 5

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8914981 [details]
Bug 1404568 - Improve webext browser_action icon fallbacks.

https://reviewboard.mozilla.org/r/186238/#review191566

Comment 6

[Shane Caraveo (:mixedpuppy)]

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

Comment 7

[Shane Caraveo (:mixedpuppy)]

Seems like an uplift candidate.

Comment 8

[Dave Townsend [:mossop]]

Can this land?

Comment 9

[Ian Moody [:Kwan] (UTC+1)]

Comment on attachment 8914981 [details]
Bug 1404568 - Improve webext browser_action icon fallbacks.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/186238/diff/1-2/

Comment 10

[Ian Moody [:Kwan] (UTC+1)]

Comment on attachment 8914982 [details]
Bug 1404568 - Use the correct browser_action theme icons when the action is in a menu-panel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/186240/diff/1-2/

Comment 11

[Ian Moody [:Kwan] (UTC+1)]

(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 12

[Ian Moody [:Kwan] (UTC+1)]

Comment on attachment 8914981 [details]
Bug 1404568 - Improve webext browser_action icon fallbacks.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/186238/diff/2-3/

Comment 13

[Ian Moody [:Kwan] (UTC+1)]

Comment on attachment 8914982 [details]
Bug 1404568 - Use the correct browser_action theme icons when the action is in a menu-panel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/186240/diff/2-3/

Comment 14

[Ian Moody [:Kwan] (UTC+1)]

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

Comment 15

[Shane Caraveo (:mixedpuppy)]

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

Comment 16

[Ian Moody [:Kwan] (UTC+1)]

Thanks Shane!

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

Comment 17

[Pulsebot]

Pushed by [e-mail]:
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

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 19

[Ian Moody [:Kwan] (UTC+1)]

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 20

[Ian Moody [:Kwan] (UTC+1)]

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 21

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8914981 [details]
Bug 1404568 - Improve webext browser_action icon fallbacks.

Recent regression, beta57+

Comment 22

[marius.santa]

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

Comment 23

[Ryan VanderMeulen [:RyanVM]]

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

Comment 24

[Dave Townsend [:mossop]]

(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.

Comment 25

[Andy McKay]

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

[marius.santa]

Attachment: 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.

Comment 27

[Julien Wajsberg [:julienw]]

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 ?

Comment 28

[Ian Moody [:Kwan] (UTC+1)]

(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

Comment 29

[Julien Wajsberg [:julienw]]

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

Comment 30

[Rémy Hubscher (:natim)]

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"
>  }

Comment 31

[Rémy Hubscher (:natim)]

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

Comment 32

[Rémy Hubscher (:natim)]

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

Comment 33

[Julien Wajsberg [:julienw]]

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.

Comment 34

[Val Packett]

It's really silly that SVG theme icons can be unused on low-DPI screens because of that size thing… especially when non-theme icons don't even have a smaller listed size i.e.

  "icons": {
    "96": "icon.svg",
    "48": "icon.svg"
  },
  "browser_action": {
    "theme_icons": [
      {
        "dark": "icon.svg",
        "light": "icon-inv.svg",
        "size": 48
      }
    ]
  }

>_<