Bug 120862 – MouseEnter and MouseLeave may be emitted on Document nodes

• Component: UI Events
• Status: RESOLVED FIXED

[Allan Sandfeld Jensen]

Only Elements can be targets of mouse-events, which means emitting mouseenter or mouseleave on a document node is a mistake.

This bug was noticed when cleaning up the overgeneric use of Nodes and replacing them with Elements in Document::updateHoverActiveState.

[Allan Sandfeld Jensen]

Created attachment 210761 [details]
Patch

[Andreas Kling]

This patch looks cool, though the new test seems to be failing on EWS. Any idea what's up with that?

[Allan Sandfeld Jensen]

(In reply to comment #2)
> This patch looks cool, though the new test seems to be failing on EWS. Any idea what's up with that?

I will take a look, but it does assume the mouse pointer starts outside the document before the test and can be moved off it again by moving it to -10,-10.

[Darin Adler]

Comment on attachment 210761 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:5868
>              for (Node* node = oldHoveredElement.get(); node; node = node->parentNode()) {
> -                if (!mustBeInActiveChain || (node->isElementNode() && toElement(node)->inActiveChain()))
> -                    nodesToRemoveFromChain.append(node);
> +                if (!node->isElementNode())
> +                    continue;
> +                Element* element = toElement(node);
> +                if (!mustBeInActiveChain || element->inActiveChain())
> +                    elementsToRemoveFromChain.append(element);
>              }

Should just write:

    for (Element* element = oldHoveredElement.get(); element; element = element->parentElement()) {

That would eliminate three lines of code below.

Also, mustBeInActiveChain should be checked outside the loop, not inside it.

> Source/WebCore/dom/Document.cpp:5878
>          for (RenderObject* curr = oldHoverObj; curr && curr != ancestor; curr = curr->hoverAncestor()) {
> -            if (!curr->node() || curr->isText())
> +            if (!curr->node() || !curr->node()->isElementNode())
>                  continue;
> -            if (!mustBeInActiveChain || (curr->node()->isElementNode() && toElement(curr->node())->inActiveChain()))
> -                nodesToRemoveFromChain.append(curr->node());
> +            Element* element = toElement(curr->node());
> +            if (!mustBeInActiveChain || element->inActiveChain())
> +                elementsToRemoveFromChain.append(element);
>          }

!mustBeInActiveChain should be checked outside the loop

> Source/WebCore/dom/Document.cpp:5893
>      for (RenderObject* curr = newHoverObj; curr; curr = curr->hoverAncestor()) {
> -        if (!curr->node() || curr->isText())
> +        if (!curr->node() || !curr->node()->isElementNode())
>              continue;
> -        if (!mustBeInActiveChain || (curr->node()->isElementNode() && toElement(curr->node())->inActiveChain()))
> -            nodesToAddToChain.append(curr->node());
> +        Element* element = toElement(curr->node());
> +        if (!mustBeInActiveChain || element->inActiveChain())
> +            elementsToAddToChain.append(element);
>      }

!mustBeInActiveChain should be checked outside the loop

> Source/WebCore/dom/Document.cpp:5896
> -    size_t removeCount = nodesToRemoveFromChain.size();
> +    size_t removeCount = elementsToRemoveFromChain.size();
>      for (size_t i = 0; i < removeCount; ++i) {

Our modern style for this is:

    for (size_t i = 0, size = elementsToRemoveFromChain.size(); i < size; ++i) {

Please consider using that style.

[Build Bot]

Comment on attachment 210761 [details]
Patch

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

New failing tests:
fast/events/mouseenterleave-on-document.html

[Build Bot]

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

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

[Build Bot]

Comment on attachment 210761 [details]
Patch

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

New failing tests:
fast/events/mouseenterleave-on-document.html

[Build Bot]

Created attachment 210901 [details]
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion

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

[Allan Sandfeld Jensen]

(In reply to comment #3)
> (In reply to comment #2)
> > This patch looks cool, though the new test seems to be failing on EWS. Any idea what's up with that?
> 
> I will take a look, but it does assume the mouse pointer starts outside the document before the test and can be moved off it again by moving it to -10,-10.

Yes, it is leaving the document that appears to be problematic

[Allan Sandfeld Jensen]

(In reply to comment #4)
> Also, mustBeInActiveChain should be checked outside the loop, not inside it.
> 
I am not sure I understand that part. mustBeInActiveChain is checked as part of an else statement, if I moved it outside the loop I would need two different loops to choose from.

[Allan Sandfeld Jensen]

Created attachment 211021 [details]
Patch

Cleanup suggested by darin, and the test now moves the mouse out of the document in positive coordinates

[Build Bot]

Comment on attachment 211021 [details]
Patch

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

New failing tests:
fast/events/mouseenterleave-on-document.html

[Build Bot]

Created attachment 211026 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion

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

[Allan Sandfeld Jensen]

(In reply to comment #12)
> (From update of attachment 211021 [details])
> Attachment 211021 [details] did not pass mac-ews (mac):
> Output: http://webkit-queues.appspot.com/results/1726533
> 
> New failing tests:
> fast/events/mouseenterleave-on-document.html

It seems moving the mouse out of the webview doesn't work on the mac test bots. If the test I uploaded to bug 121026 works on Mac though, that should be able to test this bug as well.

[Antonio Gomes]

Comment on attachment 211021 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        This cleans up the code and fixes a hidden bug.

You should mention why the change, and what the hidden bug is.

[Allan Sandfeld Jensen]

(In reply to comment #15)
> (From update of attachment 211021 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=211021&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        This cleans up the code and fixes a hidden bug.
> 
> You should mention why the change, and what the hidden bug is.

Right. The hidden bug is then one mentioned in the title, but yes it is not really clear.

[Allan Sandfeld Jensen]

Created attachment 211032 [details]
Patch

[Antonio Gomes]

Comment on attachment 211032 [details]
Patch

Looks good. Can we test that with iframe documents?

[Allan Sandfeld Jensen]

(In reply to comment #18)
> (From update of attachment 211032 [details])
> Looks good. Can we test that with iframe documents?

I assume so. I am waiting for the output from EWS on the patch in bug 121026, but it should be exactly this bug it demonstrates since the test also depends on this fix.

[Allan Sandfeld Jensen]

Created attachment 211037 [details]
Patch

[Allan Sandfeld Jensen]

Committed r155351: <http://trac.webkit.org/changeset/155351>