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

RESOLVED FIXED in Firefox 17

Status

()

Firefox
SocialAPI
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: jaws, Assigned: jaws)

Tracking

Trunk
Firefox 19
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox17+ fixed, firefox18+ fixed, firefox19 fixed)

Details

(Whiteboard: [Fx17][qa-])

Attachments

(1 attachment)

+++ 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.
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.
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.
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Attachment #675930 - Flags: review?(fryn)
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

5 years ago
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.
(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.
Attachment #675930 - Flags: review?(felipc)
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) + '")';
Attachment #675930 - Flags: review?(felipc) → review+
(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?
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

5 years ago
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.
Attachment #675930 - Flags: review?(fryn)
(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.
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 on attachment 675930 [details] [diff] [review]
Patch

[Triage Comment]
a=me for the first social release
Attachment #675930 - Flags: approval-mozilla-beta+
Attachment #675930 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/integration/mozilla-inbound/rev/37412cc39dde
Target Milestone: --- → Firefox 19
https://hg.mozilla.org/releases/mozilla-beta/rev/cbedc1355544
status-firefox17: affected → fixed
tracking-firefox17: --- → +
tracking-firefox18: --- → +
https://hg.mozilla.org/mozilla-central/rev/37412cc39dde
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
https://hg.mozilla.org/releases/mozilla-aurora/rev/18d116f5ceda
status-firefox18: affected → fixed
status-firefox19: --- → fixed
Please remove [qa-] whiteboard tag and add verifyme keyword if there's some QA testing needed here. Otherwise we will skip verification.
Whiteboard: [Fx17] → [Fx17][qa-]
You need to log in before you can comment on or make changes to this bug.