Closed Bug 596723 Opened 10 years ago Closed 7 years ago

Don't consume clicks outside of arrow panels by default

Categories

(Toolkit :: XUL Widgets, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla28
Tracking Status
blocking2.0 --- .x+

People

(Reporter: mossop, Assigned: enndeakin)

References

(Blocks 1 open bug)

Details

(Keywords: ux-mode-error, ux-userfeedback, Whiteboard: [softblocker][doorhanger])

Attachments

(1 file, 6 obsolete files)

If I have a doorhander open and I ignore it I still want to be able to interact with the webpage. If I click a link though nothing happens except the popup gets dismissed.

We should use ROLLUP_NO_CONSUME for the popup panels.
blocking2.0: --- → ?
blocking2.0: ? → final+
Duplicate of this bug: 598953
Assignee: nobody → gavin.sharp
This works for me right now. Was this maybe fixed by arrow panels landing?
I wouldn't expect arrow panels to behave any differently in this regard. Maybe bug 600406 is affecting your testing? I can still see this in a recent build.
Duplicate of this bug: 617305
Whiteboard: [soft blocker]
Whiteboard: [soft blocker] → [softblocker]
Whiteboard: [softblocker] → [softblocker][doorhanger]
Duplicate of this bug: 638066
** PRODUCT DRIVERS PLEASE NOTE **

This bug is one of 7 automatically changed from blocking2.0:final+ to blocking2.0:.x during the endgame of Firefox 4 for the following reasons:

 - it was marked as a soft blocking issue without a requirement for beta coverage
blocking2.0: final+ → .x+
I have a client who was confused by this and brought it up to me as an issue. It affects interaction on my single page application in a pretty annoying way. Seeing this fixed in the near future would make me a happy man.
I think the main problem is that we still show the hover states for the content area, including links, so we are providing misleading feedback to the user.
Assignee: gavin.sharp → nobody
Bug 600406 made the simple fix ineffective when I was testing it back around comment 2 (pass ROLLUP_NO_CONSUME to showPopup).
Depends on: 600406
I tested the other day setting ROLLUP_NO_CONSUME just before this.panel.openPopup(..) in PopupNotifications_showPanel and it seemed to work consistently for me with geolocation but not with password manager (unless you close and reopen the doorhanger).  I wonder if it has something to do with persistence or the timer/delay on those or something else related to the page-change when submitting a form.

Try builds: https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mozilla@noorenberghe.ca-b9cb09a2addb/

I can take a look at this more after some higher priority stuff.
Summary: Events that dismiss popup notifications should still be dispatched to the UI → events that dismiss popup notifications (clicks) should still be dispatched to the UI
Note that scrolls are also not dispatched for no apparent reason.

All in all doorhangers are an abysmal UI.

If somebody wants to see how to NOT do an UI behold doorhangers.
Duplicate of this bug: 830559
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → fyan
Attachment #604540 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #722571 - Flags: review?(gavin.sharp)
Attachment #722571 - Flags: feedback?(mnoorenberghe+bmo)
Comment on attachment 722571 [details] [diff] [review]
patch

This is great; so many clicks saved for so many people. A no-brainer from the UX side of things. :)
Attachment #722571 - Flags: ui-review+
Comment on attachment 722571 [details] [diff] [review]
patch

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

Please correct the commit message to reflect the fact that you are now making the change for all usages of panels in our UI and not just popup notifications.

::: browser/base/content/browser.xul
@@ -273,5 @@
>             side="right"
>             type="arrow"
>             hidden="true"
>             rolluponmousewheel="true"
> -           consumeoutsideclicks="false"

My understanding is that false is not the default value in this case so removing this attribute makes content clicks get consumed by the panel. https://mxr.mozilla.org/mozilla-central/source/layout/xul/base/src/nsMenuPopupFrame.h#121 has a table of defaults by platform. Please revert this since my quick test confirms that this changes the behaviour.
Attachment #722571 - Flags: feedback?(mnoorenberghe+bmo) → feedback-
I confirmed that the problem I was having with password manager notifications wasn't an issue with this patch (assuming I wasn't missing some detail).

Thanks for picking this up, it was high on my list of things to fix.
Summary: events that dismiss popup notifications (clicks) should still be dispatched to the UI → panels should not consume outside clicks by default
(In reply to Matthew N. [:MattN] from comment #15)
> My understanding is that false is not the default value in this case

That's why I put consumeoutsideclicks="false" on the popup.xml#arrowpanel binding <content>. Did you test with the entire patch or just DOM Inspector?

I can add a `if (this.getAttribute("consumeoutsideclicks") == "false") return;` guard above the block I added to the popuphiding listener.
Summary: panels should not consume outside clicks by default → Don't consume clicks outside of arrow panels by default
Attachment #722571 - Flags: feedback- → feedback?
The order of events when click on the panel's anchor itself to dismiss the panel is popuphiding, popuphidden, mousedown, MozAfterPaint. The mousedown event being fired after popuphidden makes it tricky to suppress any immediately subsequent click on the anchor, hence the addition of code to the popuphiding listener.
Comment on attachment 722571 [details] [diff] [review]
patch

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

Sorry, I somehow missed the popup.xml changes but noticed right after I submitted.

Could you add a test for the code to prevent reopening the panel when clicking its anchor to close it? It feels a bit dirty and prone to breakage with popup/panel changes.

::: toolkit/content/widgets/popup.xml
@@ +472,5 @@
>        <handler event="popuphiding" phase="target">
>          clearTimeout(this._fadeTimer);
>          this.style.removeProperty("opacity");
> +
> +        // prevent reopening the panel when clicking its anchor to close it

Could you only run the code below when the panel is not consuming outside clicks to avoid setting up and tearing down event listeners which aren't necessary? Since this addition is for a single purpose, it would be clearer to move it to it's own helper function (e.g. _preventClosingOnAnchorClick) IMO.

@@ +476,5 @@
> +        // prevent reopening the panel when clicking its anchor to close it
> +        let anchor = this.anchorNode;
> +        if (!anchor)
> +          return;
> +        let parent = anchor.parentNode;

Nit: How about anchorParent for a variable name? By default, I would assume a variable named |parent| would be for the parent of |this| which is not the case here.

@@ +479,5 @@
> +          return;
> +        let parent = anchor.parentNode;
> +        if (!parent)
> +          return;
> +        function capture() {

Nit: How about onAnchorParentMouseDown?

@@ +481,5 @@
> +        if (!parent)
> +          return;
> +        function capture() {
> +          this.removeEventListener("mousedown", capture, true);
> +          document.documentElement.setCapture(true);

Calling this on the documentElement seems a bit random.  How about calling setCapture on the popup instead?

@@ +486,5 @@
> +        }
> +        parent.addEventListener("mousedown", capture, true);
> +        window.addEventListener("MozAfterPaint", function expire() {
> +          this.removeEventListener("MozAfterPaint", expire, true);
> +          parent.removeEventListener("mousedown", capture, true);

Add a comment here about the event order and more details about what is going on.
Attachment #722571 - Flags: feedback?
Attached patch patch v2 (obsolete) — Splinter Review
Addressed MattN's feedback comments.
Attachment #722571 - Attachment is obsolete: true
Attachment #722571 - Flags: review?(gavin.sharp)
Attachment #725148 - Flags: ui-review+
Attachment #725148 - Flags: review?(gavin.sharp)
_preventReopeningOnAnchorClick could use a few more line breaks in it. I'll fix that locally.
Attachment #725148 - Flags: review?(dolske)
Comment on attachment 725148 [details] [diff] [review]
patch v2

The _preventReopeningOnAnchorClick() function is kind of gross/hacky, but I don't really see a better alternative. There's a wide separation down in widget land between closing the popup and dispatching the click to the window underneath. So I don't see a practical way to carry some info around (perhaps in the click event itself) so that the normal anchor-icon listener can exactly decide if the click closed the popup (and if so ignore it). 

The only other idea that comes to mind is for |popuphiding| to stash a timestamp on the anchor node, and have the anchor's listener ignore clicks if they occur < X ms since the popup last closed. But that seems worse, because jank at the wrong moment could make the timeout sometimes be exceed and the popup would reopen.

Can we add a test here? If the event ordering this depends on should ever change it would be nice to know right away. r+ with a test. :)
Attachment #725148 - Flags: review?(gavin.sharp)
Attachment #725148 - Flags: review?(dolske)
Attachment #725148 - Flags: review+
One fix I think might to pass the mouse event coordinates to nsXULPopupManager::Rollup, use them check if they are over the anchor, and treat that as a consumed case. That probably isn't too difficult.

Did you test your patch when someone cancels the popuphiding event?
(In reply to Neil Deakin from comment #23)
> One fix I think might to pass the mouse event coordinates to
> nsXULPopupManager::Rollup, use them check if they are over the anchor, and
> treat that as a consumed case. That probably isn't too difficult.

That would probably be cleaner, but I'm not familiar enough with the popup manager code to confidently write a patch for it. If you think it's a feasible and better approach, could you put together a patch? I'm indifferent to the approach; I just want this bug to be fixed.

I did look through the rollup code, and it seems that we already have code to prevent the event that rolls up the panel from reopening the panel, but that code is ineffective in our case, because the panel is not being reopened by the same event that closed it.

> Did you test your patch when someone cancels the popuphiding event?

If, by "cancel", you mean event.preventDefault(), I've tested it, and it's not a problem.
Since the Australis menu is gonna be a panel, it would be nice to have this land at the same time.
Attached patch patch v2 unbitrotted (obsolete) — Splinter Review
I'm never going to get around to writing a test for this.
If people are okay with landing this patch as-is (which returns all green locally and on Try), go for it.
Attachment #725148 - Attachment is obsolete: true
Attachment #766108 - Flags: ui-review+
Attachment #766108 - Flags: review+
Assignee: fyan → nobody
Status: ASSIGNED → NEW
(In reply to Frank Yan (:fryn) from comment #24)
> (In reply to Neil Deakin from comment #23)
> > One fix I think might to pass the mouse event coordinates to
> > nsXULPopupManager::Rollup, use them check if they are over the anchor, and
> > treat that as a consumed case. That probably isn't too difficult.
> 
> That would probably be cleaner, but I'm not familiar enough with the popup
> manager code to confidently write a patch for it. If you think it's a
> feasible and better approach, could you put together a patch?

Enn, would you mind taking this bug?
Flags: needinfo?(enndeakin)
Attached patch Patch (obsolete) — Splinter Review
This is a simple patch that implements what I suggested above. I disabled consumeonclicks on all of the arrow panels so that the effect can be more easily seen. I'm not sure which panels are desired here.

A further improvement is to be more precise about determining whether the mouse is over the anchor.

It might also be useful to have consumeoutsideclicks="parentonly" to enable this behaviour specifically without affecting existing behaviour.
Assignee: nobody → enndeakin
Status: NEW → ASSIGNED
Flags: needinfo?(enndeakin)
Attachment #766108 - Attachment is obsolete: true
This patch makes it so arrowpanels are consumeoutsideclicks="false" by default. Also, clicking on the parent anchor is always ignored no matter what value of consumeoutsideclicks is used.
Attachment #767899 - Attachment is obsolete: true
Attachment #815945 - Flags: review?(neil)
Attachment #815945 - Flags: review?(dao)
hello,

is there an easy way to get a build of browser with this patch applied?

Or will this patch make it possible to interact with applications other than Firefox while a doorhanger is open or will Firefox still grab the X server?
Attachment #815945 - Flags: review?(dao) → review+
It appears the patch fixes the annoyance that you have to double-click links in pages when a doorhanger is open.

It however does not solve the problem that the X server is grabbed and hence you cannot interact with *other* applications.
Comment on attachment 815945 [details] [diff] [review]
Don't consume on arrowpanels, always consume on the anchor

>+      nsIntPoint pos(-1, -1);
A cursor position of (-1, -1) is legal on multiple monitor systems. Unless we already have existing code that makes assumptions about a cursor position of (-1, -1) I suggest you use a different way of making the position optional, such as passing a pointer instead of a reference.

>+      if (inMsg == WM_LBUTTONDOWN || inMsg == WM_MOUSEACTIVATE) {
inMsg cannot be WM_MOUSEACTIVATE at this point.

>+        POINT point;
>+        ::GetCursorPos(&point);
>+        pos.x = point.x;
>+        pos.y = point.y;
For WM_LBUTTONDOWN prefer to use GET_X/Y_LPARAM to extract the mouse position from the message.
Attachment #815945 - Attachment is obsolete: true
Attachment #815945 - Flags: review?(neil)
Attachment #822312 - Flags: review?(neil)
Comment on attachment 822312 [details] [diff] [review]
Don't consume on arrowpanels, always consume on the anchor, v2

>+        if (rollup && rollupListener->Rollup(popupsToRollup, usePoint ? &point : NULL, nullptr)) {
NULL?
[The ? : looks ugly to me here but you're removing one in the windows widget code so that makes up for it]

>+       ::ClientToScreen(inWnd, &pt);
Nit: indentation was off on this line.
Attachment #822312 - Flags: review?(neil) → review+
Blocks: 931018
Would be nice to land this ASAP. It would for example make it possible to interact with page content when the Australis menu is open.
(In reply to Guillaume C. [:ge3k0s] from comment #36)
> Would be nice to land this ASAP. It would for example make it possible to
> interact with page content when the Australis menu is open.

I'm fairly sure that we don't want that, and if the default for panels changes, we'd update the Australis menu to close when you click outside of it, like 'regular' menus.
(In reply to :Gijs Kruitbosch from comment #37)
> (In reply to Guillaume C. [:ge3k0s] from comment #36)
> > Would be nice to land this ASAP. It would for example make it possible to
> > interact with page content when the Australis menu is open.
> 
> I'm fairly sure that we don't want that, and if the default for panels
> changes, we'd update the Australis menu to close when you click outside of
> it, like 'regular' menus.

Sorry I wasn't clear. That's exactly what this bug is gonna resolved :)
https://hg.mozilla.org/mozilla-central/rev/07ea7b11adef
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Depends on: 934988
Blocks: 934988
No longer depends on: 934988
Blocks: 950419
Keywords: verifyme
I reproduced the bug using FF 25 and Win 7 x64.

I verified the bug as fixed using the following environment:
FF 28.0b6
Build Id: 20140224220227
Os: Win 7 x 65, Mac Os 10.8.5, Ubuntu 13.04 x64
Status: RESOLVED → VERIFIED
Keywords: verifyme
Depends on: 987230
Depends on: 990846
Depends on: 1089005
You need to log in before you can comment on or make changes to this bug.