1272219 - Native toolbar button icon size is actually 16px, rather than 18px

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P1)

Description

[Hans Meyer]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/50.0.2661.94 Safari/537.36

Steps to reproduce:

Specified browser_action icon in manifest as follows:

  "browser_action": {
    "default_icon": {
      "18": "img/icon18.png",
      "32": "img/icon32.png",
      "36": "img/icon36.png",
      "64": "img/icon64.png"
    }
  },

Confirmed icon18.png dimensions are exactly 18x18 pixels.



Actual results:

Browser action icon appears blurry.

Note that using a 16x16 image for "18" does not result in scaling.

Comment 2

[Kris Maglione [:kmag]]

[Tracking Requested - why for this release]:

We should probably try to get this fixed and uplifted to 48. That's meant to be the first stable release of WebExtensions, and this bug particularly affects Windows users, which is most of our release population.

Comment 3

[Kris Maglione [:kmag]]

Apparently this issue is more complex than we thought. Certain built-in buttons use 18px icons, for the sake 1px outlines, but the native size that's applied by default is 16px. This affects all platforms.

Comment 4

[:shell escalante]

kris will do or ask blake.  not much work - but need to uplift so goes in 48 release.

Comment 5

[Kris Maglione [:kmag]]

Attachment: Bug 1272219: Use 16px icons for BrowserAction buttons, support 18px as a legacy fallback.

Review commit: https://reviewboard.mozilla.org/r/57844/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/57844/

Comment 6

[Kris Maglione [:kmag]]

Comment on attachment 8760123 [details]
Bug 1272219: Use 16px icons for BrowserAction buttons, support 18px as a legacy fallback.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57844/diff/1-2/

Comment 7

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

Comment on attachment 8760123 [details]
Bug 1272219: Use 16px icons for BrowserAction buttons, support 18px as a legacy fallback.

https://reviewboard.mozilla.org/r/57844/#review56220

::: browser/components/extensions/ext-utils.js:105
(Diff revision 2)
>  
>      return result;
>    },
>  
> +  getBestIcon: function(icons, window, size) {
> +  },

Why are we adding this empty function?

It mostly looks good to me, apart from the one thing I mention above.  So, r=me with that fixed.  :)

Comment 8

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

I strongly suspect the UI looks good based on the code and tests, but if you can throw a couple of before/after screenshots, that would really help me give it the ui-r+…  ;)

Comment 9

[Kris Maglione [:kmag]]

(In reply to Blake Winton (:bwinton) (:☕️) (PTO 'til London.  Find me there for quick answers!) from comment #8)
> I strongly suspect the UI looks good based on the code and tests, but if you
> can throw a couple of before/after screenshots, that would really help me
> give it the ui-r+…  ;)

Before: https://people.mozilla.org/~kmaglione/images/480fb07f2e4a43dd.png
After: https://people.mozilla.org/~kmaglione/images/4066e07b9523c2a2.png

Comment 10

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

Comment on attachment 8760123 [details]
Bug 1272219: Use 16px icons for BrowserAction buttons, support 18px as a legacy fallback.

Looks great!  Thanks!  :D

Comment 11

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/153553aecf160632bbcb50906ba135511da138e6
Bug 1272219: Use 16px icons for BrowserAction buttons, support 18px as a legacy fallback. r=bwinton ui-r=bwinton

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/153553aecf16

Comment 13

[Hans Meyer]

(In reply to Kris Maglione [:kmag] from comment #11)
> https://hg.mozilla.org/integration/fx-team/rev/
> 153553aecf160632bbcb50906ba135511da138e6
> Bug 1272219: Use 16px icons for BrowserAction buttons, support 18px as a
> legacy fallback. r=bwinton ui-r=bwinton

For clarity, can you confirm: browser_action icons should be furnished as 16px/32px (using 18px/36px or 19px/38px leads to scaling)

If so, I'll update the docs at https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/browser_action

Comment 14

[Kris Maglione [:kmag]]

Yes, the icons should be 16px/32px, but 18px currently does not lead to scaling

Comment 15

[Hans Meyer]

(In reply to Kris Maglione [:kmag] from comment #14)
> ... 18px currently does not lead to scaling

My mistake. Bad test.

I've updated the docs to prescribe 16px/32px.

Comment 16

[Raj Kumar Maurya]

Yes, 16px/32px  icons will work fine.

Comment 17

[Sylvestre Ledru [:Sylvestre]]

Kris, can we have an uplift request? Thanks
Tracking to make sure we avoid a visual regression.

Comment 18

[Kris Maglione [:kmag]]

Comment on attachment 8760123 [details]
Bug 1272219: Use 16px icons for BrowserAction buttons, support 18px as a legacy fallback.

Approval Request Comment
[Feature/regressing bug #]: Bug 1200674

[User impact if declined]:

This bug causes non-scalable toolbar button icons for WebExtensions to appear noticeably blurry, if they were developed using our documented guidelines. The resulting buttons look decidedly second-class relative to built-in icons, and legacy add-ons, which is a serious problem given that Firefox 48 is a major milestone release for WebExtensions.

The fix allows legacy 18px icons to be shown without scaling, and uses the preferred 16px icon size if available, which gives us acceptable icon appearance for existing and future add-ons.

[Describe test coverage new/current, TreeHerder]: There is comprehensive test coverage for this feature (including the icon selection logic and its application in the UI), both in its new and previous form.

[Risks and why]: Minimal. The main changes introduced by this patch are to remove 2px of padding from buttons using 18px, by applying CSS which already does the same for built-in icons, and to use 16px icons with the default padding when available. The CSS for both icon sizes is mature and well-tested, so the only effect of this change is to give extension icons the sizes that they actually requested.

[String/UUID change made/needed]: None.

Comment 19

[Kris Maglione [:kmag]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #17)
> Kris, can we have an uplift request? Thanks

Sorry to wait so long. I've been on PTO since shortly after this landed, and I wanted to give it some time to bake before I requested uplift.

Comment 20

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8760123 [details]
Bug 1272219: Use 16px icons for BrowserAction buttons, support 18px as a legacy fallback.

No worries, let's take it to fix a visual regression.
Should be in 48 beta 5

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c8e295f7c12f

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/0bbdf15e19c1

Comment 23

[CosminB]

Could you attache a WebExtension with this issue so I can check if is still reproducing?

I could not reproduce this issue with the WebExtensions that I have.