401528 - Must click twice to open link if fixed positioned div is removed on onmouseup: clicks pass through

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect, P4)

Description

[Agustín Fernández]

Attachment: testcase

Must click twice to open link if fixed positioned div is removed on onmouseup: clicks pass through:

Open the testcase, click on the right div, then click on the left link: the link doesn't open. It actually clicks through it as if it didn't exist, and the click reaches the window directly.

1. If you change the event from mouseup to click everything is right again.
2. If you change the position fixed of the link to position absolute another bug  happens: the browser starts to select text as if you had left the mouse button down.
3. If you prevent the default action of the mousedown event then the error doesn't happen (the click is received correctly and the selection doesn't start). Note that while this is effective in this testcase in our internal application preventing the default action on mousedown only solved the selection problem without solving the click-through one.

This was extremely hard to reproduce as a testcase, this was hurting our application badly (we have a windowing system and clicks often reached the window behind the one you clicked on). This is not 100% reproduceable in our application although it seems to be in the testcase. This was tested in Linux and Windows in both Firefox 2.0.x and today's: "3.0a9pre" firefox build.

Comment 1

[Agustín Fernández]

I noticed something extra: if you click on the right div, focus on some other application and then go back to firefox you will notice that firefox is actually selecting text, as if you had the left button pressed.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Attachment: shows where the problem is

This is just a quick hack to show where the problem is. 
Need to think about this a bit, but would be great to have this fixed in 1.9/ff3.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Attachment: possible patch

This is not so ugly and should be pretty safe.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

-  // All the events we handle below require a frame.
-  if (!mCurrentTarget) {
-    if (NS_EVENT_NEEDS_FRAME(aEvent)) {
-      NS_ERROR("Null frame for an event that requires a frame");
-      return NS_ERROR_NULL_POINTER;
-    }
+  // Most of the events we handle below require a frame.
+  // Add special cases here.
+  if (!mCurrentTarget &&
+      aEvent->message != NS_MOUSE_BUTTON_UP) {
     return NS_OK;

So we're now returning early for focus events with no frame? Is that change intended?

Would it make more sense to just always drop mouse grabbing on MOUSE_BUTTON_UP in nsViewManager?

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 286666 [details] [diff] [review]
possible patch

>-  // All the events we handle below require a frame.
>-  if (!mCurrentTarget) {
>-    if (NS_EVENT_NEEDS_FRAME(aEvent)) {
>-      NS_ERROR("Null frame for an event that requires a frame");
>-      return NS_ERROR_NULL_POINTER;
>-    }
>+  // Most of the events we handle below require a frame.
>+  // Add special cases here.
>+  if (!mCurrentTarget &&
>+      aEvent->message != NS_MOUSE_BUTTON_UP) {
>     return NS_OK;
>   }

Note the |return NS_OK;|
That is called if (!mCurrentTarget) and !NS_EVENT_NEEDS_FRAME(aEvent)
I think I want to change PostHandle to be called anyway always, even though frame gets deleted.

Hmm, but could I perhaps remove some view handling code in nsFrame+subclasses and stop capturing
mouse event always when mouseup...That is riskier though, I think.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #5)
> even though
even if

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> That is riskier though, I think.

Actually I don't think it's that risky, if anything it will catch other cases where we don't release mouse grabbing appropriately.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 286666 [details] [diff] [review]
possible patch

Will post a new patch

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Attachment: proposed patch

Roc, I'd like have the |viewMan->GrabMouseEvents(nsnull, result);| in ESM, mainly because of problematic cases like Bug 373344.
Handling similar situation in ViewManager::HandleEvent would mean copying patch
from Bug 373344 to viewmanager.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't get it. You can hold a strong reference to the viewmanager (in fact, it's HandleEvent already does) so it doesn't matter if a frame or view goes away.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

HandleEvent keeps strong pointer to presshell, not to viewmanager
(though maybe the caller keeps strong pointer to viewmanager).
But do you want GrabMouseEvents(nsnull, result); to be called, event if
MouseEventGrabber != view ?

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Sorry, yes.

> But do you want GrabMouseEvents(nsnull, result); to be called, event if
> MouseEventGrabber != view ?

Yes.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Attachment: mouse_up handling (also) in viewmanager

I prefer the previous version.
Here I add mouse_up handling to viewmanager, but some of the handling is still in 
ESM; shell->FrameSelection()->SetMouseDownState(PR_FALSE) must be called.
And I wouldn't want to make viewmanager to depend on presshell or selection handling. Frankly, I don't understand what is so wrong calling GrabMouseEvents(nsnull) in ESM.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 286838 [details] [diff] [review]
proposed patch

OK OK, I'm convinced.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 286838 [details] [diff] [review]
proposed patch

This is a small change and would be great to have fixed in 1.9.

Comment 16

[Martijn Wargers (dead)]

Attachment: testcase2

Does this patch also fix the sticky selection issue in this case when you have clicked on the green div?

Comment 17

[Martijn Wargers (dead)]

..because I think testcase2 is basically what the first issue in 390833, comment 6 is talking about, which would mean this should be blockin1.9+.
(also, is there a relation to bug 333175 regarding this bug?)

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

The patch fixes also testcase2. And yes, I think this fixes bug 333175.
Not (yet) sure about bug 390833.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

I backed out this because of regressions.
Perhaps this should go in with some version of Bug 373344.

Comment 21

[Benjamin Xiao]

This does fix bug 390833, well at least the select all behavior when sub windows are closed. Since its been backed out, it doesn't :(

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

Now I see  what the problem probably is. This needs the nsWeakView patch, after that I can upload something better...

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

Attachment: don't rely on views

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

Attachment: get viewmanager from shell

This way if view gets deleted when dispatching click, we don't use dead aView.

Comment 25

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #24)
> Created an attachment (id=289485) [details]
> get viewmanager from shell

This fix works for me, and doesn't cause bug 403662 like the previous patch did (tested on Mac).