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.
Created attachment 210761 [details] Patch
This patch looks cool, though the new test seems to be failing on EWS. Any idea what's up with that?
(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.
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.
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
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
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
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
(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
(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.
Created attachment 211021 [details] Patch Cleanup suggested by darin, and the test now moves the mouse out of the document in positive coordinates
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
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
(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.
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.
(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.
Created attachment 211032 [details] Patch
Comment on attachment 211032 [details] Patch Looks good. Can we test that with iframe documents?
(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.
Created attachment 211037 [details] Patch
Committed r155351: <http://trac.webkit.org/changeset/155351>