Bug 6821 – Fix for 5983 will not always update hover correctly.

• Component: DOM
• Status: RESOLVED FIXED

[Maciej Stachowiak]

The fix for http://bugzilla.opendarwin.org/show_bug.cgi?id=5983 sometimes won't update properly update hover when the current hover node is removed. In particular, if you remove a node in the normal flow such that a sibling is now positioned under the mouse, the sibling won't properly get hover. Instead of doing the weird walk up the render tree, it might be better to just setHover(0) and set a 0-delay timer to fire a mouse event at the current mouse position.

Another possibility that would catch all kinds of dhtml-driven hover changes would be to send a fake mouse event at the current mouse position every time we update rendering or lay out or whatever. But this runs the risk of hurting DHTML performance, according to hyatt. We'd need some way to measure.

[Maciej Stachowiak]

Created attachment 5970 [details]
a test case where hover does not update properly

Here's an example where mouseover does not update properly.

[Maciej Stachowiak]

Created attachment 5971 [details]
trickier hover test where an ancesor's hover should invalidate too

Mitz pointed out on IRC that hover state for an ancestor node may need to be explicitly invalidated as well, if the removal of a node cause its ancestor to move out from under the mouse as well. Here's a test case that demonstrates this.

The trickier test is broken even in TOT (OK stays yellow even after you move the mouse) due to another issue with the fix for bug 5983, namely that it doesn't account for text nodes (which aren't hovered even when they are the document's hovered node).

See also bug 6431

Created attachment 6106 [details]
Step 0

Not a fix yet, just moving the code into DocumentImpl, simplifying it (Hyatt said on IRC that using hoverAncestor is unnecessary here) and fixing one aspect of the trickier test, namely the problem with text nodes (so now when you move the mouse, the yellow background changes back to white). I think the next step is to get hoveredNodeDetached() to raise a flag somewhere to eventually cause hover state update.

Created attachment 6113 [details]
Set a timer to update hover state

I suspect that I'm doing too little when the timer fires, but I'm sure that's not the only problem with this patch.

[Darin Adler]

Comment on attachment 6113 [details]
Set a timer to update hover state

I think Dave said he wanted to review this one.

[Dave Hyatt]

Comment on attachment 6113 [details]
Set a timer to update hover state

(1) You need to cancel the timer if the FrameView goes away.

(2) You need to cancel the timer if a mouse move comes in before the timer fires.

(3) Doesn't :active have all of the same issues as :hover?  It seems like the exact same bugs apply to :active.

This does seem like the right approach.

[Trey Matteson]

I bet you also need to cancel the timer if the app loses active status or the window loses focus.

(In reply to comment #9)
> I bet you also need to cancel the timer if the app loses active status or the
> window loses focus.

In WebKit, hover state isn't reset when either of these happen, so even in the unlikely event that a deactivate event comes between setting the 0-delay timer and the timer firing, I don't think it's going to cause any problem.

The other thing I looked at is what happens if the window/app is inactive to begin with when hoveredNodeDetached gets called. Firefox's inactive app behavior is the same as Safari's inactive window/app behavior, which is to freeze the mouse where it was. Setting the timer when inactive (and acting upon it when it fires) is just what's needed to match Firefox's behavior for this bug's testcases too.

Created attachment 6197 [details]
Updated patch

(In reply to comment #8)
> (1) You need to cancel the timer if the FrameView goes away.

Done. How come the relayout timer doesn't need to be canceled there?

> (2) You need to cancel the timer if a mouse move comes in before the timer
> fires.

Done.

> (3) Doesn't :active have all of the same issues as :hover?  It seems like the
> exact same bugs apply to :active.

It has some of the same issues (the bug 5983 ones). I added code to trim the rendererless end of the active chain and reset m_active and m_inActiveChain, but since the active chain is locked, I think there's no need to post a mouse event.

ChangeLog entry and test still missing from this patch, so r- anyway.

[Darin Adler]

Comment on attachment 6197 [details]
Updated patch

Patch will have to be revised, since there's no such thing as startTimer any more; I just redid timers.

Also, for that text node stuff, maybe we need to make NodeImpl::ancestorElement public so we could say something more like node->ancestorElement() != m_activeNode -- I'm thinking that if it's never a text node it should be m_activeElement rather than m_activeNode, not that we have to fix that to fix this bug.

Doing review- because of the timer thing. Sorry about that.

Created attachment 6209 [details]
Updated with new timers

Created attachment 6210 [details]
Additional cases

What about these two cases? In the first one, the div jumps but stays red until the mouse is moved. In the second one, it alternates places and colors only as long as you're moving the mouse. In Firefox, the first div turns yellow immediately after it moves, and the second div keeps moving from side to side as long as the mouse is inside its original position.

Created attachment 6292 [details]
Updated patch including ChangeLog entry and manual test

See also comment #11.

[Dave Hyatt]

Comment on attachment 6292 [details]
Updated patch including ChangeLog entry and manual test

r=me

[Lucas Forschler]

Mass moving XML DOM bugs to the "DOM" Component.