Source/WebCore: Updating mouse cursor on style changes without emitting fake mousemov...
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 14 Feb 2013 10:14:24 +0000 (10:14 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 14 Feb 2013 10:14:24 +0000 (10:14 +0000)
https://bugs.webkit.org/show_bug.cgi?id=101857

Patch by Aivo Paas <aivopaas@gmail.com> on 2013-02-14
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):

LayoutTests: 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

Patch by Aivo Paas <aivopaas@gmail.com> on 2013-02-14
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:

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

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

index 8071b541b948c3f051de78e125d46cfdad9b382f..1f1f3ce977e0b4b198587d4e7c9ec2baf73546d1 100644 (file)
@@ -1,3 +1,22 @@
+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
new file mode 100644 (file)
index 0000000..48b7abe
--- /dev/null
@@ -0,0 +1,24 @@
+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
new file mode 100644 (file)
index 0000000..0d41be1
--- /dev/null
@@ -0,0 +1,78 @@
+<!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
new file mode 100644 (file)
index 0000000..a5d0645
--- /dev/null
@@ -0,0 +1,16 @@
+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
new file mode 100644 (file)
index 0000000..c459821
--- /dev/null
@@ -0,0 +1,55 @@
+<!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 42136f531ccefe47e259b8696400238afdf58cb4..8f43592aab98ea35c67d61e59d1af9a4d444ccc3 100644 (file)
@@ -1412,3 +1412,6 @@ 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
index 4768ffc54542a338669ff92b7f1929a7304072d8..f1648f74c0be8c67d19c507d4d10b2bb65443cd3 100644 (file)
@@ -1,3 +1,53 @@
+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 fb631f7b11b8877e50f825e6e1a8737915c816df..67776fc425e236e75de25ba988b67e915d132317 100644 (file)
@@ -1237,7 +1237,40 @@ bool EventHandler::useHandCursor(Node* node, bool isOverLink, bool shiftKey)
     return ((isOverLink || isSubmitImage(node)) && (!editable || editableLinkEnabled));
 }
 
-OptionalCursor EventHandler::selectCursor(const MouseEventWithHitTestResults& event, Scrollbar* scrollbar)
+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)
 {
     if (m_resizeLayer && m_resizeLayer->inResizeMode())
         return NoCursorChange;
@@ -1250,8 +1283,16 @@ OptionalCursor EventHandler::selectCursor(const MouseEventWithHitTestResults& ev
         return NoCursorChange;
 #endif
 
-    Node* node = event.targetNode();
-    RenderObject* renderer = node ? node->renderer() : 0;
+    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();
     RenderStyle* style = renderer ? renderer->style() : 0;
     bool horizontalText = !style || style->isHorizontalWritingMode();
     const Cursor& iBeam = horizontalText ? iBeamCursor() : verticalTextCursor();
@@ -1267,7 +1308,7 @@ OptionalCursor EventHandler::selectCursor(const MouseEventWithHitTestResults& ev
 
     if (renderer) {
         Cursor overrideCursor;
-        switch (renderer->getCursor(roundedIntPoint(event.localPoint()), overrideCursor)) {
+        switch (renderer->getCursor(roundedIntPoint(result.localPoint()), overrideCursor)) {
         case SetCursorBasedOnStyle:
             break;
         case SetCursor:
@@ -1314,19 +1355,19 @@ OptionalCursor EventHandler::selectCursor(const MouseEventWithHitTestResults& ev
 
     switch (style ? style->cursor() : CURSOR_AUTO) {
     case CURSOR_AUTO: {
-        bool editable = (node && node->rendererIsEditable());
+        bool editable = (node->rendererIsEditable());
 
-        if (useHandCursor(node, event.isOverLink(), event.event().shiftKey()))
+        if (useHandCursor(node, result.URLElement() && result.URLElement()->isLink(), shiftKey))
             return handCursor();
 
         bool inResizer = false;
         if (renderer) {
             if (RenderLayer* layer = renderer->enclosingLayer()) {
                 if (FrameView* view = m_frame->view())
-                    inResizer = layer->isPointInResizeControl(view->windowToContents(event.event().position()));
+                    inResizer = layer->isPointInResizeControl(view->windowToContents(roundedIntPoint(result.localPoint())));
             }
         }
-        if ((editable || (renderer && renderer->isText() && node->canStartSelection())) && !inResizer && !scrollbar)
+        if ((editable || (originalIsText && node->canStartSelection())) && !inResizer && !result.scrollbar())
             return iBeam;
         return pointerCursor();
     }
@@ -1737,7 +1778,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, scrollbar);
+            OptionalCursor optionalCursor = selectCursor(mev.hitTestResult(), mouseEvent.shiftKey());
             if (optionalCursor.isCursorChange()) {
                 m_currentMouseCursor = optionalCursor.cursor();
                 view->setCursor(m_currentMouseCursor);
index 3b8360225b58e265a55eb78d901e77c4d3b44ef9..30a3f72bea989edf035e3b010819b6936502ff82 100644 (file)
@@ -251,6 +251,7 @@ public:
 #endif
 
     bool useHandCursor(Node*, bool isOverLink, bool shiftKey);
+    void updateCursor();
 
 private:
 #if ENABLE(DRAG_SUPPORT)
@@ -277,7 +278,8 @@ private:
 #endif
     bool handleMouseReleaseEvent(const MouseEventWithHitTestResults&);
 
-    OptionalCursor selectCursor(const MouseEventWithHitTestResults&, Scrollbar*);
+    OptionalCursor selectCursor(const HitTestResult&, bool shiftKey);
+
     void hoverTimerFired(Timer<EventHandler>*);
 
     bool logicalScrollOverflow(ScrollLogicalDirection, ScrollGranularity, Node* startingNode = 0);
index f0ec4d5046142830bcbb657c31be6c93017ad937..61c20a2f816d9e71c552caee8bc71f368ce9f5db 100644 (file)
@@ -1758,6 +1758,17 @@ 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) {
@@ -1796,6 +1807,11 @@ 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
@@ -1921,17 +1937,6 @@ 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)
 {
 
@@ -1965,11 +1970,6 @@ 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)