804068 - Don't set the share icon as the background image on the share button

Status: RESOLVED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[Alfred Kayser]

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]

Attachment: A quick and simple one line patch to change backgroundImage into listStyleImage

Comment 2

[Mark Hammond [:markh] [:mhammond]]

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]

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] [:mhammond]]

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]

Attachment: V2: src = imageURL;

Comment 6

[Alfred Kayser]

Attachment: V3: right version

Comment 7

[Mark Hammond [:markh] [:mhammond]]

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] [:mhammond]]

Attachment: 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)]

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)]

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] [:mhammond]]

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)]

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] [:mhammond]]

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)]

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)]

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]

Is it known what causes these intermittent test timeouts?

Comment 17

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

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] [:mhammond]]

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)]

(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] [:mhammond]]

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]]

https://hg.mozilla.org/mozilla-central/rev/dd4d9137c1a5

Comment 22

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

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 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/951fd1bcedf6
https://hg.mozilla.org/releases/mozilla-beta/rev/4ef2bee6556d

Comment 25

[u279076]

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