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.

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.

** 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

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.

Bug 600406 made the simple fix ineffective when I was testing it back around comment 2 (pass ROLLUP_NO_CONSUME to showPopup).

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.

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.

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. :)

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.

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.

(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.

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.

Addressed MattN's feedback comments.

_preventReopeningOnAnchorClick could use a few more line breaks in it. I'll fix that locally.

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. :)

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.

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.

(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?

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.

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.

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?

There are builds with this patch at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/neil@mozilla.com-8dfae4c2f927/

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.

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.

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 :)

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