Last Comment Bug 804068 - Don't set the share icon as the background image on the share button
: Don't set the share icon as the background image on the share button
Status: RESOLVED FIXED
[Fx17][qa-]
:
Product: Firefox
Classification: Client Software
Component: SocialAPI (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 19
Assigned To: Alfred Kayser
:
Mentors:
: 803514 (view as bug list)
Depends on:
Blocks: 780987 803514
  Show dependency treegraph
 
Reported: 2012-10-22 00:26 PDT by Alfred Kayser
Modified: 2013-11-12 00:57 PST (History)
6 users (show)
markh: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed
fixed


Attachments
A quick and simple one line patch to change backgroundImage into listStyleImage (1.00 KB, patch)
2012-10-22 00:41 PDT, Alfred Kayser
no flags Details | Diff | Review
V2: src = imageURL; (986 bytes, patch)
2012-10-22 07:22 PDT, Alfred Kayser
no flags Details | Diff | Review
V3: right version (980 bytes, patch)
2012-10-22 07:38 PDT, Alfred Kayser
markh: review+
Details | Diff | Review
CSS changes to make the image the correct size (2.87 KB, patch)
2012-10-23 18:21 PDT, Mark Hammond [:markh]
jaws: review+
gavin.sharp: approval‑mozilla‑aurora+
gavin.sharp: approval‑mozilla‑beta+
Details | Diff | Review

Description Alfred Kayser 2012-10-22 00:26:59 PDT
From bg 780987:
Why is the icon image on the share button set as background image?
    shareButton.style.backgroundImage = 'url("' + encodeURI(imageURL) + '")';
This should have been .src.
Comment 1 Alfred Kayser 2012-10-22 00:41:14 PDT
Created attachment 673782 [details] [diff] [review]
A quick and simple one line patch to change backgroundImage into listStyleImage
Comment 2 Mark Hammond [:markh] 2012-10-22 03:13:22 PDT
hrmph - logically it is a background image.  MDN says about listStyleImage that "The list-style-image CSS property sets the image that will be used as the list item marker" - which doesn't seem to apply

So I really don't understand what the problem is and what the patch solves - so from my POV it needs a comment justifying the seemingly non-logical change.  I'm not saying it *is* illogical though, it just *seems* so to me :)
Comment 3 Alfred Kayser 2012-10-22 03:21:26 PDT
The problem of using background-image, is that themes can then not use a background-image to style the button. There are ways to work around it, but it is not nice. The issue really is that share-button is a 'image', so one would expect that the actual icon image is set as such.
Possibly instead of listStyleImage, actually 'src' should be used.
Comment 4 Mark Hammond [:markh] 2012-10-22 03:28:45 PDT
Comment on attachment 673782 [details] [diff] [review]
A quick and simple one line patch to change backgroundImage into listStyleImage

Thanks, makes sense - but I'd prefer setting src as it wouldn't have been necessary for me to ask the question :)  I'll leave it to your judgement though, so this is fine if you prefer.
Comment 5 Alfred Kayser 2012-10-22 07:22:35 PDT
Created attachment 673871 [details] [diff] [review]
V2: src = imageURL;
Comment 6 Alfred Kayser 2012-10-22 07:38:05 PDT
Created attachment 673876 [details] [diff] [review]
V3: right version
Comment 7 Mark Hammond [:markh] 2012-10-22 14:59:55 PDT
Comment on attachment 673876 [details] [diff] [review]
V3: right version

I'm a little surprised you don't need to setAttribute("src", ...); but assuming you have tested it, that's fine with me.  Let me know if you haven't tested it and I'll do it ASAP.
Comment 8 Mark Hammond [:markh] 2012-10-23 18:21:48 PDT
Created attachment 674476 [details] [diff] [review]
CSS changes to make the image the correct size

With just the code change the image appeared shrunken.  This new patch also adjusts the css of the button to match other similar items and it appears normally (but only actually tested using winstripe)
Comment 9 Jared Wein [:jaws] (please needinfo? me) 2012-10-23 19:15:31 PDT
Comment on attachment 674476 [details] [diff] [review]
CSS changes to make the image the correct size

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

::: browser/themes/gnomestripe/browser.css
@@ +1383,1 @@
>  }

These #share-button rules shouldn't be necessary anymore.
Comment 10 Jared Wein [:jaws] (please needinfo? me) 2012-10-23 19:16:47 PDT
Comment on attachment 674476 [details] [diff] [review]
CSS changes to make the image the correct size

>--- a/browser/themes/gnomestripe/browser.css
>+++ b/browser/themes/gnomestripe/browser.css
> #share-button {
>-  width: 16px;
>-  height: 16px;
>+  -moz-image-region: rect(0, 16px, 16px, 0);
> }

Splinter didn't show much context, I meant that these #share-button rules can now be axed.
Comment 11 Mark Hammond [:markh] 2012-10-23 19:48:37 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9c1eb0c7b8d

I'm guessing we may want to uplift this if it means themes are will look ugly without it.
Comment 12 Phil Ringnalda (:philor) 2012-10-23 21:30:39 PDT
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/a3a24807434c - as https://tbpl.mozilla.org/php/getParsedLog.php?id=16402915&tree=Mozilla-Inbound says, there's a browser-chrome test that was still trying to test what was no longer true.
Comment 13 Mark Hammond [:markh] 2012-10-23 21:50:20 PDT
Oops - sorry about that!

Relanded with fixed tests: https://hg.mozilla.org/integration/mozilla-inbound/rev/ed8fc02602d4
Comment 14 Jared Wein [:jaws] (please needinfo? me) 2012-10-23 22:17:49 PDT
I talked with Frank offline and he said that we should have kept the -moz-image-region since it will protect against images that are larger than 16px by 16px.

Pushed a follow-up to inbound,
https://hg.mozilla.org/integration/mozilla-inbound/rev/95aa292658aa
Comment 15 Jared Wein [:jaws] (please needinfo? me) 2012-10-23 23:47:43 PDT
Backed out due to intermittent test timeouts on browser_social_shareButton.js on Linux and on WinXP.

https://hg.mozilla.org/integration/mozilla-inbound/rev/8dffd0757c9c
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d2cb96108c1
Comment 16 Alfred Kayser 2012-10-25 00:35:01 PDT
Is it known what causes these intermittent test timeouts?
Comment 17 Jared Wein [:jaws] (please needinfo? me) 2012-10-25 17:35:57 PDT
I looked in to the timeouts and found the issue.

This is the TBPL push, https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=ed8fc02602d4

If you look at this screenshot of the test when it hangs, you can see that two share buttons are showing http://screencast.com/t/gKflLvDrvh

The share and unshare image that is set in the test is this image, http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/social_share_image.png, which should only be a 16x16 image for each of them. I'm curious if just changing that test image to be a 16x16 image would fix the test hang.

These share images are set here, http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/social_worker.js#111
Comment 18 Mark Hammond [:markh] 2012-10-25 17:44:56 PDT
Interestingly, that implies the css styling which is supposed to ensure only a 16x16 version of the image is used (ie:

  -moz-image-region: rect(0, 16px, 16px 0);
)

isn't working correctly.  Note also that I'm looking at a try push I made at https://tbpl.mozilla.org/?tree=Try&rev=07aa210ea42c - this try push includes the .css, but only goes orange on 2 out of 13 runs - and that retrying those runs went green (on at least one of the 2 - the other is pending).

So I'm not sure what is going on here, but it *looks* to me that the fact we don't use a 16x16 image isn't the root problem.
Comment 19 Frank Yan (:fryn) 2012-10-25 21:43:44 PDT
(In reply to Mark Hammond (:markh) from comment #18)
> Interestingly, that implies the css styling which is supposed to ensure only
> a 16x16 version of the image is used (ie:
> 
>   -moz-image-region: rect(0, 16px, 16px 0);

There's a typo.
It should be:

-moz-image-region: rect(0, 16px, 16px, 0);

the property also accepts no commas, e.g. -moz-image-region: rect(0 16px 16px 0);
but I don't think it accepts a mixed version with 0 < n < 3 commas.
Comment 20 Mark Hammond [:markh] 2012-10-25 21:57:53 PDT
repushed with the css typos fixed: https://hg.mozilla.org/integration/mozilla-inbound/rev/dd4d9137c1a5

Previous orange was fixed by changing the tests to use element.click() rather than  EventUtils.synthesizeMouseAtCenter(element, {}); - thanks to bug 775089 for the clue to that.

Green try at https://tbpl.mozilla.org/?tree=Try&rev=3436d9b483d9
Comment 21 Ryan VanderMeulen [:RyanVM] 2012-10-26 04:33:49 PDT
https://hg.mozilla.org/mozilla-central/rev/dd4d9137c1a5
Comment 22 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-10-27 17:54:17 PDT
Comment on attachment 674476 [details] [diff] [review]
CSS changes to make the image the correct size

[Triage Comment]
a=me since this is social-only and blocks bug 803514
Comment 24 Jared Wein [:jaws] (please needinfo? me) 2012-10-30 15:10:42 PDT
*** Bug 803514 has been marked as a duplicate of this bug. ***
Comment 25 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-12-04 14:58:33 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.