887368 - social notification icon not sized correctly on a retina screen

Status: VERIFIED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[Florian Quèze [:florian]]

Attachment: Screenshot

Forcing the height of #social-notification-icon > .toolbarbutton-icon to 16px fixes the issue but I haven't found why this is needed (yet).

The other icon that doesn't look right on the attached screenshot is already handled in bug 886816.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

possibly related to bug 886859?

Comment 2

[Florian Quèze [:florian]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #1)
> possibly related to bug 886859?

Maybe. But the size of the icon is totally reasonable when I move the window to my external (non-retina) monitor.

Comment 3

[Florian Quèze [:florian]]

Attachment: Patch

(In reply to Florian Quèze [:florian] [:flo] from comment #0)

> Forcing the height of #social-notification-icon > .toolbarbutton-icon to
> 16px fixes the issue

This is what I'm doing in this patch.

> but I haven't found why this is needed (yet).

Now I know. The height set on the toolbarbutton isn't propagated to the xul image, which uses the natural height of the image file.

The image file for the indexedDb notification I had on my screenshot has a height of 16px on a normal screen and 32px on a retina one, as this code indicates:
http://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/browser.css#3091
3091 .indexedDB-notification-icon,
3092 #indexedDB-notification-icon {
3093   list-style-image: url(chrome://global/skin/icons/question-16.png);
3094 }
3095 @media (min-resolution: 2dppx) {
3096   .indexedDB-notification-icon,
3097   #indexedDB-notification-icon {
3098     list-style-image: url(chrome://global/skin/icons/question-32.png);
3099   }
3100 }


I'll attach soon another patch that's IMHO cleaner, but also slightly riskier if my understanding of the situation is partial or mistaken.

Comment 4

[Florian Quèze [:florian]]

Attachment: Alternative patch

The approach I'm taking here assumes that the size constraints set on the toolbarbutton isn't propagated to the xul image, so we should set the size on the xul image directly.

This removes a line from my patch for bug 887314 that's no longer necessary, and likely does part of what was intended for bug 886859.

Comment 5

[Florian Quèze [:florian]]

Comment on attachment 768301 [details] [diff] [review]
Alternative patch

This alternative patch isn't correct, we need the width and height set on .notification-anchor-icon directly, as the notifications attached to the location bar are directly xul:image nodes.

Comment 6

[Florian Quèze [:florian]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3c4bbca40720

Comment 7

[:Gavin Sharp [email: [e-mail]]]

What caused this bug?

Comment 8

[Shane Caraveo (:mixedpuppy)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #7)
> What caused this bug?

bug 809085, I added permissions support.  There are a few places I have not handled retina correctly.

Comment 9

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/3c4bbca40720

Comment 10

[Florian Quèze [:florian]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6e574b2c6b8a

Comment 11

[u279076]

Matt, can you please verify this is fixed in Firefox 24 and 25?

Comment 12

[Matt Wobensmith [:mwobensmith][:matt:]]

Attachment: Screenshot of buttons in my build

Pardon me for asking, but my icons don't resemble the screenshot in this bug, so I cannot reproduce the behavior pre- or post-patch in order to verify.

I can also see a Twitter icon in this bug's screenshot, but I can't find instructions on how to get Twitter added via the Social API.

Can someone tell me how to reproduce this bug so that I can verify it fixed on my Retina Mac? Much appreciated, and thanks in advance.

Comment 13

[Shane Caraveo (:mixedpuppy)]

(In reply to Matt Wobensmith from comment #12)
> Created attachment 779828 [details]
> Screenshot of buttons in my build
> 
> Pardon me for asking, but my icons don't resemble the screenshot in this
> bug, so I cannot reproduce the behavior pre- or post-patch in order to
> verify.
> 
> I can also see a Twitter icon in this bug's screenshot, but I can't find
> instructions on how to get Twitter added via the Social API.
> 
> Can someone tell me how to reproduce this bug so that I can verify it fixed
> on my Retina Mac? Much appreciated, and thanks in advance.

The twitter icon is from a mockup provider.  It is better to use the demo provider as it does a geo location request when its sidebar loads.

http://mixedpuppy.github.io/socialapi-demo/

Comment 14

[Matt Wobensmith [:mwobensmith][:matt:]]

Thanks Shane. I still can't see the blue info icon that is visible in the original screenshot. 

What I *did* see was the geolocation icon appear during the permission request. In the pre-patch build, the icon appeared stretched vertically (much like the blue info icon in this bug) and then disappear once permission was granted. On the post-patch build, the same icon appears normal and undistorted. Is this anything close to what we're looking for?

Comment 15

[Shane Caraveo (:mixedpuppy)]

Yes.  The stretched appearance would have affected any of the permission icons.

Comment 16

[Matt Wobensmith [:mwobensmith][:matt:]]

Perfect, thank you Shane.

I confirmed the issue on FF25 2013-06-26.
I verified the fix on FF24, FF25 2013-07-19.

Comment 17

[u279076]

Thanks for your help Matt.