Closed Bug 886816 Opened 6 years ago Closed 6 years ago

share icon for retina display is empty

Categories

(Firefox Graveyard :: SocialAPI, defect)

23 Branch
x86
macOS
defect
Not set

Tracking

(firefox23 fixed, firefox24 fixed, firefox25 verified)

RESOLVED FIXED
Firefox 25
Tracking Status
firefox23 --- fixed
firefox24 --- fixed
firefox25 --- verified

People

(Reporter: mixedpuppy, Assigned: florian)

References

Details

Attachments

(3 files, 3 obsolete files)

I've noticed that the share icon is empty on retina displays, this will need to uplift all the way :(
Boriss, also need @2 images for share
Attached patch Workaround (obsolete) — Splinter Review
The problem is that:
423 @media (min-resolution: 2dppx) {
424   /* Whitelist built-in buttons, instead of .toolbarbutton-1,
425      to avoid potentially breaking add-on toolbar buttons. */
426   :-moz-any(@primaryToolbarButtons@):not(#tabview-button) {
427     list-style-image: url("chrome://browser/skin/Toolbar@2x.png");
428   }
at http://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/browser.css#427 overrides:
1693 #social-share-button {
1694   list-style-image: url("chrome://browser/skin/social/share-button.png");
1695 }
from http://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/browser.css#1693

The attached patch is a trivial workaround that we should take if we need something very low-risk very quickly.

A better solution would be to include an @2x icon for the Share button.

And the real fix is bug 874773.
@Boriss, do we have 2x icons for share?
Flags: needinfo?(jboriss)
Duplicate of this bug: 875212
(In reply to Matt Wobensmith from comment #4)
> Dupe of bug 875212?

Looks like, duped it the other way though since this bug has a patch.
Attached patch Patch v2 (obsolete) — Splinter Review
This new patch includes 2 icons from attachment 780111 [details].

Unfortunately the icons in this attachment are 33x33px and I needed icons at twice the size of the current icons which are at the surprising size of 13x14px. I took the 33x33px icons and scaled them down to 28x28px with GIMP, and finally cut 2 columns of transparent pixels on the right size of the image to obtain 26x28px images that I could use. I assume icons made directly at the right size may look slightly better, but I honestly can't spot any visible issue with the icons I resized, so I'm going to assume they are good enough.

The !important in the CSS code are to override the rules at http://hg.mozilla.org/mozilla-central/annotate/2983ca6d4d1a/browser/themes/osx/browser.css#l418
I don't really like putting !important keywords, but the only other solution I see is what I did in attachment 767864 [details] [diff] [review], which IMHO is uglier.
Assignee: nobody → florian
Attachment #767864 - Attachment is obsolete: true
Attachment #780355 - Flags: review?(mixedpuppy)
Comment on attachment 780355 [details] [diff] [review]
Patch v2

looks ok to me, but get a reviewer to look
Attachment #780355 - Flags: review?(mixedpuppy) → review?(mconley)
Comment on attachment 780355 [details] [diff] [review]
Patch v2

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

::: browser/themes/osx/browser.css
@@ +1690,5 @@
> +}
> +
> +@media (min-resolution: 2dppx) {
> +  #social-share-button {
> +    list-style-image: url("chrome://browser/skin/social/share-button@2x.png") !important;

So it seems to me that we need these !important overrides because #social-share-button is in primaryToolbarButtons.

Why is #social-share-button in primaryToolbarButtons? Does it really need to be there? What advantage does that give us?
I can't think of a good reason for the social-share-button to be in primaryToolbarButtons. Am I missing something?
Flags: needinfo?(dao)
Attached patch Patch V3 (obsolete) — Splinter Review
tested on osx,lin,win but not on retina display.  I do not see any problems removing the button from primaryToolbarButton
Attachment #780355 - Attachment is obsolete: true
Attachment #780355 - Flags: review?(mconley)
Attachment #781899 - Flags: review?(mconley)
Attached patch Patch V3Splinter Review
missed removing 2 other !important
Attachment #781899 - Attachment is obsolete: true
Attachment #781899 - Flags: review?(mconley)
Attachment #781902 - Flags: review?(mconley)
Attachment #781902 - Flags: review?(mconley) → review+
Comment on attachment 781902 [details] [diff] [review]
Patch V3

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): 
String or IDL/UUID changes made by this patch:
Attachment #781902 - Flags: approval-mozilla-beta?
Attachment #781902 - Flags: approval-mozilla-aurora?
sorry, clicked too fast

[Approval Request Comment]
Bug caused by (feature/regressing bug #): share feature
User impact if declined: share button will have no icon on retina display
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none
Comment on attachment 781902 [details] [diff] [review]
Patch V3

low risk, needed on branches, approving now to make sure this gets in before Monday's final beta.
Attachment #781902 - Flags: approval-mozilla-beta?
Attachment #781902 - Flags: approval-mozilla-beta+
Attachment #781902 - Flags: approval-mozilla-aurora?
Attachment #781902 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/7aa74bef708c
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25
QA Contact: mwobensmith
Keywords: verifyme
mass remove verifyme requests greater than 4 months old
Keywords: verifyme
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.