Closed
Bug 855389
Opened 9 years ago
Closed 9 years ago
toolbarbutton structure causing various issues
Categories
(Firefox Graveyard :: SocialAPI, defect)
Tracking
(firefox22 verified, firefox23 verified)
VERIFIED
FIXED
Firefox 23
People
(Reporter: mixedpuppy, Assigned: mixedpuppy)
References
Details
Attachments
(1 file, 3 obsolete files)
20.51 KB,
patch
|
mixedpuppy
:
review+
akeybl
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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•9 years ago
|
||
Really better to track these separate (related) issues using the dependency list, rather than duping them all.
Assignee | ||
Comment 5•9 years ago
|
||
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)
Assignee | ||
Comment 6•9 years ago
|
||
fixes appearance on linux
Attachment #730337 -
Attachment is obsolete: true
Attachment #730337 -
Flags: feedback?(jAwS)
Attachment #730418 -
Flags: feedback?(jAwS)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → mixedpuppy
Assignee | ||
Comment 7•9 years ago
|
||
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)
Comment 8•9 years ago
|
||
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+
Assignee | ||
Comment 9•9 years ago
|
||
(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.
Assignee | ||
Comment 10•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/314ce3068d94
Comment 11•9 years ago
|
||
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
Assignee | ||
Comment 12•9 years ago
|
||
fixes tests, carry forward r+ https://tbpl.mozilla.org/?tree=Try&rev=3e123ba989fd
Attachment #732573 -
Attachment is obsolete: true
Attachment #734493 -
Flags: review+
Assignee | ||
Comment 13•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/7684baafb4e3
Comment 14•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/7684baafb4e3
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
Assignee | ||
Comment 15•9 years ago
|
||
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?
Assignee | ||
Updated•9 years ago
|
status-firefox22:
--- → affected
status-firefox23:
--- → fixed
Comment 16•9 years ago
|
||
(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?
Assignee | ||
Comment 17•9 years ago
|
||
(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.
Comment 18•9 years ago
|
||
The button-too-wide issue was introduced in bug 811835, as I commented in https://bugzilla.mozilla.org/show_bug.cgi?id=811835#c12.
Updated•9 years ago
|
Comment 19•9 years ago
|
||
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+
Comment 21•9 years ago
|
||
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.
Assignee | ||
Comment 22•9 years ago
|
||
(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.
Assignee | ||
Comment 23•9 years ago
|
||
(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 ;)
Updated•9 years ago
|
Comment 24•9 years ago
|
||
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•9 years ago
|
||
Verified fixed in Firefox 22.0a2 2013-04-12.
Updated•3 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•