Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#4218 closed change (fixed)

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

Reported by: sebastian Assignee: kzar
Priority: P1 Milestone: Adblock-Plus-1.12.2-for-Chrome-Opera-Safari
Module: Platform Keywords:
Cc: oleksandr, kzar, christiane, Ross, scheer Blocked By:
Blocking: Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Unknown Verified working: yes
Review URL(s):

https://codereview.adblockplus.org/29349240/
https://codereview.adblockplus.org/29349820/

Description (last modified by kzar)

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.

Change History (25)

comment:1 Changed 3 years ago by sebastian

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

comment:2 Changed 3 years ago by sebastian

  • Description modified (diff)

comment:3 Changed 3 years ago by 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?)

comment:4 Changed 3 years ago by 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.

comment:6 Changed 3 years ago by sebastian

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

comment:7 Changed 3 years ago by 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.

comment:8 Changed 3 years ago by sebastian

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

comment:9 Changed 3 years ago by kzar

  • Description modified (diff)
  • Platform changed from Edge to Unknown / Cross platform
  • Ready set
  • Summary changed from Add browser action icons for Microsoft Edge to Add browser action icons for Microsoft Edge and newer versions of Chrome

comment:10 Changed 3 years ago by kzar

  • Description modified (diff)

comment:11 Changed 3 years ago by kzar

Cool, updated it now. LGTS?

comment:12 Changed 3 years ago by sebastian

Sounds good to me.

@christiane: Can you provide the missing icons?

comment:13 Changed 3 years ago by kzar

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:14 Changed 3 years ago by abpbot

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

comment:15 Changed 3 years ago by kzar

  • Cc Ross scheer added
  • Description modified (diff)
  • Milestone set to Adblock-Plus-for-Chrome-Opera-Safari-next
  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:16 Changed 3 years ago by kzar

  • Owner set to kzar

comment:17 Changed 3 years ago by sebastian

  • Resolution fixed deleted
  • Status changed from closed to reopened

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.

comment:18 Changed 3 years ago by kzar

  • Priority changed from P2 to P1

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.

comment:19 Changed 3 years ago by kzar

  • Review URL(s) modified (diff)
  • Status changed from reopened to reviewing

comment:20 Changed 3 years ago by abpbot

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

comment:21 Changed 3 years ago by kzar

  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:22 Changed 3 years ago by kzar

  • Resolution fixed deleted
  • Status changed from closed to reopened

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

comment:23 Changed 3 years ago by abpbot

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

comment:24 Changed 3 years ago by kzar

  • Resolution set to fixed
  • Status changed from reopened to closed

Should finally be fixed now!

comment:25 Changed 3 years ago by rraceanu

  • Verified working set

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

Chrome 48 and Chrome 52
ABP 1.21.1.1644

Last edited 3 years ago by rraceanu (previous) (diff)
Note: See TracTickets for help on using tickets.