1376518 - svg webextensions icon not consistently showing

Status: RESOLVED WORKSFORME

(WebExtensions :: Frontend, defect, P5)

Description

[jmros]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:54.0) Gecko/20100101 Firefox/54.0
Build ID: 20170608105825

Steps to reproduce:

1) Installed my toy web extension link rel prev (https://addons.mozilla.org/en-US/firefox/addon/link-rel-prev/?src=search)
2) open about:debugging
3) open about:addons

Seen on windows 10 64 bit with the following Firefox versions:
Nightly: 56.0a1 (2017-06-27) (64-bits)
Developer: 55.0b3 (64-bit)
Stable: 54.0 (64-bits)

According to aswan on irc it works in both an old 54 dev edition and the current nightly on mac.


Actual results:

1) installation succeeds (GOOD)
2) icon shows (GOOD)
3) icon does not show (BAD)


Expected results:

The same icon that shows on about:debugging should also show on about:addons

Comment 1

[Tomislav Jovanovic :zombie]

(In reply to jmros from comment #0)
> Seen on windows 10 64 bit with the following Firefox versions:
> Nightly: 56.0a1 (2017-06-27) (64-bits)
> Developer: 55.0b3 (64-bit)
> Stable: 54.0 (64-bits)

Is that a list of versions where the icon is visible, or where it is not?

Comment 2

[jmros]

(In reply to Tomislav Jovanovic :zombie from comment #1)
> (In reply to jmros from comment #0)
> > Seen on windows 10 64 bit with the following Firefox versions:
> > Nightly: 56.0a1 (2017-06-27) (64-bits)
> > Developer: 55.0b3 (64-bit)
> > Stable: 54.0 (64-bits)
> 
> Is that a list of versions where the icon is visible, or where it is not?

Sorry for the confusion.

The icon of this particular web extension is NOT shown in about:addons on all listed versions (all the versions I tested on):
- Nightly: 56.0a1 (2017-06-27) (64-bits)
- Developer: 55.0b3 (64-bit)
- Stable: 54.0 (64-bits)

Instead, the default add-on icon (green puzzle) is shown.

Comment 3

[jmros]

As another datapoint, the link rel prev 'back' icon does show correctly in the about:addons tab on ubuntu: Mozilla firefox for Ubuntu canonical - 1.0 54.0 (64-bits).

Comment 4

[jmros]

Today, I managed to debug a bit.

I found out that I have an odd scale factor for my display (1.07) and that plays a major role.

What seems to happen is, that during installation not only the xpi is used as source for icons, but also <icon size=64"/> and <icon size="32"> from an AMO query. My extension only has a 48 icon (just like "Your first extension" https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Your_first_WebExtension).

The icons shown during install use only the icon set from the xpi and display my icon.
But about:addons uses XPIProvider.get icons() and that combines the icons from _repositoryAddon (only when installed from AMO: loading a directory or local xpi via about:debugging has no repository information) and the addon itself and thus has an icon set of {32: null, 48: "back.svg", 64: null}.

And now my strange scale factor interferes: getPreferredIconURL gets called for size 48, but due to my scale factor that becomes 51.4... and the first larger value is 64, so a null icon is returned and the page shows the default extension icon.

So I think the gist of the problem is, that empty icon definitions from AMO are sometimes used as icon for a web extension.

Steps to validate (numeric steps are for reproduction, alphabetical for debugging):
0) ensure your scaling factor is not 1!
1) start Firefox (I tested with Nightly) with a new profile
a) open the debugger tools
b) open the options of the debugger tools
c) set debugging of chrome and broser elements to on
d) set remote debugging to on)
e) close debugger tools
f) open the browser debugger tools
2) open about:addons
3) go to the first tab page, something like "get addons"
4) scroll down to "get more addons" (not this bug, but who thought it was a good idea to hide this at the bottom, after pages full of addons that are irrelevant to me or even already installed?)
5) search for link rel prev
6) choose link rel prev from the results
g) set breakpoints at resource://modules/addons/AddonRepository.jsm line 1218, resource://modules/addons/XPIProvider.jsm line 5271 and resource://modules/AddonManager.jsm line 2384
7) install the extension
h) see that the first breakpoint is hit and the node is <icon size="64"/> (so some xml describing an icon with size 64 but no pointer to where to find it)
i) see that the first breakpoint is hit again and the node is <icon size="32"/>
8) in the extension add prompt (that shows the extension icon correctly from a set containing only the one element from the manifest), choose add
9) in the confirmation prompt (that shows the extension icon correctly from a set containing only the one element from the manifest), choose OK
10) switch back to the about:addons tab
11) in that tab, switch back to the extensions tab page
j) notice that the second breakpoint is hit twice, once for size 32 and once for 64
12) PROBLEM: notice that the link rel prev extension has the default icon

Comment 5

[Blake Winton (:bwinton) (:☕️)]

Maybe AMO could pick the "best" icon with the same code that the internal stuff uses?
https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/AddonManager.jsm#2314-2385

Comment 6

[Blake Winton (:bwinton) (:☕️)]

(Oh, we should _also_ fix the code in AddonRepository line 1218 to not store empty icon lines in the icons object… ;)

Comment 7

[jmros]

Attachment: Patch 1 - ignore icons from AMO that are not set

When installing an extension from the add-on repository, the extension
metadata that is registered in the repository gets parsed. For icons,
it turned out that the nodes <icon size="64"/> and <icon size="32"/> are present, even when no icons have been provided to the repository. Ignore those icon sizes without a URL.

Comment 8

[jmros]

Attachment: Patch 0 - Ignore visual studio workspace directory

Visual Studio 2017 supports opening a directory as solution, and then creates a .vs directory for storing user settings and probably IntelliSense databases.
Ignore that .vs directory.

Comment 9

[jmros]

Attachment: Patch 1 - ignore icons from AMO that are not set

When installing an extension from the add-on repository, the extension
metadata that is registered in the repository gets parsed. For icons,
it turned out that the nodes <icon size="64"/> and <icon size="32"/> are present, even when no icons have been provided to the repository. Ignore those icon sizes without a URL.

Comment 10

[jmros]

Attachment: Patch 2 - only use the repository-provided icons when the extension does not provide icons itself through it's manifest

The repository-provided icons are always png, but a web extension can provide icons in svg which might scale better. And due to that, a web extension might provide only one svg (with an arbitrary size specified, but let's assume 48 as that is what the documentation suggests as a size you must provide).
When the repository-provided pngs get added to that, the weird situation might arise that the total iconset is {32: a.png, 48: b.svg, 64: c.png} meaning a.png will be used for sizes 16 an lower (luckily only 16 is used regularly) and c.png will be used for sizes 49 and up. But chances are b.svg is actually better than c.png for all sizes except possibly 64.
Therefor, ignore the repository-provided icons as soon as an addon provides an iconset.
Because legacy addons might not provide their own icon in the wellknown location it does make sense to use the repository icons as base and augment that data with the wellknown locations.

Comment 11

[jmros]

(In reply to jmros from comment #7)
> Created attachment 8893253 [details] [diff] [review]
> Patch 1 - ignore icons from AMO that are not set
> 
> When installing an extension from the add-on repository, the extension
> metadata that is registered in the repository gets parsed. For icons,
> it turned out that the nodes <icon size="64"/> and <icon size="32"/> are
> present, even when no icons have been provided to the repository. Ignore
> those icon sizes without a URL.

I reuploaded this patch because I got an error from bzexport saying I could not assign the bug to myself.

Comment 12

[jmros]

(In reply to jmros from comment #9)
> Created attachment 8893255 [details] [diff] [review]
> Patch 1 - ignore icons from AMO that are not set
> 
> When installing an extension from the add-on repository, the extension
> metadata that is registered in the repository gets parsed. For icons,
> it turned out that the nodes <icon size="64"/> and <icon size="32"/> are
> present, even when no icons have been provided to the repository. Ignore
> those icon sizes without a URL.

This is the patch that bwinton asks for in comment $6.

Comment 13

[jmros]

I could use some advise how to go from here.

1) What automated tests (if any) are needed to test those patches, where should they be placed and which level + framework should be used.

2) I think merging the icon selection code is a good idea (if only because it reduces the chance that the code acts differently), but this is my first dive into the mozilla sourcecode and I feel not comfortable yet to refactor code across files.

Also, because this is a spare time project, I do not know in advance how much time I can spend. Last week was a good week, but I expect that next week I will not be able to do much.

Comment 14

[jmros]

(In reply to jmros from comment #9)
> Created attachment 8893255 [details] [diff] [review]
> Patch 1 - ignore icons from AMO that are not set
> 
> When installing an extension from the add-on repository, the extension
> metadata that is registered in the repository gets parsed. For icons,
> it turned out that the nodes <icon size="64"/> and <icon size="32"/> are
> present, even when no icons have been provided to the repository. Ignore
> those icon sizes without a URL.

O, I see eslint gives two errors on this change:

$ mach lint -l eslint -o
c:\mozilla-source\mozilla-central\toolkit\mozapps\extensions\internal\AddonRepository.jsm
  1001:14  error  Method '_parseAddon' has a complexity of 61.  complexity (eslint)
  1219:11  error  Expected space(s) after "if".                 keyword-spacing (eslint)

? 2 problems (2 errors, 0 warnings)

The second one is easy to fix: just add a space.
But how could I resolve the first? Apparently I just tipped the scale by adding an extra if.

Comment 15

[jmros]

Attachment: Patch 1 - ignore icons from AMO that are not set

When installing an extension from the add-on repository, the extension
metadata that is registered in the repository gets parsed. For icons,
it turned out that the nodes <icon size="64"/> and <icon size="32"/> are present, even when no icons have been provided to the repository. Ignore those icon sizes without a URL.

Comment 16

[Blake Winton (:bwinton) (:☕️)]

Hey jmros, I assigned the bug to you, because Mozilla trusts me with that sort of power.  ;)

For the complexity, you're right, the problem is adding the extra "if".  To fix it, is there any way you could simplify the method, or split it up a little?  Maybe move all the empty-element-handling into a more generic separate function or something?

The last thing to do is to request review from people for your patches.  I'll look in the hg logs, and see who might be a good choice, but since it's your first set of patches, I'm going to let you do the requesting, so that you get a quicker response.  :)

For patch 0, "gps" or "ted" seem good.
For patch 1, let's try "kmag".
And for patch 2, either "rhelmer" or "kmag" (again).  :)

Thanks for the work!

Comment 17

[jmros]

Comment on attachment 8893254 [details] [diff] [review]
Patch 0 - Ignore visual studio workspace directory

I've seen Mercurial correctly ignoring the .vs folder Visual Studio creates when opening a directory as directory project locally, but .gitignore is untested.

Comment 18

[jmros]

Comment on attachment 8893276 [details] [diff] [review]
Patch 1 - ignore icons from AMO that are not set

This is my first patch.
Known problem: the method is getting too complex (61).
And mach test says no tests are registered for this method.

But I expect to not have any time to work on this next week and could use some advice on both how to reduce the complexity (splitting the addon type translation? or splitting the basi handling from the custom handling?) and which tests to run and add.
Therefor I as for a preliminary review.

Comment 19

[Kris Maglione [:kmag]]

In general, we want the repository data to override the packaged extension data so that it can be updated on AMO without shipping a complete add-on update. These patches reverse that behavior, and I'm not entirely sure that's desirable.

Mossop, Andrew, Jorge, do you have any opinions on this?

Comment 20

[Dave Townsend [:mossop]]

(In reply to Kris Maglione [:kmag] from comment #19)
> In general, we want the repository data to override the packaged extension
> data so that it can be updated on AMO without shipping a complete add-on
> update. These patches reverse that behavior, and I'm not entirely sure
> that's desirable.

That's certainly what it's always done and I think is the correct behaviour.

Comment 21

[Jorge Villalobos [:jorgev] (he/him)]

(In reply to Kris Maglione [:kmag] from comment #19)
> In general, we want the repository data to override the packaged extension
> data so that it can be updated on AMO without shipping a complete add-on
> update. These patches reverse that behavior, and I'm not entirely sure
> that's desirable.

Agree, we should stick to the current behavior. We have talked about being more consistent on AMO and to not allow overriding certain metadata, but at the moment the correct behavior is to favor the metadata on AMO.

Comment 22

[jmros]

(In reply to Jorge Villalobos [:jorgev] from comment #21)
> (In reply to Kris Maglione [:kmag] from comment #19)
> > In general, we want the repository data to override the packaged extension
> > data so that it can be updated on AMO without shipping a complete add-on
> > update. These patches reverse that behavior, and I'm not entirely sure
> > that's desirable.
> 
> Agree, we should stick to the current behavior. We have talked about being
> more consistent on AMO and to not allow overriding certain metadata, but at
> the moment the correct behavior is to favor the metadata on AMO.

Fair enough, I'll withdraw patch 2.

What I tried to fix is the situation that an extension provides a scalable icon (svg, registered as size 48 since that is the recommended size to register, no way to register a scalable icon exists and it feels like a waste of time to register the icon multiple times for multipli sizes that just happen to be needed for one reason or another) in its package, but it gets overruled by non-scalable icons from AMO (it only supports png, size 32 and 64 at least) and then when an icon smaller than 32 or larger than 48 is requested, a png gets scaled while a scalable svg is available.

But that might better be fixed in the Web extension spec by allowing any, or a range, for size and prefering those icons when an exact match is not found before scaling an imperfect match.

However, the code before my change appears to prefer the icon data from package and only fall back to the AMO one when a certain size is not provided in the package but is on AMO. And that is not what you describe as the correct behaviour at the moment.
The current 'incorrect' behaviour is great in my situation as I can at least refer to my svg in the extension package for all sizes I register a png icon in AMO for.

Comment 23

[jmros]

Could you please consider patch 1, which ignores empty metadata for icons from AMO? Although it could be said that empty metadata is still metadata and therefor important, I never uploaded an icon and AMO delivers a 32 and 64 icon key out of nothing. So the solution could also be that AMO itself does not deliver metadata that was not provided.

Comment 24

[Andy McKay]

Mark, weren't you looking at something similar in bug 1388705?

Comment 25

[jmros]

(In reply to Andy McKay [:andym] from comment #24)
> Mark, weren't you looking at something similar in bug 1388705?

Although it is not in my interest as bug reporter to say this, I think both bugs are not at all similar. They are both about svg not showing, but the analysis points in a different direction.

But I do suspect the bugs together can make it very hard for webextension authors to use an svg icon.

I will sum up what I think makes using svg hard.

- Spec problem: it is not possible to designate an icon as scalable (webextension only allows specifying the size, not a size range)
- Direction of this bug: 32 and 48 icon from AMO are prefered but are png and thus not scalable
- There are two js files that implement a method to find the best icon. Using slightly different algorithm and one taking into consideration the display and the other not.
- As 1388705 states, the usage of max-width makes it required to set the width of the svg
- In addition to that, the max-width is 32 px in css, but the value is set from js for an icon of 48 px (and additionally scaled for the display)

As author this leads to the following constraints to get the best result:
- Set a width in the svg, the largest width the svg looks good at
- upload a 32 and 48 render of the svg to AMO
- In the webextension manifest, specify the svg for a whole lot of sizes: the minimum the svg looks good at, the maximum the svg looks good at, 31, 33, 47 and 49.

Comment 28

[jmros]

I finally got around to report the spec problem: https://github.com/browserext/browserext/issues/55

No progress on the firefox side, as I don't see a way forward.

Comment 29

[Mark Striemer [:mstriemer]]

I see SVGs working in about:addons now. The part of the page showing icons is now in HTML like about:debugging. There hasn't been any activity on this bug in 2 years, seems likely the issues that we had before are resolved.