Closed Bug 855389 Opened 9 years ago Closed 9 years ago

toolbarbutton structure causing various issues

Categories

(Firefox Graveyard :: SocialAPI, defect)

x86
macOS
defect
Not set
normal

Tracking

(firefox22 verified, firefox23 verified)

VERIFIED FIXED
Firefox 23
Tracking Status
firefox22 --- verified
firefox23 --- verified

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
Duplicate of this bug: 806696
Duplicate of this bug: 854497
Duplicate of this bug: 829868
Really better to track these separate (related) issues using the dependency list, rather than duping them all.
Blocks: 806696, 829868, 854497
Attached patch buttonfix (obsolete) — Splinter Review
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.
Attachment #730337 - Flags: feedback?(jAwS)
Attached patch buttonfix (obsolete) — Splinter Review
fixes appearance on linux
Attachment #730337 - Attachment is obsolete: true
Attachment #730337 - Flags: feedback?(jAwS)
Attachment #730418 - Flags: feedback?(jAwS)
Assignee: nobody → mixedpuppy
Attached patch buttonfix (obsolete) — Splinter Review
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.
Attachment #730418 - Attachment is obsolete: true
Attachment #730418 - Flags: feedback?(jAwS)
Attachment #732573 - Flags: review?(felipc)
Blocks: 846893
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.
Attachment #732573 - Flags: review?(felipc) → review+
(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.
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
Attached patch buttonfixSplinter Review
fixes tests, carry forward r+

https://tbpl.mozilla.org/?tree=Try&rev=3e123ba989fd
Attachment #732573 - Attachment is obsolete: true
Attachment #734493 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/7684baafb4e3
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
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.
Attachment #734493 - Flags: approval-mozilla-aurora?
(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?
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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.
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 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.
Attachment #734493 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
(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.
(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 ;)
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.
Blocks: 832570
Verified fixed in Firefox 22.0a2 2013-04-12.
Status: RESOLVED → VERIFIED
Keywords: verifyme
Depends on: 862567
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.