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

VERIFIED FIXED in Firefox 63

Status

P3
normal
VERIFIED FIXED
8 months ago
4 months ago

People

(Reporter: Oriol, Assigned: Oriol)

Tracking

unspecified
mozilla63

Firefox Tracking Flags

(firefox63 verified)

Details

Attachments

(7 attachments, 3 obsolete attachments)

(Assignee)

Description

8 months ago
Created attachment 8990542 [details]
test.xpi

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?
(Assignee)

Comment 1

8 months ago
Created attachment 8990543 [details]
screenshot.png
Oriol - do you want to take this one?
Flags: needinfo?(oriol-bugzilla)
Priority: -- → P3
(Assignee)

Comment 3

7 months ago
Yes
Assignee: nobody → oriol-bugzilla
Status: NEW → ASSIGNED
Flags: needinfo?(oriol-bugzilla)
(Assignee)

Comment 4

7 months ago
Created attachment 8991873 [details]
test.xpi

Better testing addon
Attachment #8990542 - Attachment is obsolete: true
(Assignee)

Comment 5

7 months ago
Created attachment 8991880 [details]
screenshot.png

The install popup is also wrong
Attachment #8990543 - Attachment is obsolete: true
Comment hidden (mozreview-request)
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)
(Assignee)

Comment 9

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

Comment 12

7 months ago
(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 13

7 months ago
mozreview-review
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-
Comment hidden (mozreview-request)
(Assignee)

Comment 15

7 months ago
(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.
(Assignee)

Updated

7 months ago
Summary: Wrong icon size is chosen for about:addons and about:debugging → Wrong icon size is chosen for about:addons and install popup
(Assignee)

Updated

7 months ago
See Also: → bug 1475703
(Assignee)

Comment 16

7 months ago
Created attachment 8992085 [details]
test.xpi

Fix typos in case you want to test with layout.css.devPixelsPerPx=2
Attachment #8991873 - Attachment is obsolete: true

Comment 17

7 months ago
mozreview-review
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+
(Assignee)

Updated

7 months ago
Keywords: checkin-needed

Comment 18

7 months ago
Created attachment 8992974 [details]
icon-size-selected-without-the-fix.png

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)

Comment 19

7 months ago
Created attachment 8992976 [details]
icon-size-selected-with-the-fix.png

This screenshot shows how it looks with the attached fix applied and layout.css.devPixelsPerPx=2.

Comment 20

7 months ago
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

Comment 21

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e75b08486dc6
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63

Comment 22

7 months ago
Created attachment 8994843 [details]
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)
(Assignee)

Comment 23

7 months ago
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)

Comment 24

7 months ago
Created attachment 8995071 [details]
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.

Updated

7 months ago
Status: RESOLVED → VERIFIED
status-firefox63: fixed → verified

Comment 25

6 months ago
Doesn't theme_icons need to be fixed as well ?
Flags: needinfo?(oriol-bugzilla)
(Assignee)

Comment 26

6 months ago
Can you include an example?
Flags: needinfo?(oriol-bugzilla) → needinfo?(ntim.bugs)

Comment 27

6 months ago
(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)
(Assignee)

Comment 28

6 months ago
They seem to work well
(Assignee)

Updated

4 months ago
See Also: → bug 1499693
Depends on: 1499693
You need to log in before you can comment on or make changes to this bug.