- Revision
- 264690
- Author
- wenson_hs...@apple.com
- Date
- 2020-07-21 19:47:29 -0700 (Tue, 21 Jul 2020)
Log Message
REGRESSION (r258871): Shift + click to extend selection loses currently selected text
https://bugs.webkit.org/show_bug.cgi?id=214617
<rdar://problem/64980223>
Reviewed by Megan Gardner.
Source/WebCore:
After the changes in r258871, shift clicking sometimes fails to preserve the existing selected text range on
macOS and iOS. The logic in `EventHandler::handleMousePressEventSingleClick` uses the `textDistance` helper
method to count the number of characters between the start of the current selection to the newly selected
extent, as well as the number of characters between the end of the current selection and the newly selected
extent position. It compares these two character counts, and attempts to choose the new selection extents in
such a way that maximizes the amount of selected text.
However, after r258871, `textDistance` uses `characterCount` instead of `TextIterator::rangeLength`. Unlike the
former, `rangeLength` is robust in the case where the start position comes after the end position (in document
order), since the process of creating a live `Range` object swaps the start and end if needed. This isn't the
case when using `SimpleRange`. Instead, when given a simple range where the start comes after the end,
`characterCount` will iterate text in the DOM, starting from the start position and ending at the end of the
document rather than the end position. The result is that `characterCount` actually counts the number of
characters between the start position and the end of the document, rather than the number of characters between
the two positions.
In the context of this bug, if the start of the current selection is "far away" (in terms of character count)
from the end of the document and the new extent position comes after end of the current selection, we will end
up choosing the end (instead of the start) as one of the new extents of the updated selection.
To fix this (as well as other similar issues that might've arisen when replacing uses of
`TextIterator::rangeLength` with `characterCount`), simply teach `characterCount` to flip the start and end
positions if the end position of the `SimpleRange` comes before the start.
Test: editing/selection/shift-click-includes-existing-selection.html
* editing/TextIterator.cpp:
(WebCore::characterCount):
LayoutTests:
Add a new layout test to verify that the bug does not occur.
* editing/mac/spelling/autocorrection-contraction-expected.txt:
Rebaseline an existing layout test, restoring the test expectations to what they were prior to r258871. It seems
that the more recently added expectations (while not seemingly incorrect) were dependent on `characterCount`
computing the number of characters from the start position to the end of the document, rather than the end.
* editing/selection/shift-click-includes-existing-selection-expected.txt: Added.
* editing/selection/shift-click-includes-existing-selection.html: Added.
* resources/ui-helper.js:
(window.UIHelper.activateAt.return.new.Promise):
(window.UIHelper.activateAt):
(window.UIHelper.activateElement):
Add an optional `modifiers` argument to the `activateAt` and `activateElement` helper methods, which can be used
to simulate key modifiers being held while synthesizing the "activation" (i.e. tap or click).
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (264689 => 264690)
--- trunk/LayoutTests/ChangeLog 2020-07-22 02:09:48 UTC (rev 264689)
+++ trunk/LayoutTests/ChangeLog 2020-07-22 02:47:29 UTC (rev 264690)
@@ -1,3 +1,29 @@
+2020-07-21 Wenson Hsieh <wenson_hs...@apple.com>
+
+ REGRESSION (r258871): Shift + click to extend selection loses currently selected text
+ https://bugs.webkit.org/show_bug.cgi?id=214617
+ <rdar://problem/64980223>
+
+ Reviewed by Megan Gardner.
+
+ Add a new layout test to verify that the bug does not occur.
+
+ * editing/mac/spelling/autocorrection-contraction-expected.txt:
+
+ Rebaseline an existing layout test, restoring the test expectations to what they were prior to r258871. It seems
+ that the more recently added expectations (while not seemingly incorrect) were dependent on `characterCount`
+ computing the number of characters from the start position to the end of the document, rather than the end.
+
+ * editing/selection/shift-click-includes-existing-selection-expected.txt: Added.
+ * editing/selection/shift-click-includes-existing-selection.html: Added.
+ * resources/ui-helper.js:
+ (window.UIHelper.activateAt.return.new.Promise):
+ (window.UIHelper.activateAt):
+ (window.UIHelper.activateElement):
+
+ Add an optional `modifiers` argument to the `activateAt` and `activateElement` helper methods, which can be used
+ to simulate key modifiers being held while synthesizing the "activation" (i.e. tap or click).
+
2020-07-21 Jer Noble <jer.no...@apple.com>
[Cocoa] Add experimental MSE WebM parser
Modified: trunk/LayoutTests/editing/mac/spelling/autocorrection-contraction-expected.txt (264689 => 264690)
--- trunk/LayoutTests/editing/mac/spelling/autocorrection-contraction-expected.txt 2020-07-22 02:09:48 UTC (rev 264689)
+++ trunk/LayoutTests/editing/mac/spelling/autocorrection-contraction-expected.txt 2020-07-22 02:47:29 UTC (rev 264690)
@@ -151,10 +151,23 @@
EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 6 of #text > DIV > DIV > BODY > HTML > #document to 6 of #text > DIV > DIV > BODY > HTML > #document toDOMRange:range from 7 of #text > DIV > DIV > BODY > HTML > #document to 7 of #text > DIV > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
EDITING DELEGATE: webViewDidChange:WebViewDidChangeNotification
+EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 7 of #text > DIV > DIV > BODY > HTML > #document to 7 of #text > DIV > DIV > BODY > HTML > #document toDOMRange:range from 0 of #text > DIV > DIV > BODY > HTML > #document to 6 of #text > DIV > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
+EDITING DELEGATE: shouldInsertText:would replacingDOMRange:range from 0 of #text > DIV > DIV > BODY > HTML > #document to 6 of #text > DIV > DIV > BODY > HTML > #document givenAction:WebViewInsertActionTyped
+EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 7 of #text > DIV > DIV > BODY > HTML > #document to 7 of #text > DIV > DIV > BODY > HTML > #document toDOMRange:range from 0 of #text > DIV > DIV > BODY > HTML > #document to 6 of #text > DIV > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
+EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 7 of #text > DIV > DIV > BODY > HTML > #document to 7 of #text > DIV > DIV > BODY > HTML > #document toDOMRange:range from 0 of #text > DIV > DIV > BODY > HTML > #document to 6 of #text > DIV > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
-EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 6 of #text > DIV > DIV > BODY > HTML > #document to 6 of #text > DIV > DIV > BODY > HTML > #document toDOMRange:range from 8 of #text > DIV > DIV > BODY > HTML > #document to 8 of #text > DIV > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
+EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 0 of DIV > DIV > BODY > HTML > #document to 0 of DIV > DIV > BODY > HTML > #document toDOMRange:range from 5 of #text > DIV > DIV > BODY > HTML > #document to 5 of #text > DIV > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
+EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
EDITING DELEGATE: webViewDidChange:WebViewDidChangeNotification
+EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
+EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
+EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 5 of #text > DIV > DIV > BODY > HTML > #document to 5 of #text > DIV > DIV > BODY > HTML > #document toDOMRange:range from 7 of #text > DIV > DIV > BODY > HTML > #document to 7 of #text > DIV > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
+EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
+EDITING DELEGATE: webViewDidChange:WebViewDidChangeNotification
+EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 7 of #text > DIV > DIV > BODY > HTML > #document to 7 of #text > DIV > DIV > BODY > HTML > #document toDOMRange:range from 8 of #text > DIV > DIV > BODY > HTML > #document to 8 of #text > DIV > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
+EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
+EDITING DELEGATE: webViewDidChange:WebViewDidChangeNotification
EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 8 of #text > DIV > DIV > BODY > HTML > #document to 8 of #text > DIV > DIV > BODY > HTML > #document toDOMRange:range from 9 of #text > DIV > DIV > BODY > HTML > #document to 9 of #text > DIV > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
EDITING DELEGATE: webViewDidChange:WebViewDidChangeNotification
@@ -173,19 +186,6 @@
EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 13 of #text > DIV > DIV > BODY > HTML > #document to 13 of #text > DIV > DIV > BODY > HTML > #document toDOMRange:range from 14 of #text > DIV > DIV > BODY > HTML > #document to 14 of #text > DIV > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
EDITING DELEGATE: webViewDidChange:WebViewDidChangeNotification
-EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 14 of #text > DIV > DIV > BODY > HTML > #document to 14 of #text > DIV > DIV > BODY > HTML > #document toDOMRange:range from 0 of #text > DIV > DIV > BODY > HTML > #document to 6 of #text > DIV > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
-EDITING DELEGATE: shouldInsertText:would replacingDOMRange:range from 0 of #text > DIV > DIV > BODY > HTML > #document to 6 of #text > DIV > DIV > BODY > HTML > #document givenAction:WebViewInsertActionTyped
-EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 14 of #text > DIV > DIV > BODY > HTML > #document to 14 of #text > DIV > DIV > BODY > HTML > #document toDOMRange:range from 0 of #text > DIV > DIV > BODY > HTML > #document to 6 of #text > DIV > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
-EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 14 of #text > DIV > DIV > BODY > HTML > #document to 14 of #text > DIV > DIV > BODY > HTML > #document toDOMRange:range from 0 of #text > DIV > DIV > BODY > HTML > #document to 6 of #text > DIV > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
-EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
-EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
-EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 0 of DIV > DIV > BODY > HTML > #document to 0 of DIV > DIV > BODY > HTML > #document toDOMRange:range from 5 of #text > DIV > DIV > BODY > HTML > #document to 5 of #text > DIV > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
-EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
-EDITING DELEGATE: webViewDidChange:WebViewDidChangeNotification
-EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
-EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 13 of #text > DIV > DIV > BODY > HTML > #document to 13 of #text > DIV > DIV > BODY > HTML > #document toDOMRange:range from 14 of #text > DIV > DIV > BODY > HTML > #document to 14 of #text > DIV > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
-EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
-EDITING DELEGATE: webViewDidChange:WebViewDidChangeNotification
EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 14 of #text > DIV > DIV > BODY > HTML > #document to 14 of #text > DIV > DIV > BODY > HTML > #document toDOMRange:range from 0 of DIV > DIV > BODY > HTML > #document to 0 of DIV > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
EDITING DELEGATE: webViewDidChange:WebViewDidChangeNotification
Added: trunk/LayoutTests/editing/selection/shift-click-includes-existing-selection-expected.txt (0 => 264690)
--- trunk/LayoutTests/editing/selection/shift-click-includes-existing-selection-expected.txt (rev 0)
+++ trunk/LayoutTests/editing/selection/shift-click-includes-existing-selection-expected.txt 2020-07-22 02:47:29 UTC (rev 264690)
@@ -0,0 +1,36 @@
+
+Before extending the selection:
+| "
+ "
+| <p>
+| "The quick brown fox jumped over the "
+| <span>
+| id="start"
+| "<#selection-anchor>lazy dog.<#selection-focus>"
+| "
+ "
+| <p>
+| <span>
+| id="end"
+| "T"
+| "he quick brown fox jumped over the lazy dog."
+| "
+"
+
+After extending the selection:
+| "
+ "
+| <p>
+| "The quick brown fox jumped over the "
+| <span>
+| id="start"
+| "<#selection-anchor>lazy dog."
+| "
+ "
+| <p>
+| <span>
+| id="end"
+| "T<#selection-focus>"
+| "he quick brown fox jumped over the lazy dog."
+| "
+"
Added: trunk/LayoutTests/editing/selection/shift-click-includes-existing-selection.html (0 => 264690)
--- trunk/LayoutTests/editing/selection/shift-click-includes-existing-selection.html (rev 0)
+++ trunk/LayoutTests/editing/selection/shift-click-includes-existing-selection.html 2020-07-22 02:47:29 UTC (rev 264690)
@@ -0,0 +1,46 @@
+<!DOCTYPE html> <!-- webkit-test-runner [ useFlexibleViewport=true ] -->
+<html>
+<head>
+<meta name="viewport" content="width=device-width, initial-scale=1">
+<script src=""
+<script src=""
+<style>
+p {
+ font-size: 20px;
+}
+
+#start {
+ border: 1px solid blue;
+}
+
+#end {
+ padding-right: 2px;
+ border: 1px solid tomato;
+}
+</style>
+</head>
+<body>
+<div>This test verifies that shift-clicking to extend an existing text selection does not unselect the currently selected text. To test manually, load the page and shift-click the capital letter T on the second line. The text 'lazy dog. T' should be selected.</div>
+<div id="container">
+ <p>The quick brown fox jumped over the <span id="start">lazy dog.</span></p>
+ <p><span id="end">T</span>he quick brown fox jumped over the lazy dog.</p>
+</div>
+<script>
+getSelection().selectAllChildren(document.getElementById("start"));
+Markup.waitUntilDone();
+
+(async function () {
+ if (!window.testRunner)
+ return;
+
+ internals.settings.setEditingBehavior("Mac");
+ Markup.dump(document.getElementById("container"), "Before extending the selection");
+
+ await UIHelper.callFunctionAndWaitForEvent(() => UIHelper.activateElement(document.getElementById("end"), ["shiftKey"]), document.body, "click");
+
+ Markup.dump(document.getElementById("container"), "After extending the selection");
+ Markup.notifyDone();
+})();
+</script>
+</body>
+</html>
\ No newline at end of file
Modified: trunk/LayoutTests/resources/ui-helper.js (264689 => 264690)
--- trunk/LayoutTests/resources/ui-helper.js 2020-07-22 02:09:48 UTC (rev 264689)
+++ trunk/LayoutTests/resources/ui-helper.js 2020-07-22 02:47:29 UTC (rev 264690)
@@ -240,28 +240,28 @@
});
}
- static activateAt(x, y)
+ static activateAt(x, y, modifiers=[])
{
if (!this.isWebKit2() || !this.isIOSFamily()) {
eventSender.mouseMoveTo(x, y);
- eventSender.mouseDown();
- eventSender.mouseUp();
+ eventSender.mouseDown(0, modifiers);
+ eventSender.mouseUp(0, modifiers);
return Promise.resolve();
}
return new Promise((resolve) => {
testRunner.runUIScript(`
- uiController.singleTapAtPoint(${x}, ${y}, function() {
+ uiController.singleTapAtPointWithModifiers(${x}, ${y}, ${JSON.stringify(modifiers)}, function() {
uiController.uiScriptComplete();
});`, resolve);
});
}
- static activateElement(element)
+ static activateElement(element, modifiers=[])
{
const x = element.offsetLeft + element.offsetWidth / 2;
const y = element.offsetTop + element.offsetHeight / 2;
- return UIHelper.activateAt(x, y);
+ return UIHelper.activateAt(x, y, modifiers);
}
static async doubleActivateAt(x, y)
Modified: trunk/Source/WebCore/ChangeLog (264689 => 264690)
--- trunk/Source/WebCore/ChangeLog 2020-07-22 02:09:48 UTC (rev 264689)
+++ trunk/Source/WebCore/ChangeLog 2020-07-22 02:47:29 UTC (rev 264690)
@@ -1,3 +1,40 @@
+2020-07-21 Wenson Hsieh <wenson_hs...@apple.com>
+
+ REGRESSION (r258871): Shift + click to extend selection loses currently selected text
+ https://bugs.webkit.org/show_bug.cgi?id=214617
+ <rdar://problem/64980223>
+
+ Reviewed by Megan Gardner.
+
+ After the changes in r258871, shift clicking sometimes fails to preserve the existing selected text range on
+ macOS and iOS. The logic in `EventHandler::handleMousePressEventSingleClick` uses the `textDistance` helper
+ method to count the number of characters between the start of the current selection to the newly selected
+ extent, as well as the number of characters between the end of the current selection and the newly selected
+ extent position. It compares these two character counts, and attempts to choose the new selection extents in
+ such a way that maximizes the amount of selected text.
+
+ However, after r258871, `textDistance` uses `characterCount` instead of `TextIterator::rangeLength`. Unlike the
+ former, `rangeLength` is robust in the case where the start position comes after the end position (in document
+ order), since the process of creating a live `Range` object swaps the start and end if needed. This isn't the
+ case when using `SimpleRange`. Instead, when given a simple range where the start comes after the end,
+ `characterCount` will iterate text in the DOM, starting from the start position and ending at the end of the
+ document rather than the end position. The result is that `characterCount` actually counts the number of
+ characters between the start position and the end of the document, rather than the number of characters between
+ the two positions.
+
+ In the context of this bug, if the start of the current selection is "far away" (in terms of character count)
+ from the end of the document and the new extent position comes after end of the current selection, we will end
+ up choosing the end (instead of the start) as one of the new extents of the updated selection.
+
+ To fix this (as well as other similar issues that might've arisen when replacing uses of
+ `TextIterator::rangeLength` with `characterCount`), simply teach `characterCount` to flip the start and end
+ positions if the end position of the `SimpleRange` comes before the start.
+
+ Test: editing/selection/shift-click-includes-existing-selection.html
+
+ * editing/TextIterator.cpp:
+ (WebCore::characterCount):
+
2020-07-21 Alex Christensen <achristen...@webkit.org>
Fix Windows build.
Modified: trunk/Source/WebCore/editing/TextIterator.cpp (264689 => 264690)
--- trunk/Source/WebCore/editing/TextIterator.cpp 2020-07-22 02:09:48 UTC (rev 264689)
+++ trunk/Source/WebCore/editing/TextIterator.cpp 2020-07-22 02:47:29 UTC (rev 264690)
@@ -2332,8 +2332,16 @@
uint64_t characterCount(const SimpleRange& range, TextIteratorBehavior behavior)
{
+ auto comparisonResult = Range::compareBoundaryPoints(&range.startContainer(), range.startOffset(), &range.endContainer(), range.endOffset());
+ if (comparisonResult.hasException())
+ return 0;
+
+ auto adjustedRange(range);
+ if (comparisonResult.releaseReturnValue() > 0)
+ std::swap(adjustedRange.start, adjustedRange.end);
+
uint64_t length = 0;
- for (TextIterator it(range, behavior); !it.atEnd(); it.advance())
+ for (TextIterator it(adjustedRange, behavior); !it.atEnd(); it.advance())
length += it.text().length();
return length;
}
