- Revision
- 142861
- Author
- commit-qu...@webkit.org
- Date
- 2013-02-14 02:14:24 -0800 (Thu, 14 Feb 2013)
Log Message
Source/WebCore: Updating mouse cursor on style changes without emitting fake mousemove event
https://bugs.webkit.org/show_bug.cgi?id=101857
Patch by Aivo Paas <aivop...@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 <aivop...@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:
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (142860 => 142861)
--- trunk/LayoutTests/ChangeLog 2013-02-14 09:16:17 UTC (rev 142860)
+++ trunk/LayoutTests/ChangeLog 2013-02-14 10:14:24 UTC (rev 142861)
@@ -1,3 +1,22 @@
+2013-02-14 Aivo Paas <aivop...@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 <g...@chromium.org>
Adds the WebGL Conformance Tests limits folder.
Added: trunk/LayoutTests/fast/events/mouse-cursor-change-expected.txt (0 => 142861)
--- trunk/LayoutTests/fast/events/mouse-cursor-change-expected.txt (rev 0)
+++ trunk/LayoutTests/fast/events/mouse-cursor-change-expected.txt 2013-02-14 10:14:24 UTC (rev 142861)
@@ -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
+
Added: trunk/LayoutTests/fast/events/mouse-cursor-change.html (0 => 142861)
--- trunk/LayoutTests/fast/events/mouse-cursor-change.html (rev 0)
+++ trunk/LayoutTests/fast/events/mouse-cursor-change.html 2013-02-14 10:14:24 UTC (rev 142861)
@@ -0,0 +1,78 @@
+<!DOCTYPE html>
+<html>
+<head>
+<script src=""
+<style type="text/css">
+</style>
+</head>
+<body>
+<p id="description"></p>
+<p><a href="" 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=""
+</body>
+</html>
Added: trunk/LayoutTests/fast/events/mouse-cursor-no-mousemove-expected.txt (0 => 142861)
--- trunk/LayoutTests/fast/events/mouse-cursor-no-mousemove-expected.txt (rev 0)
+++ trunk/LayoutTests/fast/events/mouse-cursor-no-mousemove-expected.txt 2013-02-14 10:14:24 UTC (rev 142861)
@@ -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
+
Added: trunk/LayoutTests/fast/events/mouse-cursor-no-mousemove.html (0 => 142861)
--- trunk/LayoutTests/fast/events/mouse-cursor-no-mousemove.html (rev 0)
+++ trunk/LayoutTests/fast/events/mouse-cursor-no-mousemove.html 2013-02-14 10:14:24 UTC (rev 142861)
@@ -0,0 +1,55 @@
+<!DOCTYPE html>
+<html>
+<head>
+<script src=""
+<style type="text/css">
+</style>
+</head>
+<body>
+<p id="description"></p>
+<p><a href="" 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=""
+</body>
+</html>
Modified: trunk/LayoutTests/platform/mac/TestExpectations (142860 => 142861)
--- trunk/LayoutTests/platform/mac/TestExpectations 2013-02-14 09:16:17 UTC (rev 142860)
+++ trunk/LayoutTests/platform/mac/TestExpectations 2013-02-14 10:14:24 UTC (rev 142861)
@@ -1412,3 +1412,6 @@
# 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
Modified: trunk/Source/WebCore/ChangeLog (142860 => 142861)
--- trunk/Source/WebCore/ChangeLog 2013-02-14 09:16:17 UTC (rev 142860)
+++ trunk/Source/WebCore/ChangeLog 2013-02-14 10:14:24 UTC (rev 142861)
@@ -1,3 +1,53 @@
+2013-02-14 Aivo Paas <aivop...@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 <loi...@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.
Modified: trunk/Source/WebCore/page/EventHandler.cpp (142860 => 142861)
--- trunk/Source/WebCore/page/EventHandler.cpp 2013-02-14 09:16:17 UTC (rev 142860)
+++ trunk/Source/WebCore/page/EventHandler.cpp 2013-02-14 10:14:24 UTC (rev 142861)
@@ -1237,8 +1237,41 @@
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 @@
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 @@
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 @@
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 @@
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);
Modified: trunk/Source/WebCore/page/EventHandler.h (142860 => 142861)
--- trunk/Source/WebCore/page/EventHandler.h 2013-02-14 09:16:17 UTC (rev 142860)
+++ trunk/Source/WebCore/page/EventHandler.h 2013-02-14 10:14:24 UTC (rev 142861)
@@ -251,6 +251,7 @@
#endif
bool useHandCursor(Node*, bool isOverLink, bool shiftKey);
+ void updateCursor();
private:
#if ENABLE(DRAG_SUPPORT)
@@ -277,7 +278,8 @@
#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);
Modified: trunk/Source/WebCore/rendering/RenderObject.cpp (142860 => 142861)
--- trunk/Source/WebCore/rendering/RenderObject.cpp 2013-02-14 09:16:17 UTC (rev 142860)
+++ trunk/Source/WebCore/rendering/RenderObject.cpp 2013-02-14 10:14:24 UTC (rev 142861)
@@ -1758,6 +1758,17 @@
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 @@
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 @@
}
}
-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 @@
// 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)
