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.
Reproducible on a G4/867DP.
Created attachment 7051 [details]
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 :-)
Comment on attachment 7051 [details]
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.)
Created attachment 7115 [details]
additional test case
Proposed by Domas Mituzas on IRC.
Created attachment 7137 [details]
Created attachment 7144 [details]
Improved compatibility with WinIE and Firefox; corrected expected results.
Created attachment 7147 [details]
Comment on attachment 7147 [details]
Actually no, the logic in FrameView::viewportMouseMoveEvent() still doesn't look right...
Created attachment 7156 [details]
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 :)
I thought leapForward takes in the # of milliseconds to move timestamps forward by. Or did that change recently?
Created attachment 7169 [details]
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.
Comment on attachment 7169 [details]
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.
Created attachment 7179 [details]
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.
Comment on attachment 7179 [details]
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.
(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.
> + 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
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);
+ RefPtr<Frame> newSubframe = subframeForEvent(mev);
+ if (newSubframe && d->oldSubframe != newSubframe)
+ m_frame->passSubframeEventToSubframe(mev, newSubframe.get());
+ setCursor(selectCursor(mev, m_frame.get(), d->mousePressed));
+ d->oldSubframe = newSubframe;
(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.