Update browserAction and pageAction icons after pixel density change

RESOLVED DUPLICATE of bug 1272222

Status

()

Toolkit
WebExtensions: Untriaged
P3
normal
RESOLVED DUPLICATE of bug 1272222
2 years ago
10 months ago

People

(Reporter: wbamberg, Unassigned, Mentored)

Tracking

({good-first-bug})

unspecified
good-first-bug
Points:
---
Dependency tree / graph
Bug Flags:
blocking-webextensions -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [browserAction][webextensions-polish][berlin]triaged)

Attachments

(1 attachment)

3.76 KB, application/x-xpinstall
Details
(Reporter)

Description

2 years ago
Created attachment 8659568 [details]
test.xpi

With browser actions you're supposed to be able to supply 2 icons, one 19x19 and one 38x38, and the browser picks the right one for your pixel density.

On my (Retina) display, I'm seeing Firefox pick the 19x19 icon, while Chrome picks the 38x38 icon.

It looks like this call: https://dxr.mozilla.org/mozilla-central/source/browser/components/extensions/ext-browserAction.js#186 is giving me res.value of 1, and that's why we are choosing the 19x19 icon.

I've attached a test WebExtension that shows the problem.
Mentor: wmccloskey@mozilla.com

Updated

2 years ago
Whiteboard: [browserAction]
Whiteboard: [browserAction] → [browserAction][webextensions-polish]
This should be fixed by my patch for bug 1197422, which should implement the correct logic for pageAction, and extends it to browserAction. I haven't been able to test on an actual Retina display yet, though, so no promises.
Assignee: nobody → kmaglione+bmo
Depends on: 1197422
In fact, if you (or anyone else with a Retna mac) could test this try build for the correct behavior, I'd appreciate it: https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/maglione.k@gmail.com-93b3bb291925/try-macosx64/firefox-44.0a1.en-US.mac.dmg
(I'll give it a try when I get to the office tomorrow, if no-one has picked it up by then.)
Okay, so, here's what I found: https://dl.dropboxusercontent.com/u/2301433/Firefox/WebExtensions/Bug1203738.png
If I enable the add-on on the retina screen I get the correct pawprint icon.
If I enable the add-on on the non-retina screen (labeled @1x in the image for space reasons) I get the correct globe icon.

If I move the add-on from the retina screen to the non-retina screen, or vice versa, the icon doesn't update.
Should it?  Probably.  Would I rather get this checked in and fix that in a follow up bug?  Hell yes!  ;)

So, when you upload the patch, I think you can say ui-r=me with a followup bug.  :)

Thanks!
Blake, do any of our other toolbar icons update when you change screens? If so, I'd like to see how they do it. Maybe we could use the same mechanism.
The #loop-button _seems_ to…  I think Mike Conley might know more (or know who would know more).
(In reply to Blake Winton (:bwinton) (:☕️) from comment #4)
> If I move the add-on from the retina screen to the non-retina screen, or
> vice versa, the icon doesn't update.
> Should it?  Probably.  Would I rather get this checked in and fix that in a
> follow up bug?  Hell yes!  ;)

Ideally, yeah. But there are complications, so I decided to save it for a follow-up bug. In any case, if we do correctly change pixel density when moving between screens, the icon should update for the new resolution the first time you change a tab, so it's a fairly short-lived problem.

Thanks for testing.

(In reply to Bill McCloskey (:billm) from comment #5)
> Blake, do any of our other toolbar icons update when you change screens? If
> so, I'd like to see how they do it. Maybe we could use the same mechanism.

The other toolbar buttons use @media queries and list-style-image to set the images, so they should update on pixel density change. I looked into ways to do the same for browserAction/pageAction buttons, but it's not trivial. The two most workable ways I could come up with were to add a stylesheet to the browser document that assigns an icon to the button using media queries, or to add a second image to the toolbarbutton binding, and select the correct one via CSS.

I didn't really like either of those approaches. Ideally, we could just use an attr() selector with list-style-image to pick the right image attribute via CSS, but we don't currently support that. Someone else might be able to think of a better option.

The easiest thing, at least for now, would probably be to just refresh the image URL when the pixel density changes.

Updated

2 years ago
Blocks: 1214433
Priority: -- → P3
Slightly retargeting this.

I did a bit of digging, and it looks like the best way to implement this is to listen for browser window `resize` events, and refresh the images if `window.devicePixelRatio` has changed.
Assignee: kmaglione+bmo → nobody
Mentor: kmaglione+bmo@mozilla.com
Summary: browserAction code doesn't correctly detect pixel density → Update browserAction and pageAction icons after pixel density change
Whiteboard: [browserAction][webextensions-polish] → [browserAction][webextensions-polish][good first bug]

Updated

2 years ago
Flags: blocking-webextensions+
Depends on: 1218175
No longer depends on: 1218175

Updated

a year ago
Whiteboard: [browserAction][webextensions-polish][good first bug] → [browserAction][webextensions-polish][good first bug]triaged

Updated

a year ago
Flags: blocking-webextensions+ → blocking-webextensions-
Whiteboard: [browserAction][webextensions-polish][good first bug]triaged → [browserAction][webextensions-polish][good first bug][berlin]triaged

Updated

10 months ago
Keywords: good-first-bug
Whiteboard: [browserAction][webextensions-polish][good first bug][berlin]triaged → [browserAction][webextensions-polish][berlin]triaged
This was fixed by bug 1272222.
Status: NEW → RESOLVED
Last Resolved: 10 months ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1272222
You need to log in before you can comment on or make changes to this bug.