Closed Bug 1272219 Opened 8 years ago Closed 8 years ago

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

Categories

(WebExtensions :: Untriaged, defect, P1)

49 Branch
defect

Tracking

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

RESOLVED FIXED
mozilla50
Tracking Status
firefox46 --- wontfix
firefox47 --- wontfix
firefox48 + fixed
firefox49 --- fixed
firefox-esr45 --- affected
firefox50 --- fixed

People

(Reporter: chef, Assigned: kmag)

Details

(Keywords: dev-doc-complete, Whiteboard: triaged)

Attachments

(1 file)

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.
OS: Unspecified → Windows 10
Hardware: Unspecified → x86_64
Status: UNCONFIRMED → RESOLVED
Closed: 8 years ago
Resolution: --- → DUPLICATE
Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: DUPLICATE → ---
Summary: 18x browser_action icon appears blurry → Browser actions use the wrong icon size on Windows
[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.
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
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
Attachment #8760123 - Flags: ui-review?(bwinton)
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+…  ;)
(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+
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
https://hg.mozilla.org/mozilla-central/rev/153553aecf16
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
(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
Yes, the icons should be 16px/32px, but 18px currently does not lead to scaling
(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.
Yes, 16px/32px  icons will work fine.
Kris, can we have an uplift request? Thanks
Tracking to make sure we avoid a visual regression.
Flags: needinfo?(kmaglione+bmo)
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?
(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+
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)
Flags: needinfo?(nerd)
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: