Closed Bug 1474132 Opened 2 years ago Closed 2 years ago

Wrong icon size is chosen for about:addons and install popup

Categories

(WebExtensions :: Frontend, defect, P3)

defect

Tracking

(firefox63 verified)

VERIFIED FIXED
mozilla63
Tracking Status
firefox63 --- verified

People

(Reporter: Oriol, Assigned: Oriol)

References

Details

Attachments

(7 files, 3 obsolete files)

Attached file test.xpi (obsolete) —
Install the attached addon, it provides multiple icons with different sizes.

The browserAction icon is 16x16 pixels big and the 16 icon is chosen, that's right.

But if you go to about:addons, the icon is 32x32 pixels but the 48 one is chosen instead.

And if you go to about:debugging, the icon is 24x24 pixels but the 48 one is also chosen.

My Tab Counter Plus add-on uses a 64x64 SVG icon which looks good in 32x32 but is a bit blurry in 24x24. But if I provide an extra icon for 24, it's ignored.

What's the point of allowing to specify multiple icons if the wrong one is chosen?
Attached image screenshot.png (obsolete) —
Oriol - do you want to take this one?
Flags: needinfo?(oriol-bugzilla)
Priority: -- → P3
Yes
Assignee: nobody → oriol-bugzilla
Status: NEW → ASSIGNED
Flags: needinfo?(oriol-bugzilla)
Attached file test.xpi (obsolete) —
Better testing addon
Attachment #8990542 - Attachment is obsolete: true
Attached image screenshot.png
The install popup is also wrong
Attachment #8990543 - Attachment is obsolete: true
Very cool test extension.

I'm not certain if the icon sizes are consistent across platforms and themes, so ni? dao to see if he knows off hand.

I'm not a fan of defining the size of the icon in the function name.  I'd personally rather see something that can account for future icon size changes and platform differences.  Maybe something along the lines of:

placements = {
  "devtools": 24,
  "installPanel": 32,
  ...
}

get iconURL(placement=null) {
  let size = 48;
  if (placement) {
    let size = placements[placement];
  }
  return AddonManager.getPreferredIconURL(this, size);
}

In this case, the placements map could be different per platform, theme, etc. as well as being updated in the future for higher resolutions.

However, Andrew is much closer to the addonmanager code than I am, so I'm going to switch the r? over to him.  He may have a different opinion on this.
Flags: needinfo?(dao+bmo)
Attachment #8991887 - Flags: review?(mixedpuppy) → review?(aswan)
(In reply to Shane Caraveo (:mixedpuppy) from comment #7)
> Very cool test extension.
> 
> I'm not certain if the icon sizes are consistent across platforms and
> themes, so ni? dao to see if he knows off hand.

They are, with the caveat that DPI settings vary. E.g. for displaying an icon at 32*32 CSS pixels with double pixel density, you'd want to pick an icon with 64*64 source pixels. Not sure how web extensions currently handle this.
Flags: needinfo?(dao+bmo)
(In reply to Dão Gottwald [::dao] from comment #8)
> They are, with the caveat that DPI settings vary. E.g. for displaying an
> icon at 32*32 CSS pixels with double pixel density, you'd want to pick an
> icon with 64*64 source pixels. Not sure how web extensions currently handle
> this.

getPreferredIconURL takes devicePixelRatio into account if a window is passed:
https://searchfox.org/mozilla-central/rev/46292b1212d2d61d7b5a7df184406774727085b8/toolkit/mozapps/extensions/AddonManager.jsm#2093-2096

XPIDatabase.jsm doesn't pass a window, though.
We do a relatively complicated dance in browserAction to deal with 2x.  

https://searchfox.org/mozilla-central/rev/46292b1212d2d61d7b5a7df184406774727085b8/browser/components/extensions/parent/ext-browserAction.js#478

However, it looks like both files (where you are accessing the icon off the addon itself) already import AddonManager.  ExtensionUI has access to window, so you could use getPreferredIconURL there.  I'm not sure how devtools uses the form object, but maybe iconURL there can become a getter.
I also wonder if the css dance done for browserAction wouldn't be better everywhere.  e.g. what happens if the window is moved from a 1x window to a 2x window while an icon is visible?  I'm not certain that is a problem or worth fixing right now.
(In reply to Shane Caraveo (:mixedpuppy) from comment #10)
> I'm not sure how devtools uses the form object, but maybe iconURL there can become a getter.

The about:debugging page asks for the details of all the addons to the Remote Debugging Server, which sends the info returned by the `form` getter defined here:

- https://searchfox.org/mozilla-central/rev/46292b1212d2d61d7b5a7df184406774727085b8/devtools/server/actors/addon/webextension.js#81

On the debugging server side, we can't assume anything about which is the window from which the request is coming from, and so it doesn't seem right to use getPreferredIconURL from there.

Currently, the about:debugging page is only used to debug locally installed addons, and so we could probably workaround it on the client side by using the addon id to get direct access to the addon object using the AddonManager and then use getPreferredIconURL with the current window.

Nevertheless, we also need to take into account that there are plans to make about:debugging also able to debug remote targets (which is currently covered by the webide) and so any change in this direction should also be double-checked from that point of view (I think that jdescottes may be a great pick to double-check the strategy described above, get more information about the plans for the remote targets support in about:debugging, and the impact that this kind of change may have).
Comment on attachment 8991887 [details]
Bug 1474132 - Choose right add-on icon size in about:addons and install popup

https://reviewboard.mozilla.org/r/256804/#review263766

Please don't add more properties to AddonWrapper, getPreferredIconURL() should work here.
As for devtools, it sounds like the client should either pass in the requested size or it should fetch all the icons and choose an appropriate one on the client side.  You may want to split the about:debugging portion into a separate bug or patch.
Attachment #8991887 - Flags: review?(aswan) → review-
(In reply to Andrew Swan [:aswan] (on PTO until 7/23) from comment #13)
> You may want to split the about:debugging portion
> into a separate bug or patch.

Sounds good. You are now on PTO so switched to Luca.
Summary: Wrong icon size is chosen for about:addons and about:debugging → Wrong icon size is chosen for about:addons and install popup
See Also: → 1475703
Attached file test.xpi
Fix typos in case you want to test with layout.css.devPixelsPerPx=2
Attachment #8991873 - Attachment is obsolete: true
Comment on attachment 8991887 [details]
Bug 1474132 - Choose right add-on icon size in about:addons and install popup

https://reviewboard.mozilla.org/r/256804/#review264728

Looks good, thanks Oriol!

I was in doubt about the need to take into account that getPreferredIconURL may raise an exception if any of the icons' key is not an integer (1), but I verified and it is cached by the validation of the manifest based on the schema (2) and actually caught here: https://searchfox.org/mozilla-central/rev/c296d5b2391c8b37374b118180b64cca66c0aa16/toolkit/components/extensions/Extension.jsm#577-581
and so the extension is considered corrupted and the install doorhanger is not actually reached.

1: https://searchfox.org/mozilla-central/rev/c296d5b2391c8b37374b118180b64cca66c0aa16/toolkit/mozapps/extensions/AddonManager.jsm#2121
2: https://searchfox.org/mozilla-central/rev/c296d5b2391c8b37374b118180b64cca66c0aa16/toolkit/components/extensions/schemas/manifest.json#94-100
Attachment #8991887 - Flags: review?(lgreco) → review+
Keywords: checkin-needed
This screenshot shows how it looks right now (without the fix) and layout.css.devPixelsPerPx=2.

The icons selected for the install doorhanger and in the about:addons page looks pretty bad (the one in the install doorhanger looks particularly wrong)
This screenshot shows how it looks with the attached fix applied and layout.css.devPixelsPerPx=2.
Pushed by ebalazs@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e75b08486dc6
Choose right add-on icon size in about:addons and install popup r=rpl
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e75b08486dc6
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Attached file 1474132.zip
I tried the extension from the Description and noticed that:

Win 7 64-bit: sidebar 16, about:debugging 48, about:addons 32, browserAction 16, pop-up 32.
Mac OS X 10.13.3: sidebar 32, about:debugging 48, about:addons 64, browserAction 32, pop-up 64.
Ubuntu 14.04 32-bit: sidebar 16, about:debugging 48, about:addons 32, browserAction 16, pop-up 32.

Are these the desired changes?
Flags: needinfo?(oriol-bugzilla)
The result looks good to me at first glance, you probably have some DPI scaling in Mac.
But please test the updated https://bugzilla.mozilla.org/attachment.cgi?id=8992085 add-on, the icons should have green background when using 100% or 200% DPI scaling (for other values it will probably be red because I didn't include icons for arbitrary sizes).
I didn't fix the about:debugging icon, that one should still be red.
Flags: needinfo?(oriol-bugzilla)
Attached file Bug1474132-part2.zip
Thanks for the testing extension!

iMac Sierra 10.12.6: sidebar 16, about:debugging 48, about:addons 32, browserAction 16, pop-up 32

MacBookPro (Retina, 13-inch) Sierra 10.13.3: sidebar 32, about:debugging 48, about:addons 64, browserAction 32, pop-up 64.

Ubuntu 14.04 32-bit: sidebar 16, about:debugging 48, about:addons 32, browserAction 16, pop-up 32.

Win 7 64-bit: sidebar 16, about:debugging 48, about:addons 32, browserAction 16, pop-up 32.

Firefox 63.0a1(20180726001822).

This issue is verified as fixed.
Status: RESOLVED → VERIFIED
Doesn't theme_icons need to be fixed as well ?
Flags: needinfo?(oriol-bugzilla)
Can you include an example?
Flags: needinfo?(oriol-bugzilla) → needinfo?(ntim.bugs)
(In reply to Oriol Brufau [:Oriol] from comment #26)
> Can you include an example?

See https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/manifest.json/browser_action
Flags: needinfo?(ntim.bugs)
They seem to work well
See Also: → 1499693
Depends on: 1499693
You need to log in before you can comment on or make changes to this bug.