935773 - Social Bookmark button icon doesn't show on Windows

Status: VERIFIED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[u279076]

Attachment: Screenshot - Windows XP.png

1. Load http://mixedpuppy.github.io/socialapi-demo/
2. Click "Activate the Demo Provider"
3. Click "Enable Services"

Expected:
Social Bookmarks button appears in the toolbar

Actual:
Social Bookmarks button appears as a blank button without icons.

I've checked the manifest and the icon URLs appear valid. I do get some errors in Browser Console though:
> 15:01:55.810 WorkerAPI: topic doesn't have a handler: 'worker.connected' WorkerAPI.jsm:41
> 15:01:55.811 WorkerAPI: topic doesn't have a handler: 'social.page-mark-config' WorkerAPI.jsm:41
> 15:01:57.113 Offline cache manifest item failed to load, URL=http://mixedpuppy.github.io/offline.html
> 15:01:57.113 Offline cache update error, URL=http://mixedpuppy.github.io/socialapi-demo/appcache.manifest
> 15:01:57.417 TypeError: hash is null panel.js:17
> 15:01:57.468 TypeError: hash is null panel.js:17 

I can reproduce this on Windows XP and Windows 8, but not on Linux, nor on OSX. Attached is a screenshot of what it looks like on Windows XP.

Comment 1

[u279076]

Nominating for tracking only because this is supposed to ship in Firefox 26.

Comment 2

[bhavana bajaj [:bajaj]]

Tracking as this is a recent regression from 891219 and passing on to you Shane.

Comment 3

[Shane Caraveo (:mixedpuppy)]

Attachment: fix socialmark button icon

This css did not affect osx, but makes the button icon disappear on windows.  Removal should simply allow the default toolbar css rules to apply here.  Works fine on osx and win7.

Comment 4

[:Gijs (he/him)]

Comment on attachment 831163 [details] [diff] [review]
fix socialmark button icon

(In reply to Shane Caraveo (:mixedpuppy) from comment #3)
> Created attachment 831163 [details] [diff] [review]
> fix socialmark button icon
> 
> This css did not affect osx, but makes the button icon disappear on windows.
> Removal should simply allow the default toolbar css rules to apply here. 
> Works fine on osx and win7.

I can't reproduce the issue either before or after this patch, building against current m-c tip, and I don't understand the patch. Surely we should constrain the size of the icon as otherwise if providers ship bigger icons, we would enlarge the toolbar?

Comment 5

[:Gijs (he/him)]

I reproduced this just now... not tried the patch again. Shane, do you know what needs to happen here?

Comment 6

[Shane Caraveo (:mixedpuppy)]

(In reply to :Gijs Kruitbosch from comment #5)
> I reproduced this just now... not tried the patch again. Shane, do you know
> what needs to happen here?

I've had no problem reproducing this on windows and the attached patch fixed it for me pre-australis.  I'm assuming it is some kind of mix of using image.src and css listStyleImage that is doing something weird.  Can you try the patch?

Comment 7

[:Gijs (he/him)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #6)
> (In reply to :Gijs Kruitbosch from comment #5)
> > I reproduced this just now... not tried the patch again. Shane, do you know
> > what needs to happen here?
> 
> I've had no problem reproducing this on windows and the attached patch fixed
> it for me pre-australis.  I'm assuming it is some kind of mix of using
> image.src and css listStyleImage that is doing something weird.  Can you try
> the patch?

Well, the problem is, I just restarted my build again and now it works. I'd not applied your patch yet, so somehow this is intermittent, at least for me. :-\

Comment 8

[Shane Caraveo (:mixedpuppy)]

this is fixed on fx-team for 28, but still needs this fix for 27.  The fixes for australis addressed this problem by using the australis customization widgets.

Comment 9

[Lukas Blakk [:lsblakk] use ?needinfo]

Doesn't appear to be concerning that this is affecting FF26 and we're one beta from shipping so this is a wontfix there. Please get in touch if that's a surprise or a concern.

Comment 10

[Shane Caraveo (:mixedpuppy)]

Attachment: fix icon and panel anchor

This isn't intermittent, it's 100% on all versions for windows, need to fix all the way up to beta.

Comment 11

[Shane Caraveo (:mixedpuppy)]

@gijs: I cannot think of any reason why windows should be different here, but it is.  I've looked at nightly/aurora/beta on osx/lin/win, only windows has a missing icon, which is fixed by the css change in attachment 8356263 [details] [diff] [review].  If you have any thoughts on that let me know, but I need to get a fix in and uplifted.  That css addition overrides css for the button found at http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.css#741

Comment 12

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8356263 [details] [diff] [review]
fix icon and panel anchor

markh or felipe

felipe, if you have any thoughts around my info request for gijs, that would be cool.

Comment 14

[:Gijs (he/him)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #11)
> @gijs: I cannot think of any reason why windows should be different here,
> but it is.  I've looked at nightly/aurora/beta on osx/lin/win, only windows
> has a missing icon, which is fixed by the css change in attachment 8356263 [details] [diff] [review]
> [details] [diff] [review].  If you have any thoughts on that let me know,
> but I need to get a fix in and uplifted.  That css addition overrides css
> for the button found at
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.
> css#741

This still doesn't make sense to me. In principle, the CSS looks like it should only alter the size of the icon that's being displayed. I don't understand how changing that maximum size could make the icon disappear completely, rather than being displayed at the wrong size. I'm also still confused about not being able to reproduce consistently. I'll give it another shot tomorrow.

Comment 15

[Shane Caraveo (:mixedpuppy)]

(In reply to :Gijs Kruitbosch from comment #14)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #11)

> This still doesn't make sense to me. In principle, the CSS looks like it
> should only alter the size of the icon that's being displayed. I don't
> understand how changing that maximum size could make the icon disappear
> completely, rather than being displayed at the wrong size. I'm also still
> confused about not being able to reproduce consistently. I'll give it
> another shot tomorrow.

It is odd that you cannot get it consistently, since it is 100% consistent for me from fx 26 through australis.  I'm guessing that you may not be looking at the correct button, since it is not visible unless you hover over it.  If you have the demo provider installed, and you inspect dom, you should see a button in the toolbar with an id that looks something like "social-marks-button-http://mixedpuppy.github.io/socialapi-demo".

Comment 16

[:Gijs (he/him)]

Comment on attachment 8356263 [details] [diff] [review]
fix icon and panel anchor

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

For whatever reason, I can reproduce reliably now. I swear something's changed since the last time I poked at this, though - in particular because now the button affects many of the other buttons in the toolbar - the icons go all small (vertically) and look squashed. I poked with DOMI and couldn't find anything to explain this. Anyhow, this patch fixes it.

::: browser/themes/windows/browser.css
@@ +1313,5 @@
> +toolbarbutton[type="socialmark"] > .toolbarbutton-icon {
> +  width: auto;
> +  height: auto;
> +  max-width: 32px;
> +  max-height: 32px;

The icon height in the navbar on Windows is 20px, plus 4px of vertical padding, so I believe this should be 24px.

Comment 17

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8356263 [details] [diff] [review]
fix icon and panel anchor

@Gijs gave r+, no need for more, will update patch per review comment.

Comment 18

[Shane Caraveo (:mixedpuppy)]

Attachment: non-australis patch (holly/aurora/beta)

the panel anchor location only became a problem with australis, removed from this version of the patch.  carry forward r+ on the css change from @Gijs.

Comment 19

[Shane Caraveo (:mixedpuppy)]

https://hg.mozilla.org/integration/fx-team/rev/88ddf7ca1704

http://hg.mozilla.org/projects/holly/rev/3c6fb9aabc3e

Comment 20

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8358061 [details] [diff] [review]
non-australis patch (holly/aurora/beta)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): socialmarks
User impact if declined: mark button will be invisible as the icon does not appear
Testing completed (on m-c, etc.): holly/australis
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/88ddf7ca1704

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6c1254f3ebff
https://hg.mozilla.org/releases/mozilla-beta/rev/e8465ab9a529

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/releases/mozilla-aurora/rev/6c1254f3ebff along with the other three commits from Ryan's uplift because one of the four caused windows m5 test failures: https://tbpl.mozilla.org/php/getParsedLog.php?id=32842361&tree=Mozilla-Aurora

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(In reply to Wes Kocher (:KWierso) from comment #23)
> Backed out in
> https://hg.mozilla.org/releases/mozilla-aurora/rev/6c1254f3ebff along with
> the other three commits from Ryan's uplift because one of the four caused
> windows m5 test failures:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=32842361&tree=Mozilla-Aurora

Looks like the patch from this bug that was uplifted to beta was fine, so I'd guess this one's not the cause of the m5 failures.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

This wasn't at fault.
https://hg.mozilla.org/releases/mozilla-aurora/rev/4b9ab5004667

Comment 26

[svbabukumar]

[bugday-20140122] - works fine in the Win 7 x64 - Nightly Build 29.0a1 (2014-01-21)

Comment 27

[u279076]

(In reply to svbabukumar from comment #26)
> [bugday-20140122] - works fine in the Win 7 x64 - Nightly Build 29.0a1
> (2014-01-21)

Thanks! Could you please also test the latest Aurora (aurora.mozilla.org) and Beta (beta.mozilla.org)?

Comment 28

[svbabukumar]

[bugday-20140122]  - Works fine in Aurora 28.0a2 (2014-01-22) and 27 Beta

Comment 29

[u279076]

Thanks!