Shift clicking on an element with -webkit-user-select: all doesn't extend selection
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 28 Mar 2013 00:01:41 +0000 (00:01 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 28 Mar 2013 00:01:41 +0000 (00:01 +0000)
https://bugs.webkit.org/show_bug.cgi?id=113270

Reviewed by Enrica Casucci.

Source/WebCore:

The bug was caused by updateSelectionForMouseDownDispatchingSelectStart always replacing selection whenever
the target node was inside an element with -webkit-suer-select even when we were attemping to extend selection
in handleMousePressEventSingleClick.

Fixed the bug by extracting this logic as a separate function (expandSelectionToRespectUserSelectAll) and deploying
it in handleMousePressEventSingleClick to extend selection as needed.

Test: editing/selection/user-select-all-with-shift.html

* page/EventHandler.cpp:
(WebCore::expandSelectionToRespectUserSelectAll): Extracted from updateSelectionForMouseDownDispatchingSelectStart.
(WebCore::EventHandler::updateSelectionForMouseDownDispatchingSelectStart):
(WebCore::EventHandler::selectClosestWordFromHitTestResult):
(WebCore::EventHandler::selectClosestWordOrLinkFromMouseEvent):
(WebCore::EventHandler::handleMousePressEventTripleClick):
(WebCore::EventHandler::handleMousePressEventSingleClick): Adjust "pos" as needed when extending selection.
Also use shouldConsiderSelectionAsDirectional() instead of manually peeking editingBehaviorType while we're at it.

LayoutTests:

Added a regression test for shift clicking on an element with -webkit-user-select: all.
Skip it on non-Mac platforms as -webkit-user-select: all hasn't been enabled on them.

* editing/selection/user-select-all-with-shift-expected.txt: Added.
* editing/selection/user-select-all-with-shift.html: Added.
* platform/chromium/TestExpectations:
* platform/efl/TestExpectations:
* platform/gtk/TestExpectations:
* platform/qt/TestExpectations:
* platform/win/TestExpectations:
* platform/wincairo/TestExpectations:

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

LayoutTests/ChangeLog
LayoutTests/editing/selection/user-select-all-with-shift-expected.txt [new file with mode: 0644]
LayoutTests/editing/selection/user-select-all-with-shift.html [new file with mode: 0644]
LayoutTests/platform/chromium/TestExpectations
LayoutTests/platform/efl/TestExpectations
LayoutTests/platform/gtk/TestExpectations
LayoutTests/platform/qt/TestExpectations
LayoutTests/platform/win/TestExpectations
LayoutTests/platform/wincairo/TestExpectations
Source/WebCore/ChangeLog
Source/WebCore/page/EventHandler.cpp

index c2cc018..6294921 100644 (file)
@@ -1,3 +1,22 @@
+2013-03-27  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Shift clicking on an element with -webkit-user-select: all doesn't extend selection
+        https://bugs.webkit.org/show_bug.cgi?id=113270
+
+        Reviewed by Enrica Casucci.
+
+        Added a regression test for shift clicking on an element with -webkit-user-select: all.
+        Skip it on non-Mac platforms as -webkit-user-select: all hasn't been enabled on them.
+
+        * editing/selection/user-select-all-with-shift-expected.txt: Added.
+        * editing/selection/user-select-all-with-shift.html: Added.
+        * platform/chromium/TestExpectations:
+        * platform/efl/TestExpectations:
+        * platform/gtk/TestExpectations:
+        * platform/qt/TestExpectations:
+        * platform/win/TestExpectations:
+        * platform/wincairo/TestExpectations:
+
 2013-03-27  Zalan Bujtas  <zalan@apple.com>
 
         REGRESSION(r143102): iframe with percentage height within table with anonymous cell fails.
diff --git a/LayoutTests/editing/selection/user-select-all-with-shift-expected.txt b/LayoutTests/editing/selection/user-select-all-with-shift-expected.txt
new file mode 100644 (file)
index 0000000..e831931
--- /dev/null
@@ -0,0 +1,206 @@
+This tests shift + selecting two discontinuous elements with user-select: all. WebKit should select the both elements instead of moving the selection.
+To manually test, click to select the first element and shift-click the second element. WebKit should select both elements.
+
+After clicking on the first element (Mac):
+| "
+"
+| <div>
+|   class="select-all"
+|   id="first"
+|   "<#selection-anchor>First element<#selection-focus>"
+| "
+Some other text.
+"
+| <div>
+|   class="select-all"
+|   id="second"
+|   "Second element"
+| "
+"
+
+After shift clicking on the second element (Mac):
+| "
+"
+| <div>
+|   class="select-all"
+|   id="first"
+|   "<#selection-anchor>First element"
+| "
+Some other text.
+"
+| <div>
+|   class="select-all"
+|   id="second"
+|   "Second element<#selection-focus>"
+| "
+"
+
+After clicking on the second element (Mac):
+| "
+"
+| <div>
+|   class="select-all"
+|   id="first"
+|   "First element"
+| "
+Some other text.
+"
+| <div>
+|   class="select-all"
+|   id="second"
+|   "<#selection-anchor>Second element<#selection-focus>"
+| "
+"
+
+After shift clicking on the first element (Mac):
+| "
+"
+| <div>
+|   class="select-all"
+|   id="first"
+|   "<#selection-focus>First element"
+| "
+Some other text.
+"
+| <div>
+|   class="select-all"
+|   id="second"
+|   "Second element<#selection-anchor>"
+| "
+"
+
+After clicking on the first element (Win):
+| "
+"
+| <div>
+|   class="select-all"
+|   id="first"
+|   "<#selection-focus>First element"
+| "
+Some other text.
+"
+| <div>
+|   class="select-all"
+|   id="second"
+|   "Second element<#selection-anchor>"
+| "
+"
+
+After shift clicking on the second element (Win):
+| "
+"
+| <div>
+|   class="select-all"
+|   id="first"
+|   "First element"
+| "
+Some other text.
+"
+| <div>
+|   class="select-all"
+|   id="second"
+|   "<#selection-focus>Second element<#selection-anchor>"
+| "
+"
+
+After clicking on the second element (Win):
+| "
+"
+| <div>
+|   class="select-all"
+|   id="first"
+|   "First element"
+| "
+Some other text.
+"
+| <div>
+|   class="select-all"
+|   id="second"
+|   "<#selection-anchor>Second element<#selection-focus>"
+| "
+"
+
+After shift clicking on the first element (Win):
+| "
+"
+| <div>
+|   class="select-all"
+|   id="first"
+|   "<#selection-focus>First element"
+| "
+Some other text.
+"
+| <div>
+|   class="select-all"
+|   id="second"
+|   "Second<#selection-anchor> element"
+| "
+"
+
+After clicking on the first element (Unix):
+| "
+"
+| <div>
+|   class="select-all"
+|   id="first"
+|   "<#selection-focus>First element"
+| "
+Some other text.
+"
+| <div>
+|   class="select-all"
+|   id="second"
+|   "Second<#selection-anchor> element"
+| "
+"
+
+After shift clicking on the second element (Unix):
+| "
+"
+| <div>
+|   class="select-all"
+|   id="first"
+|   "First element"
+| "
+Some other text.
+"
+| <div>
+|   class="select-all"
+|   id="second"
+|   "<#selection-anchor>Second element<#selection-focus>"
+| "
+"
+
+After clicking on the second element (Unix):
+| "
+"
+| <div>
+|   class="select-all"
+|   id="first"
+|   "First element"
+| "
+Some other text.
+"
+| <div>
+|   class="select-all"
+|   id="second"
+|   "<#selection-anchor>Second element<#selection-focus>"
+| "
+"
+
+After shift clicking on the first element (Unix):
+| "
+"
+| <div>
+|   class="select-all"
+|   id="first"
+|   "<#selection-focus>First element"
+| "
+Some other text.
+"
+| <div>
+|   class="select-all"
+|   id="second"
+|   "Second<#selection-anchor> element"
+| "
+"
diff --git a/LayoutTests/editing/selection/user-select-all-with-shift.html b/LayoutTests/editing/selection/user-select-all-with-shift.html
new file mode 100644 (file)
index 0000000..880e23b
--- /dev/null
@@ -0,0 +1,61 @@
+<!DOCTYPE html>
+<html>
+<body>
+<style>
+.select-all {
+    border: 1px solid black;
+    height: 100px;
+    width: 100px;
+    -webkit-user-select: all;
+    -moz-user-select: all;
+}
+</style>
+<p id="description">This tests shift + selecting two discontinuous elements with user-select: all. WebKit should select the both elements instead of moving the selection.
+To manually test, click to select the first element and shift-click the second element. WebKit should select both elements.</p>
+<div id="test">
+<div id="first" class="select-all">First element</div>
+Some other text.
+<div id="second" class="select-all">Second element</div>
+</div>
+<script src="../../resources/dump-as-markup.js"></script>
+<script>
+
+Markup.description(document.getElementById('description').textContent);
+
+function clickOnElement(element, keys) {
+    eventSender.mouseMoveTo(element.offsetLeft + 10, element.offsetTop + 10);
+    eventSender.mouseDown(0, keys);
+    eventSender.mouseUp(0, keys);
+}
+
+function runTest(editingBehavior) {
+    internals.settings.setEditingBehavior(editingBehavior);
+
+    var postfix = ' (' + editingBehavior + ')';
+
+    clickOnElement(document.getElementById('first'));
+    Markup.dump('test', 'After clicking on the first element' + postfix);
+    eventSender.leapForward(300);
+    clickOnElement(document.getElementById('second'), ['shiftKey']);
+    Markup.dump('test', 'After shift clicking on the second element' + postfix);
+
+    getSelection().removeAllRanges();
+
+    clickOnElement(document.getElementById('second'));
+    Markup.dump('test', 'After clicking on the second element' + postfix);
+    eventSender.leapForward(300);
+    clickOnElement(document.getElementById('first'), ['shiftKey']);
+    Markup.dump('test', 'After shift clicking on the first element' + postfix);
+}
+
+if (window.eventSender) {
+    runTest('Mac');
+    runTest('Win');
+    runTest('Unix');
+
+} else
+    Markup.noAutoDump();
+
+</script>
+</body>
+</html>
index 4bc1d67..8b5eff7 100644 (file)
@@ -3477,6 +3477,7 @@ webkit.org/b/100023 [ Mac ] fast/sub-pixel/file-upload-control-at-fractional-off
 webkit.org/b/100478  [ Mac ] css3/filters/custom/effect-custom.html [ ImageOnlyFailure ]
 
 webkit.org/b/100424 editing/selection/user-select-all-selection.html [ Failure ]
+webkit.org/b/100424 editing/selection/user-select-all-with-shift.html [ Failure ]
 webkit.org/b/100475 compositing/tiling/tile-cache-zoomed.html [ Failure ]
 webkit.org/b/100475 platform/chromium/virtual/softwarecompositing/tiling/tile-cache-zoomed.html [ Failure ]
 
index dda0f66..77e77fb 100644 (file)
@@ -1530,6 +1530,7 @@ webkit.org/b/99691 gamepad/gamepad-out-of-range-crasher.html [ Failure ]
 webkit.org/b/61138 http/tests/w3c/webperf/submission/Intel/resource-timing [ Skip ]
 
 webkit.org/b/100424 editing/selection/user-select-all-selection.html [ Failure ]
+webkit.org/b/100424 editing/selection/user-select-all-with-shift.html [ Failure ]
 
 # Entering into full screen playback mode fails when triggered by context menu
 Bug(EFL) media/context-menu-actions.html [ Failure ]
index 639d1f6..f66020f 100644 (file)
@@ -1234,6 +1234,7 @@ webkit.org/b/99825 accessibility/title-ui-element-correctness.html [ Failure ]
 webkit.org/b/98718 svg/animations/animate-css-xml-attributeType.html [ Failure ]
 
 webkit.org/b/100424 editing/selection/user-select-all-selection.html [ Failure ]
+webkit.org/b/100424 editing/selection/user-select-all-with-shift.html [ Failure ]
 
 webkit.org/b/100846 inspector-protocol/debugger-pause-dedicated-worker.html [ Skip ]
 webkit.org/b/100846 inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html [ Skip ]
index cd1cbe4..70063cd 100644 (file)
@@ -2569,6 +2569,7 @@ webkit.org/b/102226 fast/dom/Window/open-window-min-size.html
 webkit.org/b/100196 http/tests/inspector/network/image-as-text-loading-data-url.html
 
 webkit.org/b/100424 editing/selection/user-select-all-selection.html [ Failure ]
+webkit.org/b/100424 editing/selection/user-select-all-with-shift.html [ Failure ]
 
 webkit.org/b/101539 editing/execCommand/switch-list-type-with-orphaned-li.html [ Failure ]
 
index a6d1650..876eff4 100644 (file)
@@ -2445,6 +2445,7 @@ webkit.org/b/84893 http/tests/w3c/webperf/submission/Intel/user-timing
 webkit.org/b/61138 http/tests/w3c/webperf/submission/Intel/resource-timing
 
 webkit.org/b/100424 editing/selection/user-select-all-selection.html [ Failure ]
+webkit.org/b/100424 editing/selection/user-select-all-with-shift.html [ Failure ]
 
 # https://bugs.webkit.org/show_bug.cgi?id=99567
 # Not supported on Mac or Windows ports
index ed62851..060abe6 100644 (file)
@@ -2930,6 +2930,7 @@ webkit.org/b/84893 http/tests/w3c/webperf/submission/Intel/user-timing
 webkit.org/b/61138 http/tests/w3c/webperf/submission/Intel/resource-timing
 
 webkit.org/b/100424 editing/selection/user-select-all-selection.html [ Failure ]
+webkit.org/b/100424 editing/selection/user-select-all-with-shift.html [ Failure ]
 
 # https://bugs.webkit.org/show_bug.cgi?id=99567
 # Not supported on Mac or Windows ports
index d872488..f344b4d 100644 (file)
@@ -1,3 +1,28 @@
+2013-03-27  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Shift clicking on an element with -webkit-user-select: all doesn't extend selection
+        https://bugs.webkit.org/show_bug.cgi?id=113270
+
+        Reviewed by Enrica Casucci.
+
+        The bug was caused by updateSelectionForMouseDownDispatchingSelectStart always replacing selection whenever
+        the target node was inside an element with -webkit-suer-select even when we were attemping to extend selection
+        in handleMousePressEventSingleClick.
+
+        Fixed the bug by extracting this logic as a separate function (expandSelectionToRespectUserSelectAll) and deploying
+        it in handleMousePressEventSingleClick to extend selection as needed.
+
+        Test: editing/selection/user-select-all-with-shift.html
+
+        * page/EventHandler.cpp:
+        (WebCore::expandSelectionToRespectUserSelectAll): Extracted from updateSelectionForMouseDownDispatchingSelectStart.
+        (WebCore::EventHandler::updateSelectionForMouseDownDispatchingSelectStart):
+        (WebCore::EventHandler::selectClosestWordFromHitTestResult):
+        (WebCore::EventHandler::selectClosestWordOrLinkFromMouseEvent):
+        (WebCore::EventHandler::handleMousePressEventTripleClick):
+        (WebCore::EventHandler::handleMousePressEventSingleClick): Adjust "pos" as needed when extending selection.
+        Also use shouldConsiderSelectionAsDirectional() instead of manually peeking editingBehaviorType while we're at it.
+
 2013-03-27  Zalan Bujtas  <zalan@apple.com>
 
         REGRESSION(r143102): iframe with percentage height within table with anonymous cell fails.
index 455440e..5ecd10b 100644 (file)
@@ -442,7 +442,24 @@ static inline bool dispatchSelectStart(Node* node)
     return node->dispatchEvent(Event::create(eventNames().selectstartEvent, true, true));
 }
 
-bool EventHandler::updateSelectionForMouseDownDispatchingSelectStart(Node* targetNode, const VisibleSelection& newSelection, TextGranularity granularity)
+static VisibleSelection expandSelectionToRespectUserSelectAll(Node* targetNode, const VisibleSelection& selection)
+{
+#if ENABLE(USERSELECT_ALL)
+    Node* rootUserSelectAll = Position::rootUserSelectAllForNode(targetNode);
+    if (!rootUserSelectAll)
+        return selection;
+
+    VisibleSelection newSelection(selection);
+    newSelection.setBase(positionBeforeNode(rootUserSelectAll).upstream(CanCrossEditingBoundary));
+    newSelection.setExtent(positionAfterNode(rootUserSelectAll).downstream(CanCrossEditingBoundary));
+
+    return newSelection;
+#else
+    return selection;
+#endif
+}
+
+bool EventHandler::updateSelectionForMouseDownDispatchingSelectStart(Node* targetNode, const VisibleSelection& selection, TextGranularity granularity)
 {
     if (Position::nodeIsUserSelectNone(targetNode))
         return false;
@@ -450,13 +467,6 @@ bool EventHandler::updateSelectionForMouseDownDispatchingSelectStart(Node* targe
     if (!dispatchSelectStart(targetNode))
         return false;
 
-    VisibleSelection selection(newSelection);
-#if ENABLE(USERSELECT_ALL)
-    if (Node* rootUserSelectAll = Position::rootUserSelectAllForNode(targetNode)) {
-        selection.setBase(positionBeforeNode(rootUserSelectAll).upstream(CanCrossEditingBoundary));
-        selection.setExtent(positionAfterNode(rootUserSelectAll).downstream(CanCrossEditingBoundary));
-    }
-#endif
     if (selection.isRange())
         m_selectionInitiationState = ExtendedSelection;
     else {
@@ -484,7 +494,7 @@ void EventHandler::selectClosestWordFromHitTestResult(const HitTestResult& resul
         if (appendTrailingWhitespace == ShouldAppendTrailingWhitespace && newSelection.isRange())
             newSelection.appendTrailingWhitespace();
 
-        updateSelectionForMouseDownDispatchingSelectStart(innerNode, newSelection, WordGranularity);
+        updateSelectionForMouseDownDispatchingSelectStart(innerNode, expandSelectionToRespectUserSelectAll(innerNode, newSelection), WordGranularity);
     }
 }
 
@@ -510,7 +520,7 @@ void EventHandler::selectClosestWordOrLinkFromMouseEvent(const MouseEventWithHit
         if (pos.isNotNull() && pos.deepEquivalent().deprecatedNode()->isDescendantOf(URLElement))
             newSelection = VisibleSelection::selectionFromContentsOfNode(URLElement);
 
-        updateSelectionForMouseDownDispatchingSelectStart(innerNode, newSelection, WordGranularity);
+        updateSelectionForMouseDownDispatchingSelectStart(innerNode, expandSelectionToRespectUserSelectAll(innerNode, newSelection), WordGranularity);
     }
 }
 
@@ -548,7 +558,7 @@ bool EventHandler::handleMousePressEventTripleClick(const MouseEventWithHitTestR
         newSelection.expandUsingGranularity(ParagraphGranularity);
     }
 
-    return updateSelectionForMouseDownDispatchingSelectStart(innerNode, newSelection, ParagraphGranularity);
+    return updateSelectionForMouseDownDispatchingSelectStart(innerNode, expandSelectionToRespectUserSelectAll(innerNode, newSelection), ParagraphGranularity);
 }
 
 static int textDistance(const Position& start, const Position& end)
@@ -586,8 +596,15 @@ bool EventHandler::handleMousePressEventSingleClick(const MouseEventWithHitTestR
     TextGranularity granularity = CharacterGranularity;
 
     if (extendSelection && newSelection.isCaretOrRange()) {
-        ASSERT(m_frame->settings());
-        if (m_frame->settings()->editingBehaviorType() == EditingMacBehavior) {
+        VisibleSelection selectionInUserSelectAll = expandSelectionToRespectUserSelectAll(innerNode, VisibleSelection(pos));
+        if (selectionInUserSelectAll.isRange()) {
+            if (comparePositions(selectionInUserSelectAll.start(), newSelection.start()) < 0)
+                pos = selectionInUserSelectAll.start();
+            else if (comparePositions(newSelection.end(), selectionInUserSelectAll.end()) < 0)
+                pos = selectionInUserSelectAll.end();
+        }
+
+        if (!m_frame->editor()->behavior().shouldConsiderSelectionAsDirectional()) {
             // See <rdar://problem/3668157> REGRESSION (Mail): shift-click deselects when selection
             // was created right-to-left
             Position start = newSelection.start();
@@ -606,8 +623,8 @@ bool EventHandler::handleMousePressEventSingleClick(const MouseEventWithHitTestR
             newSelection.expandUsingGranularity(m_frame->selection()->granularity());
         }
     } else
-        newSelection = VisibleSelection(visiblePos);
-    
+        newSelection = expandSelectionToRespectUserSelectAll(innerNode, visiblePos);
+
     bool handled = updateSelectionForMouseDownDispatchingSelectStart(innerNode, newSelection, granularity);
 
     if (event.event().button() == MiddleButton) {