Closed
Bug 1404568
Opened 7 years ago
Closed 7 years ago
browser_action theme_icons default incorrectly falls back to light icon, and they aren't applied properly to menu panels
Categories
(WebExtensions :: Untriaged, defect, P2)
WebExtensions
Untriaged
Tracking
(firefox57 verified, firefox58 verified)
VERIFIED
FIXED
mozilla58
People
(Reporter: mossop, Assigned: Kwan)
References
()
Details
Attachments
(3 files)
59 bytes,
text/x-review-board-request
|
mixedpuppy
:
review+
ritu
:
approval-mozilla-beta+
|
Details |
59 bytes,
text/x-review-board-request
|
mixedpuppy
:
review+
ritu
:
approval-mozilla-beta+
|
Details |
1.39 KB,
image/png
|
Details |
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•7 years ago
|
||
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•7 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•7 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•7 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+
Comment 7•7 years ago
|
||
Seems like an uplift candidate.
status-firefox57:
--- → affected
status-firefox58:
--- → affected
Updated•7 years ago
|
Priority: -- → P2
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 11•7 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•7 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)
Comment 15•7 years ago
|
||
Yeah, that's fine. Icon issues are on my radar for more attention/work.
Flags: needinfo?(mixedpuppy)
Assignee | ||
Comment 16•7 years ago
|
||
Thanks Shane! Sheriff/c-n elf: green try end of comment #14
Keywords: checkin-needed
Comment 17•7 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
![]() |
||
Comment 18•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ca81275884eb https://hg.mozilla.org/mozilla-central/rev/38347dff7799
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Assignee | ||
Comment 19•7 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•7 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•7 years ago
|
||
Is manual testing required on this bug? If yes, please provide some STR.
Flags: needinfo?(dtownsend)
Comment 23•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/99d96c2d8636 https://hg.mozilla.org/releases/mozilla-beta/rev/bbda3799f0c8
Reporter | ||
Comment 24•7 years ago
|
||
(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•7 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•7 years ago
|
||
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•7 years ago
|
Comment 27•7 years ago
|
||
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•7 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
Comment 29•7 years ago
|
||
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•7 years ago
|
||
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" > }
Updated•7 years ago
|
Comment 31•7 years ago
|
||
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•7 years ago
|
||
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•7 years ago
|
||
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•6 years ago
|
Product: Toolkit → WebExtensions
Comment 34•3 years ago
|
||
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
}
]
}
>_<
You need to log in
before you can comment on or make changes to this bug.
Description
•