805885 - Set -moz-image-region on the .social-notification-icon-image class

Status: RESOLVED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

+++ This bug was initially created as a clone of Bug #804068 +++

The .social-notification-icon-image class uses width:16px;height:16px; to try to restrict the size of the notification icons. It should use -moz-image-region:rect(0 16px 16px 0); instead.

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

For some reason when I set this style on the element, the buttons flicker when mousing over them. I haven't figured out why yet.

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

This patch uses -moz-image-region to restrict the icon size of the provider icons to 16x16 px. I had to set .style.listStyleImage on the element instead of using .src to fix the flickering when hovering over the elements. I also increased the vertical padding on the buttons in winstripe since they didn't have enough padding and the button heights were two pixels too tall.

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Note that when this patch lands, the Facebook icons will be clipped on the bottom and right, since they are currently sending us 18x18 pixel icons (each has a 1px transparent border). I removed this 1px border locally and confirmed that the icons will look good once Facebook fixes the icons on their side (which they can do at any point in the future since they are web resources and aren't shipped with Firefox).

Comment 4

[Frank Yan (:fryn)]

Comment on attachment 675930 [details] [diff] [review]
Patch

Review of attachment 675930 [details] [diff] [review]:
-----------------------------------------------------------------

I don't think this is sufficient, because it still allows images that are smaller than 16x16 to mess up our layout.

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Frank Yan (:fryn) from comment #4)
> I don't think this is sufficient, because it still allows images that are
> smaller than 16x16 to mess up our layout.

I tested this out with a local 10x10 image for all icons and it didn't affect the layout.

Comment 6

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 675930 [details] [diff] [review]
Patch

Review of attachment 675930 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser-social.js
@@ +812,5 @@
>        // Only update the value attribute if it has changed to reduce layout changes.
>        if (!label.hasAttribute("value") || label.getAttribute("value") != labelValue)
>          label.setAttribute("value", labelValue);
>  
> +      image.style.listStyleImage = "url(" + icon.iconURL + ")";

= 'url("' + encodeURI(icon.iconURL) + '")';

Comment 7

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

(In reply to Jared Wein [:jaws] from comment #2)
> I had to set .style.listStyleImage on the element instead
> of using .src to fix the flickering when hovering over the elements.

Can you elaborate? Why would using .src cause flickering?

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

I'm not sure, it only happens when mousing in and out of the icon. My guess is there is some contention between the default .toolbarbutton-icon list-style-image of Toolbar.png and the .src property. It's not so much flickering, as it is the image disappearing momentarily and collapsing the width of the button.

Comment 9

[Frank Yan (:fryn)]

Comment on attachment 675930 [details] [diff] [review]
Patch

Review of attachment 675930 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Jared Wein [:jaws] from comment #5)
> (In reply to Frank Yan (:fryn) from comment #4)
> > I don't think this is sufficient, because it still allows images that are
> > smaller than 16x16 to mess up our layout.
> 
> I tested this out with a local 10x10 image for all icons and it didn't
> affect the layout.

Okay then.

I'd like to know how exactly src behaves differently from list-style-image that this patch can fix the problem, but I'll defer to Felipe. Good catch on the encodeURI, btw.

Comment 10

[Dão Gottwald [:dao]]

(In reply to Jared Wein [:jaws] from comment #0)
> The .social-notification-icon-image class uses width:16px;height:16px; to
> try to restrict the size of the notification icons. It should use
> -moz-image-region:rect(0 16px 16px 0); instead.

If the icon is too large, why is cutting it better than scaling it? I don't understand this bug.

Comment 11

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

Scaling images is inefficient and generally results in worse quality. By truncating the image, we make the issue of improperly sized icons immediately obvious to people developing a provider, so they won't ship that to users, ideally. A provider accidentally shipping an improperly sized icon to some users but not all seems improbable, but if that ends up being a problem, we can revert this change.

Comment 12

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

Comment on attachment 675930 [details] [diff] [review]
Patch

[Triage Comment]
a=me for the first social release

Comment 13

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/37412cc39dde

Comment 14

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

https://hg.mozilla.org/releases/mozilla-beta/rev/cbedc1355544

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/37412cc39dde

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/18d116f5ceda

Comment 17

[u279076]

Please remove [qa-] whiteboard tag and add verifyme keyword if there's some QA testing needed here. Otherwise we will skip verification.