Last Comment Bug 805885 - Set -moz-image-region on the .social-notification-icon-image class
: Set -moz-image-region on the .social-notification-icon-image class
Status: RESOLVED FIXED
[Fx17][qa-]
:
Product: Firefox
Classification: Client Software
Component: SocialAPI (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 19
Assigned To: Jared Wein [:jaws] (please needinfo? me)
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-10-26 11:29 PDT by Jared Wein [:jaws] (please needinfo? me)
Modified: 2013-11-13 03:07 PST (History)
7 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
+
fixed
fixed


Attachments
Patch (3.31 KB, patch)
2012-10-28 01:03 PDT, Jared Wein [:jaws] (please needinfo? me)
felipc: review+
gavin.sharp: approval‑mozilla‑aurora+
gavin.sharp: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Jared Wein [:jaws] (please needinfo? me) 2012-10-26 11:29:10 PDT
+++ 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) 2012-10-26 18:07:21 PDT
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) 2012-10-28 01:03:22 PDT
Created attachment 675930 [details] [diff] [review]
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) 2012-10-28 01:05:01 PDT
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) 2012-10-28 01:48:19 PDT
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) 2012-10-28 02:08:51 PDT
(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 me!) 2012-10-28 15:17:24 PDT
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: gavin@gavinsharp.com] 2012-10-28 15:59:36 PDT
(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) 2012-10-28 16:03:39 PDT
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) 2012-10-28 23:20:42 PDT
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] 2012-10-29 04:09:06 PDT
(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: gavin@gavinsharp.com] 2012-10-29 17:11:45 PDT
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: gavin@gavinsharp.com] 2012-10-29 18:10:08 PDT
Comment on attachment 675930 [details] [diff] [review]
Patch

[Triage Comment]
a=me for the first social release
Comment 13 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-10-29 23:34:02 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/37412cc39dde
Comment 14 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-10-29 23:38:12 PDT
https://hg.mozilla.org/releases/mozilla-beta/rev/cbedc1355544
Comment 15 Ryan VanderMeulen [:RyanVM] 2012-10-30 08:22:52 PDT
https://hg.mozilla.org/mozilla-central/rev/37412cc39dde
Comment 16 Ryan VanderMeulen [:RyanVM] 2012-10-30 11:58:15 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/18d116f5ceda
Comment 17 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-12-04 13:48:48 PST
Please remove [qa-] whiteboard tag and add verifyme keyword if there's some QA testing needed here. Otherwise we will skip verification.

Note You need to log in before you can comment on or make changes to this bug.