Closed
Bug 811514
Opened 12 years ago
Closed 11 years ago
social panels shouldn't consume outside clicks
Categories
(Firefox Graveyard :: SocialAPI, defect)
Firefox Graveyard
SocialAPI
Tracking
(Not tracked)
VERIFIED
FIXED
Firefox 21
People
(Reporter: marco, Assigned: marco)
References
Details
Attachments
(1 file, 1 obsolete file)
1.96 KB,
patch
|
jaws
:
review+
|
Details | Diff | Splinter Review |
In my opinion the social panels should be "transparent to focus". I mean, if a social panel is opened and you click on a field, this field should be selected, and not only the social panel closed.
Comment 1•12 years ago
|
||
Marco, can you use DOM Inspector and remove the consumeoutsideclicks attribute on the panel and see if it provides the experience you are looking for? We would need to support the case of a user clicking on the same notification icon so that it won't close-then-reopen immediately.
Assignee | ||
Comment 2•12 years ago
|
||
(In reply to Jared Wein [:jaws] from comment #1) > Marco, can you use DOM Inspector and remove the consumeoutsideclicks > attribute on the panel and see if it provides the experience you are looking > for? Yes, that's exactly what I wanted :) > We would need to support the case of a user clicking on the same > notification icon so that it won't close-then-reopen immediately. True.
Comment 3•12 years ago
|
||
Would you like to provide a patch to fix this? The code for this is here: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-social.js#831 While you're in there, you could also change the event handler from "mousedown" to "click" (bug 807716).
Updated•12 years ago
|
Summary: Better focus behaviour for social panels → social panels shouldn't consume outside clicks
Comment 4•12 years ago
|
||
Bug 789859 is where we introduced this behavior, to fix the issue Jared mentioned about clicking the button twice.
Comment 5•12 years ago
|
||
This sounds like a panel bug in that there is no standard panel behaviour we can specify to get this - eg, the new downloads panel has the same issue and it doesn't make sense for every panel user to come up with their own solution.
OS: Linux → All
Hardware: x86_64 → All
Comment 6•12 years ago
|
||
What do you mean? consumeoutsideclicks="false" and a solution to ignore clicks on the button when the panel state is "open" or "closing" should work?
Comment 7•12 years ago
|
||
It should work, yeah. It just seems unfortunate that toggling consumeoutsideclicks back in bug 789859 was explicitly about the double-show behaviour but has this focus side-effect. Now we want to toggle it back because of the focus and work-around the double-show side-effect. In both cases the general functionality of the panel is fine, it's all about the side-effects, so some way to spell "no side-effects" seems desirable.
Assignee | ||
Comment 8•12 years ago
|
||
(In reply to Mark Hammond (:markh) from comment #5) > This sounds like a panel bug in that there is no standard panel behaviour we > can specify to get this - eg, the new downloads panel has the same issue and > it doesn't make sense for every panel user to come up with their own > solution. Any pointer for the code about the standard panel behaviour?
Comment 9•12 years ago
|
||
(In reply to Marco Castelluccio [:marco] from comment #8) > Any pointer for the code about the standard panel behaviour? There is no standard behaviour that fixes this, and I also understand why it is almost impossible to do, unless we create a new kind of widget that also takes over responsibility for the click on the anchor. I was really just babbling on :) As Gavin said, the fix is simply to have the anchor click event only open the popup when the current state == "closed".
Assignee | ||
Comment 10•11 years ago
|
||
Assignee: nobody → mar.castelluccio
Status: NEW → ASSIGNED
Attachment #709927 -
Flags: review?(weinjared+bmo)
Updated•11 years ago
|
Attachment #709927 -
Flags: review?(weinjared+bmo) → review?(jaws)
Comment 11•11 years ago
|
||
Comment on attachment 709927 [details] [diff] [review] Patch drive by. > toolbarButton.addEventListener("mousedown", function (event) { >- if (event.button == 0) >+ let panel = document.getElementById("social-notification-panel"); >+ if (event.button == 0 && !panel.hasAttribute("panelopen")) > SocialToolbar.showAmbientPopup(toolbarButton); > }); panel.state != "open" might be better than the hasAttr check.
Comment 12•11 years ago
|
||
Comment on attachment 709927 [details] [diff] [review] Patch Review of attachment 709927 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/browser-social.js @@ +894,5 @@ > toolbarButton.classList.add("toolbarbutton-1"); > toolbarButton.setAttribute("id", toolbarButtonId); > toolbarButton.setAttribute("notificationFrameId", notificationFrameId); > toolbarButton.addEventListener("mousedown", function (event) { > + let panel = document.getElementById("social-notification-panel"); panel is already defined at the beginning of this function. @@ +895,5 @@ > toolbarButton.setAttribute("id", toolbarButtonId); > toolbarButton.setAttribute("notificationFrameId", notificationFrameId); > toolbarButton.addEventListener("mousedown", function (event) { > + let panel = document.getElementById("social-notification-panel"); > + if (event.button == 0 && !panel.hasAttribute("panelopen")) I agree with Shane, but I would prefer: > if (event.button == 0 && panel.state == "closed") In my testing, I couldn't hit this code when the panel had the "panelopen" attribute. ::: browser/base/content/browser.xul @@ +264,5 @@ > <panel id="social-notification-panel" > class="social-panel" > type="arrow" > hidden="true" > + consumeoutsideclicks="false" This attribute can be removed.
Attachment #709927 -
Flags: review?(jaws) → feedback+
Assignee | ||
Comment 13•11 years ago
|
||
> ::: browser/base/content/browser.xul
> @@ +264,5 @@
> > <panel id="social-notification-panel"
> > class="social-panel"
> > type="arrow"
> > hidden="true"
> > + consumeoutsideclicks="false"
>
> This attribute can be removed.
Without this attribute, outside clicks are consumed.
Attachment #709927 -
Attachment is obsolete: true
Attachment #711134 -
Flags: review?(jaws)
Comment 14•11 years ago
|
||
Comment on attachment 711134 [details] [diff] [review] Patch v2 Review of attachment 711134 [details] [diff] [review]: ----------------------------------------------------------------- Sorry, I had a misunderstanding of the default value for consumeoutsideclicks.
Attachment #711134 -
Flags: review?(jaws) → review+
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 15•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e6c148b9c35
Keywords: checkin-needed
Comment 16•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/6e6c148b9c35
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 21
Comment 17•11 years ago
|
||
So yeah, bug 789859 explicitly added consumeoutsideclicks to avoid a problem, this bug removed it to prevent some other problem, so bug 840854 gets opened to re-report the original problem. Hopefully we can work out how to break this cycle ;)
Comment 18•11 years ago
|
||
Marking this bug verified fixed since I cannot reproduce in the latest Firefox 21b6 build and bug 840854 has been verified fixed.
Status: RESOLVED → VERIFIED
Updated•5 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•