Closed
Bug 1203738
Opened 9 years ago
Closed 8 years ago
Update browserAction and pageAction icons after pixel density change
Categories
(WebExtensions :: Untriaged, defect, P3)
WebExtensions
Untriaged
Tracking
(Not tracked)
RESOLVED
DUPLICATE
of bug 1272222
People
(Reporter: wbamberg, Unassigned, Mentored)
References
Details
(Keywords: good-first-bug, Whiteboard: [browserAction][webextensions-polish][berlin]triaged)
Attachments
(1 file)
(deleted),
application/x-xpinstall
|
Details |
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
Updated•9 years ago
|
Whiteboard: [browserAction]
Updated•9 years ago
|
Whiteboard: [browserAction] → [browserAction][webextensions-polish]
Comment 1•9 years ago
|
||
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
Comment 2•9 years ago
|
||
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
Comment 3•9 years ago
|
||
(I'll give it a try when I get to the office tomorrow, if no-one has picked it up by then.)
Comment 4•9 years ago
|
||
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.
Comment 6•9 years ago
|
||
The #loop-button _seems_ to… I think Mike Conley might know more (or know who would know more).
Comment 7•9 years ago
|
||
(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.
Comment 8•9 years ago
|
||
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
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•9 years ago
|
Flags: blocking-webextensions+
Updated•9 years ago
|
Whiteboard: [browserAction][webextensions-polish][good first bug] → [browserAction][webextensions-polish][good first bug]triaged
Updated•9 years ago
|
Flags: blocking-webextensions+ → blocking-webextensions-
Updated•9 years ago
|
Whiteboard: [browserAction][webextensions-polish][good first bug]triaged → [browserAction][webextensions-polish][good first bug][berlin]triaged
Updated•8 years ago
|
Keywords: good-first-bug
Whiteboard: [browserAction][webextensions-polish][good first bug][berlin]triaged → [browserAction][webextensions-polish][berlin]triaged
Comment 9•8 years ago
|
||
This was fixed by bug 1272222.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → DUPLICATE
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•