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

RESOLVED FIXED in Firefox 48

Status

WebExtensions
Untriaged
P1
normal
RESOLVED FIXED
2 years ago
28 days ago

People

(Reporter: Hans Meyer, Assigned: kmag)

Tracking

({dev-doc-complete})

49 Branch
mozilla50
dev-doc-complete

Firefox Tracking Flags

(firefox46 wontfix, firefox47 wontfix, firefox48+ fixed, firefox49 fixed, firefox-esr45 affected, firefox50 fixed)

Details

(Whiteboard: triaged)

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
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.
(Reporter)

Updated

2 years ago
OS: Unspecified → Windows 10
Hardware: Unspecified → x86_64
(Assignee)

Updated

2 years ago
Status: UNCONFIRMED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1200674
(Assignee)

Updated

2 years ago
Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: DUPLICATE → ---
Summary: 18x browser_action icon appears blurry → Browser actions use the wrong icon size on Windows
(Assignee)

Comment 2

2 years ago
[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.
status-firefox46: --- → affected
status-firefox47: --- → affected
status-firefox48: --- → affected
status-firefox49: --- → affected
status-firefox-esr45: --- → affected
tracking-firefox48: --- → ?
Keywords: dev-doc-needed
(Assignee)

Comment 3

2 years ago
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.
OS: Windows 10 → All
Hardware: x86_64 → All
Summary: Browser actions use the wrong icon size on Windows → Native toolbar button icon size is actually 16px, rather than 18px

Comment 4

2 years ago
kris will do or ask blake.  not much work - but need to uplift so goes in 48 release.
Assignee: nobody → kmaglione+bmo
Priority: -- → P1
Whiteboard: triaged
(Assignee)

Comment 5

2 years ago
Created attachment 8760123 [details]
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/
Attachment #8760123 - Flags: review?(bwinton)
(Assignee)

Updated

2 years ago
Attachment #8760123 - Flags: ui-review?(bwinton)
(Assignee)

Comment 6

2 years ago
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 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.  :)
Attachment #8760123 - Flags: review?(bwinton) → review+
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+…  ;)
(Assignee)

Comment 9

2 years ago
(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 on attachment 8760123 [details]
Bug 1272219: Use 16px icons for BrowserAction buttons, support 18px as a legacy fallback.

Looks great!  Thanks!  :D
Attachment #8760123 - Flags: ui-review?(bwinton) → ui-review+
(Assignee)

Comment 11

2 years ago
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/153553aecf16
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
(Reporter)

Comment 13

2 years ago
(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
(Assignee)

Comment 14

2 years ago
Yes, the icons should be 16px/32px, but 18px currently does not lead to scaling
(Reporter)

Comment 15

2 years ago
(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

2 years ago
Yes, 16px/32px  icons will work fine.
Kris, can we have an uplift request? Thanks
Tracking to make sure we avoid a visual regression.
status-firefox46: affected → wontfix
status-firefox47: affected → wontfix
tracking-firefox48: ? → +
Flags: needinfo?(kmaglione+bmo)
(Assignee)

Comment 18

2 years ago
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.
Flags: needinfo?(kmaglione+bmo)
Attachment #8760123 - Flags: approval-mozilla-beta?
Attachment #8760123 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 19

2 years ago
(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 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
Attachment #8760123 - Flags: approval-mozilla-beta?
Attachment #8760123 - Flags: approval-mozilla-beta+
Attachment #8760123 - Flags: approval-mozilla-aurora?
Attachment #8760123 - Flags: approval-mozilla-aurora+

Comment 21

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/c8e295f7c12f
status-firefox49: affected → fixed

Comment 22

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/0bbdf15e19c1
status-firefox48: affected → fixed

Comment 23

2 years ago
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.
Flags: needinfo?(ps_mdn)
(Reporter)

Updated

2 years ago
Flags: needinfo?(nerd)
Keywords: dev-doc-needed → dev-doc-complete

Updated

28 days ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.