Closed Bug 935773 Opened 6 years ago Closed 6 years ago

Social Bookmark button icon doesn't show on Windows

Categories

(Firefox Graveyard :: SocialAPI, defect)

defect
Not set

Tracking

(firefox25 unaffected, firefox26+ wontfix, firefox27+ verified, firefox28+ verified, firefox29 verified)

VERIFIED FIXED
Firefox 29
Tracking Status
firefox25 --- unaffected
firefox26 + wontfix
firefox27 + verified
firefox28 + verified
firefox29 --- verified

People

(Reporter: ashughes, Assigned: mixedpuppy)

References

Details

(Keywords: regression, Whiteboard: [Australis:P-][good first verify])

Attachments

(3 files, 1 obsolete file)

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.
Blocks: 891219
Nominating for tracking only because this is supposed to ship in Firefox 26.
Tracking as this is a recent regression from 891219 and passing on to you Shane.
Assignee: nobody → mixedpuppy
Keywords: regression
Attached patch fix socialmark button icon (obsolete) — Splinter Review
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.
Attachment #831163 - Flags: review?(gijskruitbosch+bugs)
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?
Attachment #831163 - Flags: review?(gijskruitbosch+bugs)
Whiteboard: [Australis:P-]
I reproduced this just now... not tried the patch again. Shane, do you know what needs to happen here?
Flags: needinfo?(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?
Flags: needinfo?(mixedpuppy)
(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. :-\
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.
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.
This isn't intermittent, it's 100% on all versions for windows, need to fix all the way up to beta.
Attachment #831163 - Attachment is obsolete: true
Attachment #8356263 - Flags: review?(mhammond)
@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
Flags: needinfo?(gijskruitbosch+bugs)
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.
Attachment #8356263 - Flags: review?(felipc)
Duplicate of this bug: 956918
(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.
(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 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.
Attachment #8356263 - Flags: review+
Flags: needinfo?(gijskruitbosch+bugs)
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.
Attachment #8356263 - Flags: review?(mhammond)
Attachment #8356263 - Flags: review?(felipc)
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.
Attachment #8358061 - Flags: review+
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
Attachment #8358061 - Flags: approval-mozilla-beta?
Attachment #8358061 - Flags: approval-mozilla-aurora?
Attachment #8358061 - Flags: approval-mozilla-beta?
Attachment #8358061 - Flags: approval-mozilla-beta+
Attachment #8358061 - Flags: approval-mozilla-aurora?
Attachment #8358061 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/88ddf7ca1704
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
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
(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.
Whiteboard: [Australis:P-] → [Australis:P-][good first verify]
[bugday-20140122] - works fine in the Win 7 x64 - Nightly Build 29.0a1 (2014-01-21)
(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)?
Status: RESOLVED → VERIFIED
[bugday-20140122]  - Works fine in Aurora 28.0a2 (2014-01-22) and 27 Beta
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.