Bug 3439 – mouseover effects can get stuck sometimes due to missing events

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Stéphane Rieppi]

On http://www.shscomputer.be, most orange/red link gives you contextual rollover
rectangles: they appear when you pass on them and disappear when you move the mouse.

If you have a slower Mac (I believe it's harder to notice on a quick one),
you'll notice that if you move the mouse quickly, the rectangle doesn't hide as
it should, probably because it never captures the event that the mouse has moved
out.  That's annoying and it only happens with Safari, every other browser I've
tried behaves.  The bug always existed, even with Safari 1.x.

Moreover, the background colour (it uses a alpha-layered PNG) is not correct
anymore (this one is new to Tiger).  But it's a duplicate of bug #3438.

[Alexey Proskuryakov]

Reproducible on a G4/867DP.

[Alexey Proskuryakov]

Created attachment 7051 [details]
naive fix

Well, I don't really expect it to be that simple, but this passes layout tests, and additionally fixes bug 7701. Will probably get an r- for reasons other than a missing test :-)

[Geoffrey Garen]

Comment on attachment 7051 [details]
naive fix

Yeah, you do need a test. And a ChangeLog. DumpRenderTree can simulate mouse events, so a test is possible.

You can test oldUnder itself for truth instead of oldUnder.get().

Doesn't this patch prevent mouseout from firing when you move the mouse between frames? In general, what's the purpose of the test

    oldUnder->getDocument() != frame()->document()

? To ensure that the node is still in the document? inDocument() is probably better for that. (Make sure your test covers that case.)

[Alexey Proskuryakov]

Created attachment 7115 [details]
additional test case

Proposed by Domas Mituzas on IRC.

[Alexey Proskuryakov]

Created attachment 7137 [details]
large test

[Alexey Proskuryakov]

Created attachment 7144 [details]
large test

Improved compatibility with WinIE and Firefox; corrected expected results.

[Alexey Proskuryakov]

Created attachment 7147 [details]
proposed fix

[Alexey Proskuryakov]

Comment on attachment 7147 [details]
proposed fix

Actually no, the logic in FrameView::viewportMouseMoveEvent() still doesn't look right...

[Alexey Proskuryakov]

Created attachment 7156 [details]
proposed fix

Added a second test for IFRAME manipulation. There is one result that differs from Firefox: FF sends a mouseout event to DIV elements that are being removed from the document via removeChild, but does not send it to IFRAME elements. Here, either element gets a mouseout (see "mouseout on frame5" in the log).

I am not sure if we should follow Firefox here; also, this may actually be some artifact of how IFRAMEs are removed, not a feature of mouse event dispatching code. And anyway, this is already far beyond the scope of the bugs we have in Bugzilla :)

[Justin Garcia]

I thought leapForward takes in the # of milliseconds to move timestamps forward by.  Or did that change recently?

[Alexey Proskuryakov]

Created attachment 7169 [details]
proposed fix

Indeed, which means leapForward() is actually not needed here! Somehow it seemed to make a difference while writing the test, but it was probably just a wrong impression.

[Darin Adler]

Comment on attachment 7169 [details]
proposed fix

I like the general direction of this patch a lot.

But I don't like the idea of adding the subframe function to MouseEventWithHitTestResults. It's fine to have that helper function, but I don't think it should be a member function of the event class. I think we can just have a local function in FrameView.cpp (not a member of any class) that takes the mouse event or node as a parameter. Might need a clearer name than "subframe", or "subframe" might be OK.

+            if (m_oldUnder && m_oldUnder->getDocument() != frame()->document())

In the check above, do we also want to check inDocument() on m_oldUnder?

Does FrameView::dispatchMouseEvent handle the case where targetNode is destroyed as a side effect of handling the mouseout or mouseover event? I think we may need to ref it.

By the way, the "pass subframe event to subframe" code might need to be moved into FrameView eventually -- the strange way this code is distributed across FrameView and Frame is an artifact of us not wanting to modify the KHTML code much in the old days (so we put stuff in KWQKHTMLPart, now the Macintosh-specific part of Frame).

Setting review- mainly because of the subframe function, but please also consider the other comments.

[Alexey Proskuryakov]

Created attachment 7179 [details]
proposed fix

Moved the subframe code from MouseEventWithHitTestResults to a function in FrameView.cpp.

Checking for inDocument() changes the results of one of the included tests to not match Firefox and WinIE - they seem to dispatch mouseOut events to removed nodes. OTOH, as mentioned above, it changes the results for IFRAME to match Firefox. Seems better this way, but I may be missing something.

There is already a ref for targetNode as it is assigned to d->underMouse. But we actually needed a ref for "this", added now. The difference between d->underMouse and d->oldUnder looks ugly; I don't really understand it.

Other changes, as discussed on IRC: added a helper for prepareMouseEvent, removed prevMouseX/prevMouseY, moved the new data members to FrameViewPrivate.

Didn't address the fact that an additional mouse move is needed to mouseOut events to fire after an element under mouse is removed - will probably fit in bug 4117 better.

[Darin Adler]

Comment on attachment 7179 [details]
proposed fix

It's not great to pass the subframe to the passSubframeEventToSubframe function, but on the other hand, it's my own fault for insisting the subframe function be removed from MouseEventWithHitTestResults. Eventually we should be able to clean that up a bit -- I'm not sure it needs to go back through the Frame and through Macintosh-specific code.

+    RenderObject *renderer = mev.innerNode()->renderer();

New code should put the * next to the class name, not the variable.

+    RefPtr<FrameView> protector(this);

No explanation of why this was added.

I don't understand the positioning of the call to setCursor in the new code. Wouldn't we need a new call to setCursor after passing the event to the new subframe?

Ideally I think that oldUnder should be per-page, not per-frame. If we did that, I think we might be able to simply get rid of oldSubframe.

Despite those doubts and comments, looks fine to land.

r=me

[Alexey Proskuryakov]

(In reply to comment #14)
> the subframe function be removed from MouseEventWithHitTestResults.

  Frankly, I didn't really understand why - it's a part of "HitTestResult", just like innerNode. And FrameView.cpp is huge, so factoring out code into other files sounds natural...

> New code should put the * next to the class name, not the variable.

  Fixed.

> +    RefPtr<FrameView> protector(this);
> No explanation of why this was added.

  One of the tests crashed without it; the other FrameView::viewportMouseXXXEvent methods already do protect "this".

> I don't understand the positioning of the call to setCursor in the new code.
> Wouldn't we need a new call to setCursor after passing the event to the new
> subframe?

  Looks like I did get it wrong - it should either pass the event to the new subframe, or call setCursor if there's none (since passSubframeEventToSubframe() always returns true for NSMouseMoved now). Going to land like this:

-    MouseEventWithHitTestResults mev = m_frame->document()->prepareMouseEvent(d->mousePressed && m_frame->mouseDownMayStartSelect(),
-        d->mousePressed, true, xm, ym, mouseEvent);
+    MouseEventWithHitTestResults mev = prepareMouseEvent(d->mousePressed && m_frame->mouseDownMayStartSelect(),
+        d->mousePressed, true, mouseEvent);
 
-    if (!m_frame->passSubframeEventToSubframe(mev))
-        setCursor(selectCursor(mev, m_frame.get(), d->mousePressed));
-        
+    if (d->oldSubframe)
+        m_frame->passSubframeEventToSubframe(mev, d->oldSubframe.get());
+
     bool swallowEvent = dispatchMouseEvent(mousemoveEvent, mev.innerNode(), false, 0, mouseEvent, true);
     if (!swallowEvent)
         m_frame->khtmlMouseMoveEvent(mev);
+    
+    RefPtr<Frame> newSubframe = subframeForEvent(mev);
+    
+    if (newSubframe && d->oldSubframe != newSubframe)
+        m_frame->passSubframeEventToSubframe(mev, newSubframe.get());
+    else
+        setCursor(selectCursor(mev, m_frame.get(), d->mousePressed));
+    
+    d->oldSubframe = newSubframe;
 }

[Darin Adler]

(In reply to comment #15)
> (In reply to comment #14)
> > the subframe function be removed from MouseEventWithHitTestResults.
> 
>   Frankly, I didn't really understand why - it's a part of "HitTestResult",
> just like innerNode. And FrameView.cpp is huge, so factoring out code into
> other files sounds natural...

The reason why is that the class is just a little data holder. I don't want to add smarts to MouseEventWithHitTestResults.

Eventually this will go into a new EventHandler class that's going to be factored out of Frame and FrameView.

[Alexey Proskuryakov]

Landed, r13402.