1272222 - menu icon appears blurry

Status: VERIFIED 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"
    }
  }

All iconN.png sizes are exactly N x N pixels


Actual results:

When dragging browser_action icon from toolbar into menu during customization, resulting menu icon appears blurry due to scaling.

Comment 2

[Hans Meyer]

I think there may be a regression. The menu icon (furnished as 32px/64px) looks OK in FF 48.0b1 but appears scaled in Nightly 50.0a1 (2016-06-17).
OS: Windows 10

"browser_action": {
    "default_icon": {
      "16": "img/icon16.png",
      "32": "img/icon32.png",
      "64": "img/icon64.png"
    }
}

Comment 3

[Kris Maglione [:kmag]]

Attachment: Bug 1272222: Use larger icons for browser actions in the menu panel.

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

Comment 4

[:Gijs (he/him)]

Comment on attachment 8770720 [details]
Bug 1272222: Use larger icons for browser actions in the menu panel.

https://reviewboard.mozilla.org/r/64100/#review61238

I have too many questions.

::: browser/base/content/browser.css:332
(Diff revision 1)
>    display: -moz-box;
>  }
>  
> +@media not all and (min-resolution: 1.1dppx) {
> +  .webextension-browser-action {
> +    list-style-image: var(--toolbar-list-style-image, none);

I'd prefer --webextension-toolbar-image rather than using the generic 'toolbar' without the webextension qualifier, in case we ever end up with conflicts because the browser themes start using this. Ditto for menupanel/urlbar, of course.

Also not sure why the fallback to 'none'. AIUI the getURL code always returns the default webextension icon. Why would we ever need the CSS fallback? How would that even work - a button in the toolbar / url bar with no icon?

::: browser/components/extensions/ext-browserAction.js:151
(Diff revision 1)
>          color = `rgb(${color[0]}, ${color[1]}, ${color[2]})`;
>        }
>        badgeNode.style.backgroundColor = color || "";
>      }
>  
> +

Nit: useless extra whitespace.

::: browser/components/extensions/ext-browserAction.js:156
(Diff revision 1)
> +
>      const LEGACY_CLASS = "toolbarbutton-legacy-addon";
>      node.classList.remove(LEGACY_CLASS);
>  
> +    let baseSize = 16;
> +    let {icon, size} = IconDetails.getURL(tabData.icon, null, this.extension, baseSize);

This is called getURL but it returns both an icon and a size, that seems wrong. And the icon isn't even an actual URL object, but just a string. It also seems from my reading that the string might be relative? At least I don't see where it ever gets resolved to an absolute URL. In which case, I don't understand why it works. I can only assume you tested it and it does in fact work, in which case it must get resolved to an absolute URI somewhere or the whole "set the image from browser/content/browser.css" dance wouldn't work. Where?


Separately... why are you not passing a window? Seems like that should have a comment.

::: browser/components/extensions/ext-browserAction.js:170
(Diff revision 1)
> -    node.setAttribute("image", icon);
> +    let escape = str => str.replace(/"/g, encodeURIComponent);
> +    let getIcon = size => escape(IconDetails.getURL(tabData.icon, null, this.extension, size).icon);

Please please please make getURL just actually return a URL that's properly URL-encoded, then we don't need any of this.

::: browser/components/extensions/ext-pageAction.js:95
(Diff revision 1)
> -      let {icon} = IconDetails.getURL(tabData.icon, window, this.extension);
> -      button.setAttribute("src", icon);
> +      let escape = str => str.replace(/"/g, encodeURIComponent);
> +      let getIcon = size => escape(IconDetails.getURL(tabData.icon, null, this.extension, size).icon);

Ditto.

::: browser/components/extensions/test/browser/browser_ext_browserAction_pageAction_icon.js:297
(Diff revision 1)
> +
> +    yield showBrowserAction(extension);
> +    browserActionButton = browserActionWidget.forWindow(window).node;
> +
> +    for (let resolution of Object.keys(test.menuResolutions)) {
> +      SpecialPowers.setCharPref(RESOLUTION_PREF, resolution);

yield pushPrefEnv instead.

::: browser/components/extensions/test/browser/browser_ext_browserAction_pageAction_icon.js:310
(Diff revision 1)
> +
> +    CustomizableUI.addWidgetToArea(browserActionWidget.id,
> +                                   CustomizableUI.AREA_NAVBAR);
> +  }
> +
> +  SpecialPowers.clearUserPref(RESOLUTION_PREF);

Then remove this.

::: toolkit/components/extensions/ExtensionUtils.jsm:480
(Diff revision 1)
> +    if (window) {
> -    size *= window.devicePixelRatio;
> +      size *= window.devicePixelRatio;
> +    }

This is quite confusing. Can we just get rid of the window parameter and update all the consumers to use the same strategy so that moving windows from hidpi to lodpi etc. "just works"? So also update http://searchfox.org/mozilla-central/source/mobile/android/components/extensions/ext-pageAction.js#67 and other callsites that pass windows?

Comment 5

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/64100/#review61238

> I'd prefer --webextension-toolbar-image rather than using the generic 'toolbar' without the webextension qualifier, in case we ever end up with conflicts because the browser themes start using this. Ditto for menupanel/urlbar, of course.
> 
> Also not sure why the fallback to 'none'. AIUI the getURL code always returns the default webextension icon. Why would we ever need the CSS fallback? How would that even work - a button in the toolbar / url bar with no icon?

For menupanel, I chose the variable names that `menupanel.inc.css` was already using for this purpose. I'm fine with using different names, though.

As for the `none`, yes, we do always fall back to a default icon, but sometimes the CSS parser generates a parse error in the time between when we create the element and when we set the style attribute, if there's no default value.

> This is called getURL but it returns both an icon and a size, that seems wrong. And the icon isn't even an actual URL object, but just a string. It also seems from my reading that the string might be relative? At least I don't see where it ever gets resolved to an absolute URL. In which case, I don't understand why it works. I can only assume you tested it and it does in fact work, in which case it must get resolved to an absolute URI somewhere or the whole "set the image from browser/content/browser.css" dance wouldn't work. Where?
> 
> 
> Separately... why are you not passing a window? Seems like that should have a comment.

Yeah, that naming bit me a couple of times, too. It used to just return a URL, before it got rewritten to handle legacy sizes better.

As for not passing the window, it's because the window is only used for handling hidpi scaling, which is no longer necessary here.

> Please please please make getURL just actually return a URL that's properly URL-encoded, then we don't need any of this.

They should be properly URL-encoded, but in general I prefer not to take any chances when I'm dealing with interpolated strings. Especially when they're used in chrome-privileged contexts.

> This is quite confusing. Can we just get rid of the window parameter and update all the consumers to use the same strategy so that moving windows from hidpi to lodpi etc. "just works"? So also update http://searchfox.org/mozilla-central/source/mobile/android/components/extensions/ext-pageAction.js#67 and other callsites that pass windows?

I'd like to, but unfortunately it's not possible with the Android native UI. It doesn't use CSS, and we have to actually give it a rasterized image with the correct resolution.

I guess we can just have the caller handle the scaling now, though.

Comment 6

[:Gijs (he/him)]

https://reviewboard.mozilla.org/r/64100/#review61238

> I'd like to, but unfortunately it's not possible with the Android native UI. It doesn't use CSS, and we have to actually give it a rasterized image with the correct resolution.
> 
> I guess we can just have the caller handle the scaling now, though.

(as noted, pushing this responsibility onto callers makes sense to me.)

Comment 7

[:Gijs (he/him)]

https://reviewboard.mozilla.org/r/64100/#review61348

::: browser/components/extensions/ext-browserAction.js:156
(Diff revision 1)
> +
>      const LEGACY_CLASS = "toolbarbutton-legacy-addon";
>      node.classList.remove(LEGACY_CLASS);
>  
> +    let baseSize = 16;
> +    let {icon, size} = IconDetails.getURL(tabData.icon, null, this.extension, baseSize);

Well, we can rename it, right? It's all our code... :-)

And yeah, it sounds like we could push the dpi handling to consumers (which we're effectively already doing here) and then remove the parameter, to make it slightly more DRY / not having to pass a world of params we "usually" don't set. :-)

::: browser/components/extensions/ext-browserAction.js:170
(Diff revision 1)
> -    node.setAttribute("image", icon);
> +    let escape = str => str.replace(/"/g, encodeURIComponent);
> +    let getIcon = size => escape(IconDetails.getURL(tabData.icon, null, this.extension, size).icon);

if nsIURI.spec isn't URI-encoding quotes we'll have bigger problems than the insertion into some CSS variables (which practically still doesn't really get you anywhere much... I'm fairly sure css variables can't 'escape' property values and construct extra property names + values or whatever). If you are very intent on keeping this, please add a comment that it shouldn't be necessary but you're trying to be extra careful or whatever. Might also make sense to just write tests to cover this.

I'm not really sure what the webextensions threat model is in this case, but where does this stuff get checked for e.g. js or data URIs, or SVG that runs script? (I think we disable script if the SVG is loaded in an <img> but I could be wrong. I've certainly not checked. Might be worth a separate bug as it's technically not a new thing for what you're doing here?)

Comment 8

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/64100/#review61348

> if nsIURI.spec isn't URI-encoding quotes we'll have bigger problems than the insertion into some CSS variables (which practically still doesn't really get you anywhere much... I'm fairly sure css variables can't 'escape' property values and construct extra property names + values or whatever). If you are very intent on keeping this, please add a comment that it shouldn't be necessary but you're trying to be extra careful or whatever. Might also make sense to just write tests to cover this.
> 
> I'm not really sure what the webextensions threat model is in this case, but where does this stuff get checked for e.g. js or data URIs, or SVG that runs script? (I think we disable script if the SVG is loaded in an <img> but I could be wrong. I've certainly not checked. Might be worth a separate bug as it's technically not a new thing for what you're doing here?)

I'm not worried about the variable so much as the "style" property itself. If something changed, and a caller somehow managed to pass a string that wasn't properly URL-escaped, and we wound up with `"); --moz-binding: url("` ..., or something like that, it could be a huge problem.

It would definitely make sense to add tests for it.

The values get checked and normalized here: http://dev.searchfox.org/mozilla-central/source/toolkit/components/extensions/ExtensionUtils.jsm#410

And, yes, we do disable scripts and external resources for SVGs loaded this way.

Comment 9

[Kris Maglione [:kmag]]

Comment on attachment 8770720 [details]
Bug 1272222: Use larger icons for browser actions in the menu panel.

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

Comment 10

[:Gijs (he/him)]

Comment on attachment 8770720 [details]
Bug 1272222: Use larger icons for browser actions in the menu panel.

https://reviewboard.mozilla.org/r/64100/#review61618

r=me with the resolved URI-ness and CSS warnings / adding items without icons stuff sorted out.

::: browser/base/content/browser.css:332
(Diff revision 2)
>    display: -moz-box;
>  }
>  
> +@media not all and (min-resolution: 1.1dppx) {
> +  .webextension-browser-action {
> +    list-style-image: var(--webextension-toolbar-image, none);

FWIW, I don't see why this would ever warn. `onCreated` should be called before the button is inserted into the DOM, so the variable should be defined at all times when the button is in the document. Can you add a comment here and file a followup bug? The only thing I can think of off-hand is setting the browser-action class only after definining this image in the style attribute. Maybe that'll help.

::: browser/components/extensions/ext-contextMenus.js:59
(Diff revision 2)
>          let parentWindow = contextData.menu.ownerDocument.defaultView;
>          let extension = root.extension;
>  
> -        let {icon} = IconDetails.getURL(extension.manifest.icons, parentWindow, extension, 16 /* size */);
> +        let {icon} = IconDetails.getPreferredIcon(extension.manifest.icons, extension,
> +                                                  16 * parentWindow.devicePixelRatio);
>          let resolvedURL = root.extension.baseURI.resolve(icon);

If these are fully resolved URLs, why is this necessary here? (And if necessary here, why isn't it for the other consumers?)

::: browser/components/extensions/ext-pageAction.js:118
(Diff revision 2)
>    addButton(window) {
>      let document = window.document;
>  
>      let button = document.createElement("image");
>      button.id = this.id;
> -    button.setAttribute("class", "urlbar-icon");
> +    button.setAttribute("class", "urlbar-icon webextension-page-action");

Looks like we should call `updateButton` here or we'll initially add the button without an icon, which would explain the warnings you were seeing?

Comment 11

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/64100/#review61618

> FWIW, I don't see why this would ever warn. `onCreated` should be called before the button is inserted into the DOM, so the variable should be defined at all times when the button is in the document. Can you add a comment here and file a followup bug? The only thing I can think of off-hand is setting the browser-action class only after definining this image in the style attribute. Maybe that'll help.

I never actually got an error for the browser action, only the page action. I just figured it would be best to add the fallback to all of the variables, just to be safe.

> If these are fully resolved URLs, why is this necessary here? (And if necessary here, why isn't it for the other consumers?)

The icons in the manifest are a special case. They're used by the add-on manager even when the add-on is not enabled, so we don't pre-resolve them. All of the other callers handle normalization when the `icons` object is created, rather than when it's used.

> Looks like we should call `updateButton` here or we'll initially add the button without an icon, which would explain the warnings you were seeing?

This gets called from `updateButton` (and in theory only from `updateButton`), so we can't call it from here. But adding the class from `updateButton` instead of here solves the problem, so I'll do that instead.

Comment 12

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/c47d2ca42dff8772f2e86be895ff331346a37b6c
Bug 1272222: Use larger icons for browser actions in the menu panel. r=Gijs

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c47d2ca42dff

Comment 14

[CosminB]

I tested some WebExtensions and  this issue is reproducing to some of them.

Here is a screenshot: http://screencast.com/t/ZVytcZtNS

Here are some WebExtensions with this issue:
https://addons-dev.allizom.org/en-US/firefox/addon/alexa-traffic-rank/
https://addons-dev.allizom.org/en-US/firefox/addon/gismeteooo/ 

Verified on: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:50.0) Gecko/20100101 Firefox/50.0

Comment 15

[CosminB]

Here is another screenshot: http://screencast.com/t/IY4PmUVTUA

Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:50.0) Gecko/20100101 Firefox/50.0

Comment 16

[Kris Maglione [:kmag]]

This is expected. Those add-ons only provide 16px icons, so they're scaled up to fit. They need to provide 32px and/or 64px icons to prevent them from looking blurry in the menu panel.

Comment 18

[Will Bamberg [:wbamberg]]

I'm pretty sure this is already documented:

https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/browser_action#Choosing_icon_sizes

Am I missing anything here?

Comment 19

[Kris Maglione [:kmag]]

Sorry, you're right. The docs match the new behavior. They didn't match the old.

Comment 20

[CosminB]

I can reproduce this issue on Firefox 49.0a1 (20160511030221).

This issue is verified as fixed on Firefox 50.0b9 (20161020152750) and 52.0a1 (20161020030211) under Windows 7 64-bit, the images are loaded correctly in the menu panel.
Here are some videos for this issue: 32px - http://screencast.com/t/SEvCy1kSwU 
				     64px - http://screencast.com/t/I2B5WD2OQl