Mouseenter/-leave not triggered when element under cursor is moved/removed

• Component: UI Events
• Status: REOPENED ---

Steps to reproduce:
----------------

1) Open the attached example with web inspector open.
2) Click the red div.

Expected: When the red div is removed and the green div takes its place, a mouseenter event should be triggered causing 'enter green' to be output to the console.

Actual: No mouseenter event is triggered.

Build: WebKit r154944 was built on 01 September 2013

Created attachment 210644 [details]
Reproduction

[Alexey Proskuryakov]

Still not sure why we added support for these events, but now we are stuck polishing their behavior...

[Allan Sandfeld Jensen]

I am guessing you can create a similar bug with CSS hover effects not being updated.

[Allan Sandfeld Jensen]

This is a similar problem to when we do scrolling, we need to send a fake mousemove to make sure hover effects are updated (mouseenter/-leave is handled by the same code as CSS hover). 

One solution could be to emit a fake mousemove event when an element that has Element::hovered() set is moved or removed.

[Allan Sandfeld Jensen]

We actually have code to update the hover/active state after a hovered element is removed.

Looking closer at the test-case, there is no layered content here. It only tests that mouseleave is not emitted when an element is removed from the document.

[Allan Sandfeld Jensen]

Created attachment 210721 [details]
Patch

A quick fix to get updateHoverActive to generate mouseenter/-leave events when hoverstate is updated internally. It might be cleaner to just issue fake mousemoves here, but that could have other web visible effects.

[Darin Adler]

Comment on attachment 210721 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=210721&action=review

We should make a regression test. Can we do that?

I’m going to say r=me, but the lack of a test is a big problem, and there are some minor issues in the code, too.

> Source/WebCore/page/EventHandler.cpp:3210
> +            bool shiftKey;
> +            bool ctrlKey;
> +            bool altKey;
> +            bool metaKey;
> +            PlatformKeyboardEvent::getCurrentModifierState(shiftKey, ctrlKey, altKey, metaKey);
> +            PlatformMouseEvent fakeMouseMoveEvent(m_lastKnownMousePosition, m_lastKnownMouseGlobalPosition, NoButton, PlatformEvent::MouseMoved, 0, shiftKey, ctrlKey, altKey, metaKey, currentTime());

It doesn’t seem great to combine the latest modifier state with the “last known” mouse position.

And this code sequence reads awkwardly and poorly here. I would prefer that we factor this into a separate function, and I also suggest we record the modifier state when we record the mouse position instead of explicitly re-getting it now.

[Allan Sandfeld Jensen]

(In reply to comment #7)
> (From update of attachment 210721 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=210721&action=review
> 
> We should make a regression test. Can we do that?
> 
> I’m going to say r=me, but the lack of a test is a big problem, and there are some minor issues in the code, too.
> 
> > Source/WebCore/page/EventHandler.cpp:3210
> > +            bool shiftKey;
> > +            bool ctrlKey;
> > +            bool altKey;
> > +            bool metaKey;
> > +            PlatformKeyboardEvent::getCurrentModifierState(shiftKey, ctrlKey, altKey, metaKey);
> > +            PlatformMouseEvent fakeMouseMoveEvent(m_lastKnownMousePosition, m_lastKnownMouseGlobalPosition, NoButton, PlatformEvent::MouseMoved, 0, shiftKey, ctrlKey, altKey, metaKey, currentTime());
> 
> It doesn’t seem great to combine the latest modifier state with the “last known” mouse position.
> 
> And this code sequence reads awkwardly and poorly here. I would prefer that we factor this into a separate function, and I also suggest we record the modifier state when we record the mouse position instead of explicitly re-getting it now.


I noticed later that we issue a fake mousemove when the hovered element is removed due to CSS display being set to none. I will prefer in the next patch to remove the hovertimer, and use the same fake mousemove mechanism for this case as well. It means removing code instead of adding code :)

[Allan Sandfeld Jensen]

Created attachment 211025 [details]
Patch

[Build Bot]

Comment on attachment 211025 [details]
Patch

Attachment 211025 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1718996

New failing tests:
fast/events/mouseenterleave-detached-element.html

[Build Bot]

Created attachment 211041 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.4

[Allan Sandfeld Jensen]

(In reply to comment #11)
> Created an attachment (id=211041) [details]
> Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2
> 
> The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
> Bot: webkit-ews-14  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.4

 mouseenter on DIV#mydiv1
 mouseenter on BODY
 mouseenter on HTML
+mouseenter on #document
 removing DIV#mydiv1
 mouseenter on DIV#mydiv2
 
It is only failing because it was tested before the patch for bug 120862 was applied.

[Antonio Gomes]

Comment on attachment 211025 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=211025&action=review

Change itself looks good. I am keeping cq- as test might get improved a bit.

> LayoutTests/fast/events/mouseenterleave-detached-element.html:20
> +    document.addEventListener('mouseleave', logMouseEvent, true);

is there any "mouselevave" event expected to be thrown?

> LayoutTests/fast/events/mouseenterleave-detached-element.html:26
> +        window.setTimeout(endTest, 200);

200 is quite a lot :(

If we know exactly how many events are being thrown, can we use a count instead, and call endTest when the count reaches the expected amount?

[Allan Sandfeld Jensen]

(In reply to comment #13)
> (From update of attachment 211025 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=211025&action=review
> 
> Change itself looks good. I am keeping cq- as test might get improved a bit.
> 
> > LayoutTests/fast/events/mouseenterleave-detached-element.html:20
> > +    document.addEventListener('mouseleave', logMouseEvent, true);
> 
> is there any "mouselevave" event expected to be thrown?
> 
> > LayoutTests/fast/events/mouseenterleave-detached-element.html:26
> > +        window.setTimeout(endTest, 200);
> 
> 200 is quite a lot :(
> 
The fake mousemove event has a minimum delay of 100ms.

> If we know exactly how many events are being thrown, can we use a count instead, and call endTest when the count reaches the expected amount?

Currently we only get one mouseenter to the new element. Some might argue we should also get a mouseleave on the removed element, but that is currently a lot harder because it would require emitting an immediately event during handling of the remove. I would also argue that you can not expect mouse events on detached elements or elements being detached.

[Allan Sandfeld Jensen]

Committed r155519: <http://trac.webkit.org/changeset/155519>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 121174

[Alexey Proskuryakov]

This change caused test failures, and Allan is away, so rolling out.

http://build.webkit.org/results/Apple%20MountainLion%20Release%20WK1%20(Tests)/r155540%20(12792)/results.html

The included test case fails, and editing/pasteboard/copy-standalone-image-crash.html started to crash.

[Giulio Bonanome]

Hi. This issue could be linked to this one: https://bugs.webkit.org/show_bug.cgi?id=4117 ? 
CSS :hover is not triggered untile mouse is moved.

[Daniel Trebbien]

See also: http://crbug.com/276329

I am not able to reproduce the issue in Safari 8.0.3 on Mac OS 10.10.2.

[Lucas Forschler]

Mass move bugs into the DOM component.

[Ryosuke Niwa]

These are UI events.