Add browser action icons for Microsoft Edge and newer versions of Chrome

[sebastian]

Background

In Chrome the four ​browser action icons were always displayed as 19x19px. To support high density displays we also needed to provide the images at 2x resolution so 38x38px.

In newer versions of Chrome, ​with the switch to material design the icons will instead be displayed as 16x16px.

Also we are adding support for the Microsoft Edge browser and there the icons are displayed as 20x20px.

So to avoid the browsers scaling our icons and potentially adding artifacts we need to provide the four icons in the following resolutions: 16px, 19px, 20px, 32px, 38px, 40px.

What to change

Create 16x16px, 20x20px, 32x32px and 40x40px versions of the four browser icons abp-X-notification-critical.png, abp-X-notification-information.png, abp-X-whitelisted.png, abp-X.png. Placing them in the chrome/icons directory.

Ensure they are all added to the browser_action.default_icons field in the generated mainfest.json:

  "browser_action": {
    "default_icon": {
      "16": "icons/abp-16.png",
      "19": "icons/abp-19.png",
      "20": "icons/abp-20.png",
      "32": "icons/abp-32.png",
      "38": "icons/abp-38.png",
      "40": "icons/abp-40.png"
    }, 

Also ensure the new resolution images are considered by all code that changes the icon, for example the animation code in lib/icon.js.

Hints for testers

Ensure the browser icons still work for Chrome and Microsoft Edge. When using a ​more modern version of Chrome that uses material design ensure the icon isn't scaled and fuzzy. Same for Microsoft Edge, ensure the icon looks crisp.

Ensure that icon animations still work for the various supported browsers. You can do this simply by typing the following commands into the extension's background console:

// Play icon animation
require("icon").startIconAnimation("critical");
// and stop it
require("icon").stopIconAnimation("critical");

Ensure the icons haven't broken for Safari too.

[sebastian]

@christiane: Mind creating a 20x20px and 40x40px version of the Adblock Plus icon and attach it to this issue?

[kzar]

(I noticed the icon in Chrome looks a bit fuzzy since I upgraded to Chrome 51 the other day. I think the preferred icon size there is now 16x16px although I'm not sure. Perhaps we should provide correct size icons for newer versions of Chrome too while at it?)

[sebastian]

According to the ​documentation, 19px and 38px is still the correct size for Chrome. If it doesn't render properly please file a Chrome bug.

[kzar]

​https://bugs.chromium.org/p/chromium/issues/detail?id=546206
​http://stackoverflow.com/questions/37735149/browser-action-icon-size-changing-from-19x-to-16x

[sebastian]

Great, I see. Feel free to update the issue or file a separate one.

[kzar]

I don't mind updating this issue but first I just want to double check which icons need updating. Is it ​the four icons here?

I guess we'll need the following sizes for them now 16px, 19px, 20px, 32px, 38px, 40px.

[sebastian]

Yes, we do. Note that we already have icons in 16px and 32px but don't use them for the browser action yet.

[kzar]

Cool, updated it now. LGTS?

[sebastian]

Sounds good to me.

@christiane: Can you provide the missing icons?

[abpbot]

A commit referencing this issue has landed:
​Issue 4218 - Browser icons for newer Chrome + Edge

[sebastian]

When toggling "Disabled/Enabled on this site", the icon doesn't update, but I get following exception, in Chrome 47, since this change landed:

Error in response to tabs.get: Error: Invalid value for argument 1. Property 'path': Value does not match any valid type choices.
    at Object.BrowserAction._applyChanges (chrome-extension://nlohjgnkebefjephgiamhedijajoblie/ext/background.js:227:30)
    at Object.<anonymous> (chrome-extension://nlohjgnkebefjephgiamhedijajoblie/ext/background.js:274:16)
    at Object.BrowserAction._queueChanges (chrome-extension://nlohjgnkebefjephgiamhedijajoblie/ext/background.js:258:19)
    at Object.BrowserAction._addChange (chrome-extension://nlohjgnkebefjephgiamhedijajoblie/ext/background.js:283:14)
    at Object.BrowserAction.setIcon (chrome-extension://nlohjgnkebefjephgiamhedijajoblie/ext/background.js:289:12)
    at setIcon (chrome-extension://nlohjgnkebefjephgiamhedijajoblie/lib/adblockplus.js:720:28)
    at chrome-extension://nlohjgnkebefjephgiamhedijajoblie/lib/adblockplus.js:737:7
    at Object.require.scopes.events.exports.EventEmitter.emit (chrome-extension://nlohjgnkebefjephgiamhedijajoblie/lib/adblockplus.js:3352:18)
    at revalidateWhitelistingState (chrome-extension://nlohjgnkebefjephgiamhedijajoblie/lib/adblockplus.js:2455:20)

It seems that (at least old versions of) Chrome expect no properties other than 19 and 38 to be in the paths objects passed to browserActsion.setIcon. The extra sizes given in the mainfest.json however seem to be fine.

[kzar]

Well this is interesting, if you remove the other resolutions you're back to fuzzy icons for newer versions of Chrome with material design, but if you don't older versions of Chrome break! Looks like we'll have to check the browser's version before picking the correct sizes.

[abpbot]

A commit referencing this issue has landed:
​Issue 4218 - Fix setIcon for older versions of Chrome

[kzar]

(Sebastian noticed that icon animations are still broken for older versions of Chrome, argh...)

[abpbot]

A commit referencing this issue has landed:
​Issue 4218 - Fix icon animations for older versions of Chrome

[kzar]

Should finally be fixed now!

[rraceanu]

Icon animation transition is functional (Notification and whitelisting); No scaling or blur/fuzziness issues.

Chrome 48 and Chrome 52
ABP 1.21.1.1644