855389 - toolbarbutton structure causing various issues

Status: VERIFIED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[Shane Caraveo (:mixedpuppy)]

Our toolbarbutton is structured like:

toolbaritem(group)->toolbaritem(container)->toolbarbutton

The container acts like a button, but does not cause the panel to appear.  As well, themes, dragging, etc are all affected.

The structure was created to deal with the badges we use on the buttons, but that should move into a xbl component allowing us to get rid of the middle layer.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Really better to track these separate (related) issues using the dependency list, rather than duping them all.

Comment 5

[Shane Caraveo (:mixedpuppy)]

Attachment: buttonfix

This patch fixes all the dependency bugs on osx.  I have not tested on win/lin yet.  The toolbar xbl could go into toolkit, or a different file, I just went for simplicity right now until we validate win/lin, and someone has an opinion on whether it belongs in browser or toolkit.

Comment 6

[Shane Caraveo (:mixedpuppy)]

Attachment: buttonfix

fixes appearance on linux

Comment 7

[Shane Caraveo (:mixedpuppy)]

Attachment: buttonfix

I've got this working on win/lin/osx in small and large button formats, with or without themes.  Since jaws is out, asking felipe for review.

Comment 8

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 732573 [details] [diff] [review]
buttonfix

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

Looks good. Did you also try moving the buttons to the tab bar to see if it works well?
The new binding sounds useful for other use cases too, shouldn't it live in toolbarbutton.xml instead of urlbarBindings.xml? And then the part from browser/base/content/browser.css would also need to go to toolkit.  Could be left for a follow-up too as I'm not sure how much other non social-specific CSS would need to be created to properly support it.

Comment 9

[Shane Caraveo (:mixedpuppy)]

(In reply to :Felipe Gomes from comment #8)
> Comment on attachment 732573 [details] [diff] [review]
> buttonfix
> 
> Review of attachment 732573 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good. Did you also try moving the buttons to the tab bar to see if it
> works well?

I have not made the button movable yet, we still need to figure out some ux issues around that.

> The new binding sounds useful for other use cases too, shouldn't it live in
> toolbarbutton.xml instead of urlbarBindings.xml? And then the part from
> browser/base/content/browser.css would also need to go to toolkit.  Could be
> left for a follow-up too as I'm not sure how much other non social-specific
> CSS would need to be created to properly support it.

Lets get it in now, do it as a follow up.

Comment 10

[Shane Caraveo (:mixedpuppy)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/314ce3068d94

Comment 11

[Phil Ringnalda (:philor)]

browser-chrome says

22:21:18 WARNING - TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/social/browser_social_mozSocial_API.js | Status icon didn't become non-hidden
22:21:18 WARNING - TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/social/browser_social_mozSocial_API.js | uncaught exception - TypeError: aTarget is null at chrome://mochikit/content/tests/SimpleTest/EventUtils.js:287
22:21:45 WARNING - TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/social/browser_social_mozSocial_API.js | Test timed out
22:21:52 WARNING - TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/social/browser_social_toolbar.js | statusIcon was never found
22:21:52 WARNING - TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/social/browser_social_toolbar.js | uncaught exception - TypeError: statusIcon is null at chrome://mochitests/content/browser/browser/base/content/test/social/browser_social_toolbar.js:111
22:22:19 WARNING - TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/social/browser_social_toolbar.js | Test timed out

Backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/de4f34069146

Comment 12

[Shane Caraveo (:mixedpuppy)]

Attachment: buttonfix

fixes tests, carry forward r+

https://tbpl.mozilla.org/?tree=Try&rev=3e123ba989fd

Comment 13

[Shane Caraveo (:mixedpuppy)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7684baafb4e3

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7684baafb4e3

Comment 15

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 734493 [details] [diff] [review]
buttonfix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): various button visual breakage (themes, small icon, wide buttons, etc), no functional breakage
User impact if declined: poor visual experience 
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): low, changes in browser xul specific to social.  alternative is to skip it for 22.
String or IDL/UUID changes made by this patch: none

I wouldn't say this bug is absolutely necessary, but does provide a more polished appearance, so I'm going to ask for uplift to aurora.

Comment 16

[:Gavin Sharp [email: [e-mail]]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #15)
> Bug caused by (feature/regressing bug #): various button visual breakage
> (themes, small icon, wide buttons, etc), no functional breakage

What change caused this bug (or, I suppose, the various bugs this blocks), and in which version was it introduced? Has this been a problem since we originally implemented these buttons, or did we somehow make them worse at some point? Were these problems caused by bug 811835?

Comment 17

[Shane Caraveo (:mixedpuppy)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #16)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #15)
> > Bug caused by (feature/regressing bug #): various button visual breakage
> > (themes, small icon, wide buttons, etc), no functional breakage
> 
> What change caused this bug (or, I suppose, the various bugs this blocks),
> and in which version was it introduced? Has this been a problem since we
> originally implemented these buttons, or did we somehow make them worse at
> some point? Were these problems caused by bug 811835?

I would imagine that some of the issues (at least themes) are from the start.  The buttons too wide issue, and probably the click/drag, certainly started after 811835.  I think this gets the button structure/layout issues settled.

Comment 18

[Jared Wein [:jaws] (please needinfo? me)]

The button-too-wide issue was introduced in bug 811835, as I commented in https://bugzilla.mozilla.org/show_bug.cgi?id=811835#c12.

Comment 19

[Alex Keybl [:akeybl]]

Comment on attachment 734493 [details] [diff] [review]
buttonfix

Definitely OK for Aurora given where we are in the cycle and the amount/risk of code change this patch represents. Doesn't seem critical enough to track/uplift for 21.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/df268e1e0987

Comment 21

[u279076]

The following bugs have been verified fixed in Firefox Nightly 23.0a1 2013-04-11
> Bug 846893 - SocialAPI icons broke "Use Small Icons" feature 
> Bug 829868 - The icons in the Facebook toolbar are wider than before
> Bug 854497 - Toolbar button appears wrong with themes

However, I found the following bug may not be 100% fixed:
> Bug 806696 - Clicks around edge of jewel buttons drag the window

Note that all above verifications will be completed for Firefox 22 tomorrow pending Aurora builds.

Comment 22

[Shane Caraveo (:mixedpuppy)]

(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #21)
> The following bugs have been verified fixed in Firefox Nightly 23.0a1
> 2013-04-11
> > Bug 846893 - SocialAPI icons broke "Use Small Icons" feature 
> > Bug 829868 - The icons in the Facebook toolbar are wider than before
> > Bug 854497 - Toolbar button appears wrong with themes
> 
> However, I found the following bug may not be 100% fixed:
> > Bug 806696 - Clicks around edge of jewel buttons drag the window
> 
> Note that all above verifications will be completed for Firefox 22 tomorrow
> pending Aurora builds.

I can do the same thing on other toolbar buttons.

Comment 23

[Shane Caraveo (:mixedpuppy)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #22)
> (In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #21)
> > However, I found the following bug may not be 100% fixed:
> > > Bug 806696 - Clicks around edge of jewel buttons drag the window

> I can do the same thing on other toolbar buttons.

to be more specific, I can grab a button 1px inside the border and drag, sometimes the button will depress, sometimes not.  Seems like an "edge" case ;)

Comment 24

[u279076]

Thanks Shane. I can confirm that as well. I update bug 806696 with that information. All that's left now is to verify these bugs with tomorrow's Aurora build.

Comment 25

[u279076]

Verified fixed in Firefox 22.0a2 2013-04-12.