Reverting r142861. Hit testing inside of style recalc is fundamentally wrong
authorsimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 15 Feb 2013 04:28:45 +0000 (04:28 +0000)
committersimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 15 Feb 2013 04:28:45 +0000 (04:28 +0000)
Source/WebCore:

* page/EventHandler.cpp:
(WebCore::EventHandler::selectCursor):
(WebCore::EventHandler::handleMouseMoveEvent):
* page/EventHandler.h:
* rendering/RenderObject.cpp:
(WebCore::RenderObject::setStyle):
(WebCore::areNonIdenticalCursorListsEqual):
(WebCore::areCursorsEqual):
(WebCore::RenderObject::styleDidChange):

LayoutTests:

* fast/events/mouse-cursor-change-expected.txt: Removed.
* fast/events/mouse-cursor-change.html: Removed.
* fast/events/mouse-cursor-no-mousemove-expected.txt: Removed.
* fast/events/mouse-cursor-no-mousemove.html: Removed.
* platform/mac/TestExpectations:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@142956 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/fast/events/mouse-cursor-change-expected.txt [deleted file]
LayoutTests/fast/events/mouse-cursor-change.html [deleted file]
LayoutTests/fast/events/mouse-cursor-no-mousemove-expected.txt [deleted file]
LayoutTests/fast/events/mouse-cursor-no-mousemove.html [deleted file]
LayoutTests/platform/mac/TestExpectations
Source/WebCore/ChangeLog
Source/WebCore/page/EventHandler.cpp
Source/WebCore/page/EventHandler.h
Source/WebCore/rendering/RenderObject.cpp

index 6173cd9..0083376 100644 (file)
@@ -1,3 +1,13 @@
+2013-02-14  Simon Fraser  <simon.fraser@apple.com>
+
+        Reverting r142861. Hit testing inside of style recalc is fundamentally wrong
+
+        * fast/events/mouse-cursor-change-expected.txt: Removed.
+        * fast/events/mouse-cursor-change.html: Removed.
+        * fast/events/mouse-cursor-no-mousemove-expected.txt: Removed.
+        * fast/events/mouse-cursor-no-mousemove.html: Removed.
+        * platform/mac/TestExpectations:
+
 2013-02-14  Florin Malita  <fmalita@chromium.org>
 
         [SVG] Cached filter results are not invalidated on repaint rect change
         * fast/text-autosizing/narrow-descendants-combined-expected.html: Added.
         * fast/text-autosizing/narrow-descendants-combined.html: Added.
 
-2013-02-14  Aivo Paas  <aivopaas@gmail.com>
-
-        Updating mouse cursor on style changes without emitting fake mousemove event
-        https://bugs.webkit.org/show_bug.cgi?id=101857
-        Changing CSS cursor should work no matter is mouse button is pressed or not
-        https://bugs.webkit.org/show_bug.cgi?id=53341
-
-        Reviewed by Allan Sandfeld Jensen.
-
-        Added tests for changing cursor on mousemove, mousedown, mouseup and mousemove
-        while mouse button being hold down. Also added test to verify that a mousemove
-        event is not fired for changing cursor while mouse is not moving.
-
-        * fast/events/mouse-cursor-change-expected.txt: Added.
-        * fast/events/mouse-cursor-change.html: Added.
-        * fast/events/mouse-cursor-no-mousemove-expected.txt: Added.
-        * fast/events/mouse-cursor-no-mousemove.html: Added.
-        * platform/mac/TestExpectations:
-
 2013-02-06  Gregg Tavares  <gman@chromium.org>
 
         Adds the WebGL Conformance Tests limits folder.
diff --git a/LayoutTests/fast/events/mouse-cursor-change-expected.txt b/LayoutTests/fast/events/mouse-cursor-change-expected.txt
deleted file mode 100644 (file)
index 48b7abe..0000000
+++ /dev/null
@@ -1,24 +0,0 @@
-Test that mouse cursors are changed correctly on mouse events.
-
-On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
-
-
-Bug 53341
-
-
-Mouse move
-Cursor Info: type=Hand hotSpot=0,0
-
-Mouse down
-Cursor Info: type=Progress hotSpot=0,0
-
-Mouse hold down, move
-Cursor Info: type=Hand hotSpot=0,0
-
-Mouse up
-Cursor Info: type=Help hotSpot=0,0
-
-PASS successfullyParsed is true
-
-TEST COMPLETE
-
diff --git a/LayoutTests/fast/events/mouse-cursor-change.html b/LayoutTests/fast/events/mouse-cursor-change.html
deleted file mode 100644 (file)
index 0d41be1..0000000
+++ /dev/null
@@ -1,78 +0,0 @@
-<!DOCTYPE html>
-<html>
-<head>
-<script src="../js/resources/js-test-pre.js"></script>
-<style type="text/css">
-</style>
-</head>
-<body>
-<p id="description"></p>
-<p><a href="https://bugs.webkit.org/show_bug.cgi?id=53341">Bug 53341</a></p>
-<div id="test-container">
-    <div id="target" onMouseDown="style.cursor='progress';event.preventDefault();" onMouseMove="style.cursor='pointer';" onMouseUp="style.cursor='help';" style="cursor:pointer;">Play with mouse on this element. Cursors change on events - mousemove: pointer(hand), mousedown: progress, mouseup: help.</div>
-</div>
-<br/>
-<div id="console"></div>
-<script>
-description("Test that mouse cursors are changed correctly on mouse events.");
-
-if (!window.eventSender) {
-    testFailed('This test requires DumpRenderTree');
-}
-
-if (window.testRunner) {
-    testRunner.dumpAsText();
-    testRunner.waitUntilDone();
-    window.jsTestIsAsync = true;
-}
-
-function runTest(prepare, next) {
-    prepare();
-    setTimeout(function() {
-        debug('Cursor Info: ' + window.internals.getCurrentCursorInfo(document));
-        debug('');
-        next();
-    }, 0);
-}
-
-function testsDone() {
-    // This text is redundant with the test output - hide it
-    document.getElementById('test-container').style.display = 'none';
-    finishJSTest();
-}
-
-// Can't do anything useful here without eventSender
-if (window.eventSender) {
-    var target = document.getElementById('target');
-    eventSender.dragMode = false;
-    var tests = [
-        function() {
-            debug('Mouse move');
-            eventSender.mouseMoveTo(target.offsetLeft + 3, target.offsetTop + 3);
-        }, function() {
-            debug('Mouse down');
-            eventSender.mouseDown();
-        }, function() {
-            debug('Mouse hold down, move');
-            eventSender.mouseMoveTo(target.offsetLeft + 13, target.offsetTop + 3);
-        }, function() {
-            debug('Mouse up');
-            eventSender.mouseUp();
-        }
-    ];
-
-    var i = 0;
-    function nextTest() {
-        if (i < tests.length) {
-            runTest(tests[i++], nextTest);
-        } else {
-            testsDone();
-        }
-    }
-    nextTest();
-}
-
-</script>
-<script src="../../fast/js/resources/js-test-post.js"></script>
-</body>
-</html>
diff --git a/LayoutTests/fast/events/mouse-cursor-no-mousemove-expected.txt b/LayoutTests/fast/events/mouse-cursor-no-mousemove-expected.txt
deleted file mode 100644 (file)
index a5d0645..0000000
+++ /dev/null
@@ -1,16 +0,0 @@
-Test that there is no mousemove event fired when changing cursor.
-
-On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
-
-
-Bug 85343
-
-
-TEST CASE: Mouse idle, change cursor should not fire mousemove event
-Cursor Info: type=Pointer hotSpot=0,0
-Cursor Info: type=Help hotSpot=0,0
-
-PASS successfullyParsed is true
-
-TEST COMPLETE
-
diff --git a/LayoutTests/fast/events/mouse-cursor-no-mousemove.html b/LayoutTests/fast/events/mouse-cursor-no-mousemove.html
deleted file mode 100644 (file)
index c459821..0000000
+++ /dev/null
@@ -1,55 +0,0 @@
-<!DOCTYPE html>
-<html>
-<head>
-<script src="../js/resources/js-test-pre.js"></script>
-<style type="text/css">
-</style>
-</head>
-<body>
-<p id="description"></p>
-<p><a href=https://bugs.webkit.org/show_bug.cgi?id=85343>Bug 85343</a></p>
-<div id="test-container">
-    <div id="target" style="cursor:default">Mouse idle, change cursor should not fire mousemove event</div>
-</div>
-<br/>
-<div id="console"></div>
-<script>
-description("Test that there is no mousemove event fired when changing cursor.");
-
-if (!window.eventSender) {
-    testFailed('This test requires DumpRenderTree');
-}
-
-if (window.testRunner) {
-    testRunner.dumpAsText();
-    testRunner.waitUntilDone();
-    window.jsTestIsAsync = true;
-}
-
-// Can't do anything useful here without eventSender
-if (window.eventSender) {
-    var node = document.getElementById('target');
-    debug('TEST CASE: ' + node.textContent);
-    eventSender.mouseMoveTo(node.offsetLeft + 3, node.offsetTop + 3);
-    debug('Cursor Info: ' + window.internals.getCurrentCursorInfo(document));
-    node.addEventListener('mousemove', function() {
-        testFailed('Mousemove event should not be fired when changing cursor');
-        finishJSTest();
-    });
-    node.style.cursor = 'help';
-    setTimeout(function() {
-        debug('Cursor Info: ' + window.internals.getCurrentCursorInfo(document));
-        debug('');
-    }, 0);
-
-    // Give some time for the change to resolve. Fake mousemove event that caused bug, is using a timer
-    setTimeout(function() {
-        document.getElementById('test-container').style.display = 'none';
-        finishJSTest();
-    }, 150);
-}
-
-</script>
-<script src="../../fast/js/resources/js-test-post.js"></script>
-</body>
-</html>
index 09ea12a..9b8b9b8 100644 (file)
@@ -1416,7 +1416,4 @@ webkit.org/b/109209 fast/text/international/bidi-ignored-for-first-child-inline.
 # Crashing after https://webkit.org/b/105667
 webkit.org/b/109232 [ Debug ] inspector/debugger/debugger-reload-on-pause.html [ Crash ]
 
-# Mac fails cursor change test for unknown reasons
-webkit.org/b/103857 fast/events/mouse-cursor-change.html
-
 webkit.org/b/109869 media/media-captions.html [ Crash ]
index 0806dbd..ccbba53 100644 (file)
@@ -1,3 +1,17 @@
+2013-02-14  Simon Fraser  <simon.fraser@apple.com>
+
+        Reverting r142861. Hit testing inside of style recalc is fundamentally wrong
+
+        * page/EventHandler.cpp:
+        (WebCore::EventHandler::selectCursor):
+        (WebCore::EventHandler::handleMouseMoveEvent):
+        * page/EventHandler.h:
+        * rendering/RenderObject.cpp:
+        (WebCore::RenderObject::setStyle):
+        (WebCore::areNonIdenticalCursorListsEqual):
+        (WebCore::areCursorsEqual):
+        (WebCore::RenderObject::styleDidChange):
+
 2013-02-14  Florin Malita  <fmalita@chromium.org>
 
         [SVG] Cached filter results are not invalidated on repaint rect change
         * inspector/front-end/TimelinePanel.js:
         * inspector/front-end/WebKit.qrc:
 
-2013-02-14  Aivo Paas  <aivopaas@gmail.com>
-
-        Updating mouse cursor on style changes without emitting fake mousemove event
-        https://bugs.webkit.org/show_bug.cgi?id=101857
-
-        Reviewed by Allan Sandfeld Jensen.
-
-        Mouse cursor changes in styles used to be reflected in UI through dispatching a fake
-        mousemove event. The old approach has some flaws: it emits a mousemove event in
-        javascript when there is no mouse movement involved (bug 85343); the fake mousemove
-        event is cancelled while there is a mouse button held down - cursor won't change
-        until mouse is moved or the button released (bug 53341); it has extra overhead of
-        using a timer which was introduced to make scrolling smoother.
-
-        The new approach does not use the fake mousemove event. Instead, it uses only the logic
-        needed for the actual cursor change to happen. This bypasses all the mousemove event related
-        overhead. The remaining code is a stripped version of what was run through the mousemove
-        event path. Everything that was not needed for changing a cursor is stripped off, everything
-        that is needed, remains the same.
-
-        The call to update cursor was moved up in the call tree from RenderObject::StyleDidChange
-        to RenderObject::SetStyle right after the StyleDidChange call. This allows to any updates
-        and style propagations in StyleDidChange to happen and makes sure that a cursor change is
-        not missed. Previous place was at the end of RenderObject::StyleDidChange, where it could
-        have been missed because of an early exit. For example, cursor change on mousedown/up on
-        a text node missed the correct cursor in the first pass.
-
-        Refactored EventHandler::selectCursor to not take a whole mouse event but instead work with
-        HitTestResult so that EventHandler::updateCursor must not create a useless PlatformEvent.
-
-        Fixes: https://bugs.webkit.org/show_bug.cgi?id=85343 (mousemove event on cursor change)
-               https://bugs.webkit.org/show_bug.cgi?id=53341 (no cursor change when mouse button down)
-
-        Tests: fast/events/mouse-cursor-change.html
-               fast/events/mouse-cursor-no-mousemove.html
-
-        * page/EventHandler.cpp:
-        (WebCore::EventHandler::updateCursor): Newly added method for updating mouse cursor
-        (WebCore):
-        (WebCore::EventHandler::selectCursor):
-        (WebCore::EventHandler::handleMouseMoveEvent):
-        * page/EventHandler.h:
-        (EventHandler):
-        * rendering/RenderObject.cpp:
-        (WebCore::areNonIdenticalCursorListsEqual):
-        (WebCore):
-        (WebCore::areCursorsEqual):
-        (WebCore::RenderObject::setStyle):
-        (WebCore::RenderObject::styleDidChange):
-
 2013-02-13  Ilya Tikhonovsky  <loislo@chromium.org>
 
         Web Inspector: Native Memory Instrumentation: Report child nodes as direct members of a container node to make them look like a tree in the snapshot.
index 67776fc..fb631f7 100644 (file)
@@ -1237,40 +1237,7 @@ bool EventHandler::useHandCursor(Node* node, bool isOverLink, bool shiftKey)
     return ((isOverLink || isSubmitImage(node)) && (!editable || editableLinkEnabled));
 }
 
-void EventHandler::updateCursor()
-{
-    if (m_mousePositionIsUnknown)
-        return;
-
-    Settings* settings = m_frame->settings();
-    if (settings && !settings->deviceSupportsMouse())
-        return;
-
-    FrameView* view = m_frame->view();
-    if (!view)
-        return;
-
-    if (!m_frame->page() || !m_frame->page()->isOnscreen() || !m_frame->page()->focusController()->isActive())
-        return;
-
-    bool shiftKey;
-    bool ctrlKey;
-    bool altKey;
-    bool metaKey;
-    PlatformKeyboardEvent::getCurrentModifierState(shiftKey, ctrlKey, altKey, metaKey);
-
-    HitTestRequest request(HitTestRequest::ReadOnly);
-    HitTestResult result(view->windowToContents(m_lastKnownMousePosition));
-    m_frame->document()->renderView()->hitTest(request, result);
-
-    OptionalCursor optionalCursor = selectCursor(result, shiftKey);
-    if (optionalCursor.isCursorChange()) {
-        m_currentMouseCursor = optionalCursor.cursor();
-        view->setCursor(m_currentMouseCursor);
-    }
-}
-
-OptionalCursor EventHandler::selectCursor(const HitTestResult& result, bool shiftKey)
+OptionalCursor EventHandler::selectCursor(const MouseEventWithHitTestResults& event, Scrollbar* scrollbar)
 {
     if (m_resizeLayer && m_resizeLayer->inResizeMode())
         return NoCursorChange;
@@ -1283,16 +1250,8 @@ OptionalCursor EventHandler::selectCursor(const HitTestResult& result, bool shif
         return NoCursorChange;
 #endif
 
-    Node* node = result.targetNode();
-    if (!node)
-        return NoCursorChange;
-    bool originalIsText = node->isTextNode();
-    if (node && originalIsText)
-        node = node->parentNode();
-    if (!node)
-        return NoCursorChange;
-
-    RenderObject* renderer = node->renderer();
+    Node* node = event.targetNode();
+    RenderObject* renderer = node ? node->renderer() : 0;
     RenderStyle* style = renderer ? renderer->style() : 0;
     bool horizontalText = !style || style->isHorizontalWritingMode();
     const Cursor& iBeam = horizontalText ? iBeamCursor() : verticalTextCursor();
@@ -1308,7 +1267,7 @@ OptionalCursor EventHandler::selectCursor(const HitTestResult& result, bool shif
 
     if (renderer) {
         Cursor overrideCursor;
-        switch (renderer->getCursor(roundedIntPoint(result.localPoint()), overrideCursor)) {
+        switch (renderer->getCursor(roundedIntPoint(event.localPoint()), overrideCursor)) {
         case SetCursorBasedOnStyle:
             break;
         case SetCursor:
@@ -1355,19 +1314,19 @@ OptionalCursor EventHandler::selectCursor(const HitTestResult& result, bool shif
 
     switch (style ? style->cursor() : CURSOR_AUTO) {
     case CURSOR_AUTO: {
-        bool editable = (node->rendererIsEditable());
+        bool editable = (node && node->rendererIsEditable());
 
-        if (useHandCursor(node, result.URLElement() && result.URLElement()->isLink(), shiftKey))
+        if (useHandCursor(node, event.isOverLink(), event.event().shiftKey()))
             return handCursor();
 
         bool inResizer = false;
         if (renderer) {
             if (RenderLayer* layer = renderer->enclosingLayer()) {
                 if (FrameView* view = m_frame->view())
-                    inResizer = layer->isPointInResizeControl(view->windowToContents(roundedIntPoint(result.localPoint())));
+                    inResizer = layer->isPointInResizeControl(view->windowToContents(event.event().position()));
             }
         }
-        if ((editable || (originalIsText && node->canStartSelection())) && !inResizer && !result.scrollbar())
+        if ((editable || (renderer && renderer->isText() && node->canStartSelection())) && !inResizer && !scrollbar)
             return iBeam;
         return pointerCursor();
     }
@@ -1778,7 +1737,7 @@ bool EventHandler::handleMouseMoveEvent(const PlatformMouseEvent& mouseEvent, Hi
         if (scrollbar && !m_mousePressed)
             scrollbar->mouseMoved(mouseEvent); // Handle hover effects on platforms that support visual feedback on scrollbar hovering.
         if (FrameView* view = m_frame->view()) {
-            OptionalCursor optionalCursor = selectCursor(mev.hitTestResult(), mouseEvent.shiftKey());
+            OptionalCursor optionalCursor = selectCursor(mev, scrollbar);
             if (optionalCursor.isCursorChange()) {
                 m_currentMouseCursor = optionalCursor.cursor();
                 view->setCursor(m_currentMouseCursor);
index 30a3f72..3b83602 100644 (file)
@@ -251,7 +251,6 @@ public:
 #endif
 
     bool useHandCursor(Node*, bool isOverLink, bool shiftKey);
-    void updateCursor();
 
 private:
 #if ENABLE(DRAG_SUPPORT)
@@ -278,8 +277,7 @@ private:
 #endif
     bool handleMouseReleaseEvent(const MouseEventWithHitTestResults&);
 
-    OptionalCursor selectCursor(const HitTestResult&, bool shiftKey);
-
+    OptionalCursor selectCursor(const MouseEventWithHitTestResults&, Scrollbar*);
     void hoverTimerFired(Timer<EventHandler>*);
 
     bool logicalScrollOverflow(ScrollLogicalDirection, ScrollGranularity, Node* startingNode = 0);
index 61c20a2..f0ec4d5 100644 (file)
@@ -1758,17 +1758,6 @@ void RenderObject::setPseudoStyle(PassRefPtr<RenderStyle> pseudoStyle)
     setStyle(pseudoStyle);
 }
 
-static bool areNonIdenticalCursorListsEqual(const RenderStyle* a, const RenderStyle* b)
-{
-    ASSERT(a->cursors() != b->cursors());
-    return a->cursors() && b->cursors() && *a->cursors() == *b->cursors();
-}
-
-static inline bool areCursorsEqual(const RenderStyle* a, const RenderStyle* b)
-{
-    return a->cursor() == b->cursor() && (a->cursors() == b->cursors() || areNonIdenticalCursorListsEqual(a, b));
-}
-
 void RenderObject::setStyle(PassRefPtr<RenderStyle> style)
 {
     if (m_style == style) {
@@ -1807,11 +1796,6 @@ void RenderObject::setStyle(PassRefPtr<RenderStyle> style)
 
     styleDidChange(diff, oldStyle.get());
 
-    if (oldStyle.get() && !areCursorsEqual(oldStyle.get(), this->style())) {
-        if (Frame* frame = this->frame())
-            frame->eventHandler()->updateCursor();
-    }
-
     // FIXME: |this| might be destroyed here. This can currently happen for a RenderTextFragment when
     // its first-letter block gets an update in RenderTextFragment::styleDidChange. For RenderTextFragment(s),
     // we will safely bail out with the doesNotNeedLayout flag. We might want to broaden this condition
@@ -1937,6 +1921,17 @@ void RenderObject::styleWillChange(StyleDifference diff, const RenderStyle* newS
     }
 }
 
+static bool areNonIdenticalCursorListsEqual(const RenderStyle* a, const RenderStyle* b)
+{
+    ASSERT(a->cursors() != b->cursors());
+    return a->cursors() && b->cursors() && *a->cursors() == *b->cursors();
+}
+
+static inline bool areCursorsEqual(const RenderStyle* a, const RenderStyle* b)
+{
+    return a->cursor() == b->cursor() && (a->cursors() == b->cursors() || areNonIdenticalCursorListsEqual(a, b));
+}
+
 void RenderObject::styleDidChange(StyleDifference diff, const RenderStyle* oldStyle)
 {
 
@@ -1970,6 +1965,11 @@ void RenderObject::styleDidChange(StyleDifference diff, const RenderStyle* oldSt
 
     // Don't check for repaint here; we need to wait until the layer has been
     // updated by subclasses before we know if we have to repaint (in setStyle()).
+
+    if (oldStyle && !areCursorsEqual(oldStyle, style())) {
+        if (Frame* frame = this->frame())
+            frame->eventHandler()->dispatchFakeMouseMoveEventSoon();
+    }
 }
 
 void RenderObject::propagateStyleToAnonymousChildren(bool blockChildrenOnly)