Closed Bug 811514 Opened 7 years ago Closed 7 years ago

social panels shouldn't consume outside clicks

Categories

(Firefox Graveyard :: SocialAPI, defect)

defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 21

People

(Reporter: marco, Assigned: marco)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
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.
(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.
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).
Summary: Better focus behaviour for social panels → social panels shouldn't consume outside clicks
Bug 789859 is where we introduced this behavior, to fix the issue Jared mentioned about clicking the button twice.
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
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?
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.
(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?
(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".
Attached patch Patch (obsolete) — Splinter Review
Assignee: nobody → mar.castelluccio
Status: NEW → ASSIGNED
Attachment #709927 - Flags: review?(weinjared+bmo)
Attachment #709927 - Flags: review?(weinjared+bmo) → review?(jaws)
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 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+
Attached patch Patch v2Splinter Review
> ::: 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 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+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6e6c148b9c35
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 21
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 ;)
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
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.