886816 - share icon for retina display is empty

Status: RESOLVED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[Shane Caraveo (:mixedpuppy)]

I've noticed that the share icon is empty on retina displays, this will need to uplift all the way :(

Comment 1

[Shane Caraveo (:mixedpuppy)]

Boriss, also need @2 images for share

Comment 2

[Florian Quèze [:florian]]

Attachment: Workaround

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/[e-mail]");
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.

Comment 3

[Shane Caraveo (:mixedpuppy)]

@Boriss, do we have 2x icons for share?

Comment 4

[Matt Wobensmith [:mwobensmith][:matt:]]

Dupe of bug 875212?

Comment 6

[u279076]

(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.

Comment 7

[Jennifer Morrow [:Boriss] (UX)]

Attachment: OSX retina paper plane icons with states default, focused, pressed, toggled, and inactive

Comment 8

[Florian Quèze [:florian]]

Attachment: Patch v2

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.

Comment 9

[Florian Quèze [:florian]]

Attachment: Screenshot with attachment 780355

Comment 10

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 780355 [details] [diff] [review]
Patch v2

looks ok to me, but get a reviewer to look

Comment 11

[Mike Conley (: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/[e-mail]") !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?

Comment 12

[Mike Conley (:mconley) (:⚙️)]

I can't think of a good reason for the social-share-button to be in primaryToolbarButtons. Am I missing something?

Comment 13

[Shane Caraveo (:mixedpuppy)]

Attachment: Patch V3

tested on osx,lin,win but not on retina display.  I do not see any problems removing the button from primaryToolbarButton

Comment 14

[Shane Caraveo (:mixedpuppy)]

Attachment: Patch V3

missed removing 2 other !important

Comment 15

[Shane Caraveo (:mixedpuppy)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7aa74bef708c

Comment 16

[Shane Caraveo (:mixedpuppy)]

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:

Comment 17

[Shane Caraveo (:mixedpuppy)]

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 18

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7aa74bef708c

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6c395ae8c304
https://hg.mozilla.org/releases/mozilla-beta/rev/2949252278a5

Comment 21

[Matt Wobensmith [:mwobensmith][:matt:]]

Verified m-c 2013-08-01

Comment 22

[Tracy Walker [:tracy]]

mass remove verifyme requests greater than 4 months old