Closed Bug 401528 Opened 14 years ago Closed 14 years ago

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

Categories

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

x86
All
defect

Tracking

()

RESOLVED FIXED

People

(Reporter: agustinmfernandez, Assigned: smaug)

References

Details

Attachments

(5 files, 3 obsolete files)

Attached file 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.
Component: General → DOM: CSSOM
Product: Firefox → Core
QA Contact: general → general
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.
Component: DOM: CSSOM → Event Handling
QA Contact: general → events
Attached patch shows where the problem is (obsolete) — Splinter Review
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.
Attached patch possible patch (obsolete) — Splinter Review
This is not so ugly and should be pretty safe.
Assignee: nobody → Olli.Pettay
Attachment #286664 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #286666 - Flags: superreview?(roc)
Attachment #286666 - Flags: review?(roc)
-  // 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 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.
(In reply to comment #5)
> even though
even if
> 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 on attachment 286666 [details] [diff] [review]
possible patch

Will post a new patch
Attachment #286666 - Attachment is obsolete: true
Attachment #286666 - Flags: superreview?(roc)
Attachment #286666 - Flags: review?(roc)
Attached patch proposed patch (obsolete) — Splinter Review
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.
Attachment #286838 - Flags: review?(roc)
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.
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 ?
Sorry, yes.

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

Yes.
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.
Attachment #286965 - Flags: review?(roc)
Comment on attachment 286838 [details] [diff] [review]
proposed patch

OK OK, I'm convinced.
Attachment #286838 - Flags: superreview+
Attachment #286838 - Flags: review?(roc)
Attachment #286838 - Flags: review+
Attachment #286965 - Flags: review?(roc)
Comment on attachment 286838 [details] [diff] [review]
proposed patch

This is a small change and would be great to have fixed in 1.9.
Attachment #286838 - Flags: approval1.9?
Attached file testcase2
Does this patch also fix the sticky selection issue in this case when you have clicked on the green div?
..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?)
Flags: blocking1.9?
Flags: in-testsuite?
Attachment #286838 - Flags: approval1.9? → approval1.9+
The patch fixes also testcase2. And yes, I think this fixes bug 333175.
Not (yet) sure about bug 390833.
Duplicate of this bug: 333175
Flags: blocking1.9? → blocking1.9+
Priority: -- → P4
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Depends on: 403662
I backed out this because of regressions.
Perhaps this should go in with some version of Bug 373344.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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 :(
Now I see  what the problem probably is. This needs the nsWeakView patch, after that I can upload something better...
Attachment #286838 - Attachment is obsolete: true
This way if view gets deleted when dispatching click, we don't use dead aView.
Attachment #289485 - Flags: superreview?(roc)
Attachment #289485 - Flags: review?(roc)
(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).
Attachment #289485 - Flags: superreview?(roc)
Attachment #289485 - Flags: superreview+
Attachment #289485 - Flags: review?(roc)
Attachment #289485 - Flags: review+
Status: REOPENED → RESOLVED
Closed: 14 years ago14 years ago
Resolution: --- → FIXED
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.