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

RESOLVED FIXED in Firefox 17

Status

()

Firefox
SocialAPI
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Alfred Kayser, Assigned: Alfred Kayser)

Tracking

Trunk
Firefox 19
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox17 fixed, firefox18 fixed, firefox19 fixed)

Details

(Whiteboard: [Fx17][qa-])

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

5 years ago
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.
Blocks: 780987
(Assignee)

Comment 1

5 years ago
Created attachment 673782 [details] [diff] [review]
A quick and simple one line patch to change backgroundImage into listStyleImage
(Assignee)

Updated

5 years ago
Attachment #673782 - Attachment is patch: true
Assignee: nobody → alfredkayser
Status: NEW → ASSIGNED
(Assignee)

Updated

5 years ago
Attachment #673782 - Flags: review?(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 :)
(Assignee)

Comment 3

5 years ago
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 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.
(Assignee)

Comment 5

5 years ago
Created attachment 673871 [details] [diff] [review]
V2: src = imageURL;
Attachment #673782 - Attachment is obsolete: true
Attachment #673782 - Flags: review?(mhammond)
Attachment #673871 - Flags: review?(mhammond)
(Assignee)

Comment 6

5 years ago
Created attachment 673876 [details] [diff] [review]
V3: right version
Attachment #673871 - Attachment is obsolete: true
Attachment #673871 - Flags: review?(mhammond)
Attachment #673876 - Flags: review?(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.
Attachment #673876 - Flags: review?(mhammond) → review+
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)
Attachment #673876 - Attachment is obsolete: true
Attachment #674476 - Flags: review?(jaws)
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.
Attachment #674476 - Flags: review?(jaws) → review+
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.
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.
status-firefox17: --- → affected
status-firefox18: --- → affected
Flags: in-testsuite-
Whiteboard: [Fx17]
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.
Oops - sorry about that!

Relanded with fixed tests: https://hg.mozilla.org/integration/mozilla-inbound/rev/ed8fc02602d4
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
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
(Assignee)

Comment 16

5 years ago
Is it known what causes these intermittent test timeouts?
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
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

5 years ago
(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.
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
https://hg.mozilla.org/mozilla-central/rev/dd4d9137c1a5
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 19
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
Attachment #674476 - Flags: approval-mozilla-beta+
Attachment #674476 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/951fd1bcedf6
https://hg.mozilla.org/releases/mozilla-beta/rev/4ef2bee6556d
status-firefox17: affected → fixed
status-firefox18: affected → fixed
status-firefox19: --- → fixed

Updated

5 years ago
Blocks: 803514
Duplicate of this bug: 803514
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.