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

VERIFIED FIXED in Firefox 63

Status

defect
P3
normal
VERIFIED FIXED
11 months ago
7 months ago

People

(Reporter: Oriol, Assigned: Oriol)

Tracking

unspecified
mozilla63
Dependency tree / graph

Firefox Tracking Flags

(firefox63 verified)

Details

Attachments

(7 attachments, 3 obsolete attachments)

Assignee

Description

11 months ago
Posted 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?
Assignee

Comment 1

11 months ago
Posted image screenshot.png (obsolete) —
Oriol - do you want to take this one?
Flags: needinfo?(oriol-bugzilla)
Priority: -- → P3
Assignee

Comment 3

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

Comment 4

11 months ago
Posted file test.xpi (obsolete) —
Better testing addon
Attachment #8990542 - Attachment is obsolete: true
Assignee

Comment 5

11 months ago
Posted image 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

11 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

11 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

11 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

11 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

11 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

11 months ago
See Also: → 1475703
Assignee

Comment 16

11 months ago
Posted file test.xpi
Fix typos in case you want to test with layout.css.devPixelsPerPx=2
Attachment #8991873 - Attachment is obsolete: true

Comment 17

10 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

10 months ago
Keywords: checkin-needed

Comment 18

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

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

Comment 20

10 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

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

Comment 22

10 months ago
Posted 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)
Assignee

Comment 23

10 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

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

10 months ago
Status: RESOLVED → VERIFIED

Comment 25

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

Comment 26

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

Comment 27

9 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

9 months ago
They seem to work well
Assignee

Updated

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