- Revision
- 265678
- Author
- [email protected]
- Date
- 2020-08-14 10:38:05 -0700 (Fri, 14 Aug 2020)
Log Message
REGRESSION (r259184): Typing -- then Return into an email moves the selection by two lines
https://bugs.webkit.org/show_bug.cgi?id=215491
<rdar://problem/66938121>
Reviewed by Darin Adler.
Source/WebCore:
When inserting a newline after text that is about to be replaced (e.g. using smart dashes, or a system-wide text
replacement), logic in `Editor::markAndReplaceFor` attempts to detect the fact that we've just inserted a new
paragraph (setting `adjustSelectionForParagraphBoundaries` to `true`), and subsequently causes us to advance
the selection forward by 1 character after we're done handling the text replacement.
This logic appears to have been added to deal with the fact that prior to r259184, `TextIterator::subrange()`
with a range that ended in a line break would not include the line break as a part of the resulting subrange.
For instance, suppose we're inserting a newline after "--", text replacement has just run and replaced the
two hyphens with a single dash (—), and there is a newline after the dash. The extended paragraph range consists
of the dash, followed by the line break after it ("—\n"), with a `selectionOffset` of 2. The following code:
`auto selectionRange = extendedParagraph.subrange({ 0, selectionOffset });`
...would compute a `selectionRange` that encompasses only the "—", despite a selection offset of 2, causing the
following code to only move the selection to the end of the "—":
`m_document.selection().moveTo(createLegacyEditingPosition(selectionRange.end), DOWNSTREAM);`
This requires the logic in the `adjustSelectionForParagraphBoundaries` to subsequently move the selection to the
line break, as the user would expect. However, after the changes in r259184, the subrange now (correctly)
returns a range that starts before the "—" and ends at the following line break. However, this also means that
the subsequent adjustment logic will cause us to advance unnecessarily!
To fix this, we remove the `adjustSelectionForParagraphBoundaries` case altogether, since its only (apparent)
purpose is to work around the fact that `TextIterator::subrange` didn't include trailing line breaks before
r259184.
Test: editing/spelling/text-replacement-after-typing-to-word.html
* editing/Editor.cpp:
(WebCore::Editor::markAndReplaceFor):
LayoutTests:
Tweaks an existing layout test so that it additionally exercises this bug. The erroneous code was already hit
during this test, but it currently does not result in any behavioral difference because after inserting the
newline, the caret is already at the end of the editable root; this means that the redundant call to advance
the selection forward by 1 character ends up being a no-op, and the test passes.
To adjust the test so that it fails, we simply start the root editable container off with two lines instead of
just one, such that the test (without this fix) will cause the selection to end up at the very end of the root
editable container instead of the first line after the "?".
To check that the selection ends up in the right place, we also use `dump-as-markup.js` to dump the caret
position after inserting the newline.
* editing/spelling/text-replacement-after-typing-to-word-expected.txt:
* editing/spelling/text-replacement-after-typing-to-word.html:
Modified Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (265677 => 265678)
--- trunk/LayoutTests/ChangeLog 2020-08-14 17:34:41 UTC (rev 265677)
+++ trunk/LayoutTests/ChangeLog 2020-08-14 17:38:05 UTC (rev 265678)
@@ -1,3 +1,26 @@
+2020-08-14 Wenson Hsieh <[email protected]>
+
+ REGRESSION (r259184): Typing -- then Return into an email moves the selection by two lines
+ https://bugs.webkit.org/show_bug.cgi?id=215491
+ <rdar://problem/66938121>
+
+ Reviewed by Darin Adler.
+
+ Tweaks an existing layout test so that it additionally exercises this bug. The erroneous code was already hit
+ during this test, but it currently does not result in any behavioral difference because after inserting the
+ newline, the caret is already at the end of the editable root; this means that the redundant call to advance
+ the selection forward by 1 character ends up being a no-op, and the test passes.
+
+ To adjust the test so that it fails, we simply start the root editable container off with two lines instead of
+ just one, such that the test (without this fix) will cause the selection to end up at the very end of the root
+ editable container instead of the first line after the "?".
+
+ To check that the selection ends up in the right place, we also use `dump-as-markup.js` to dump the caret
+ position after inserting the newline.
+
+ * editing/spelling/text-replacement-after-typing-to-word-expected.txt:
+ * editing/spelling/text-replacement-after-typing-to-word.html:
+
2020-08-14 Takeshi Kurosawa <[email protected]>
@font-face font-weight descriptor should reject bolder and lighter
Modified: trunk/LayoutTests/editing/spelling/text-replacement-after-typing-to-word-expected.txt (265677 => 265678)
--- trunk/LayoutTests/editing/spelling/text-replacement-after-typing-to-word-expected.txt 2020-08-14 17:34:41 UTC (rev 265677)
+++ trunk/LayoutTests/editing/spelling/text-replacement-after-typing-to-word-expected.txt 2020-08-14 17:38:05 UTC (rev 265678)
@@ -1,8 +1,28 @@
+To manually test, add an automatic text replacement mapping from the string 'YT?' to 'You there?', and then type the string 'YT?'. It should not be immediately corrected to 'You there?'. However, entering a newline should subsequently trigger autocorrection, and place the selection on the line after 'You there?'
+
Before pressing enter:
-PASS editor.textContent is 'YT?'
-PASS successfullyParsed is true
+| "
+ "
+| <div>
+| "YT?<#selection-caret>"
+| "
+ "
+| <div>
+| <br>
+| "
+ "
-TEST COMPLETE
-You there?
-
-
+After pressing enter:
+| "
+ "
+| <div>
+| "You there?"
+| <div>
+| <#selection-caret>
+| <br>
+| "
+ "
+| <div>
+| <br>
+| "
+ "
Modified: trunk/LayoutTests/editing/spelling/text-replacement-after-typing-to-word.html (265677 => 265678)
--- trunk/LayoutTests/editing/spelling/text-replacement-after-typing-to-word.html 2020-08-14 17:34:41 UTC (rev 265677)
+++ trunk/LayoutTests/editing/spelling/text-replacement-after-typing-to-word.html 2020-08-14 17:38:05 UTC (rev 265678)
@@ -1,16 +1,12 @@
<html>
<head>
<script src=""
-<script src=""
+<script src=""
<script src=""
<script>
-jsTestIsAsync = true;
+Markup.description("To manually test, add an automatic text replacement mapping from the string 'YT?' to 'You there?', and then type the string 'YT?'. It should not be immediately corrected to 'You there?'. However, entering a newline should subsequently trigger autocorrection, and place the selection on the line after 'You there?'");
+Markup.waitUntilDone();
-function zeroDelayTimer()
-{
- return new Promise(resolve => setTimeout(resolve, 0));
-}
-
async function runTest()
{
if (window.internals) {
@@ -42,20 +38,15 @@
editor.focus();
for (const character of "YT?")
typeCharacterCommand(character);
- await zeroDelayTimer();
+ await UIHelper.delayFor(0);
- if (!window.testRunner) {
- description.innerHTML = `To manually test, add an automatic text replacement mapping from the string "YT?" to
- "You there?", and then type the string "YT?". It should not be immediately corrected to "You there?".
- However, entering a newline should subsequently trigger autocorrection.`;
- return;
- }
+ Markup.dump(editor, "Before pressing enter");
- debug("Before pressing enter:");
- shouldBe("editor.textContent", "'YT?'");
insertParagraphCommand();
- await zeroDelayTimer();
- finishJSTest();
+ await UIHelper.delayFor(0);
+
+ Markup.dump(editor, "After pressing enter");
+ Markup.notifyDone();
}
</script>
</head>
@@ -62,6 +53,9 @@
<body _onload_="runTest()">
<div id="description"></div>
- <div contenteditable style="margin-bottom: 1em; border: 1px orange dashed;" id="editor"></div>
+ <div contenteditable style="margin-bottom: 1em; border: 1px orange dashed;" id="editor">
+ <div><br></div>
+ <div><br></div>
+ </div>
</body>
</html>
Modified: trunk/Source/WebCore/ChangeLog (265677 => 265678)
--- trunk/Source/WebCore/ChangeLog 2020-08-14 17:34:41 UTC (rev 265677)
+++ trunk/Source/WebCore/ChangeLog 2020-08-14 17:38:05 UTC (rev 265678)
@@ -1,3 +1,43 @@
+2020-08-14 Wenson Hsieh <[email protected]>
+
+ REGRESSION (r259184): Typing -- then Return into an email moves the selection by two lines
+ https://bugs.webkit.org/show_bug.cgi?id=215491
+ <rdar://problem/66938121>
+
+ Reviewed by Darin Adler.
+
+ When inserting a newline after text that is about to be replaced (e.g. using smart dashes, or a system-wide text
+ replacement), logic in `Editor::markAndReplaceFor` attempts to detect the fact that we've just inserted a new
+ paragraph (setting `adjustSelectionForParagraphBoundaries` to `true`), and subsequently causes us to advance
+ the selection forward by 1 character after we're done handling the text replacement.
+
+ This logic appears to have been added to deal with the fact that prior to r259184, `TextIterator::subrange()`
+ with a range that ended in a line break would not include the line break as a part of the resulting subrange.
+ For instance, suppose we're inserting a newline after "--", text replacement has just run and replaced the
+ two hyphens with a single dash (—), and there is a newline after the dash. The extended paragraph range consists
+ of the dash, followed by the line break after it ("—\n"), with a `selectionOffset` of 2. The following code:
+
+ `auto selectionRange = extendedParagraph.subrange({ 0, selectionOffset });`
+
+ ...would compute a `selectionRange` that encompasses only the "—", despite a selection offset of 2, causing the
+ following code to only move the selection to the end of the "—":
+
+ `m_document.selection().moveTo(createLegacyEditingPosition(selectionRange.end), DOWNSTREAM);`
+
+ This requires the logic in the `adjustSelectionForParagraphBoundaries` to subsequently move the selection to the
+ line break, as the user would expect. However, after the changes in r259184, the subrange now (correctly)
+ returns a range that starts before the "—" and ends at the following line break. However, this also means that
+ the subsequent adjustment logic will cause us to advance unnecessarily!
+
+ To fix this, we remove the `adjustSelectionForParagraphBoundaries` case altogether, since its only (apparent)
+ purpose is to work around the fact that `TextIterator::subrange` didn't include trailing line breaks before
+ r259184.
+
+ Test: editing/spelling/text-replacement-after-typing-to-word.html
+
+ * editing/Editor.cpp:
+ (WebCore::Editor::markAndReplaceFor):
+
2020-08-14 Takeshi Kurosawa <[email protected]>
@font-face font-weight descriptor should reject bolder and lighter
Modified: trunk/Source/WebCore/editing/Editor.cpp (265677 => 265678)
--- trunk/Source/WebCore/editing/Editor.cpp 2020-08-14 17:34:41 UTC (rev 265677)
+++ trunk/Source/WebCore/editing/Editor.cpp 2020-08-14 17:38:05 UTC (rev 265678)
@@ -2782,7 +2782,6 @@
bool useAmbiguousBoundaryOffset = false;
bool selectionChanged = false;
bool restoreSelectionAfterChange = false;
- bool adjustSelectionForParagraphBoundaries = false;
if (shouldPerformReplacement || shouldMarkSpelling || shouldCheckForCorrection) {
if (m_document.selection().selection().selectionType() == VisibleSelection::CaretSelection) {
@@ -2790,8 +2789,6 @@
Position caretPosition = m_document.selection().selection().end();
selectionOffset = paragraph.offsetTo(caretPosition).releaseReturnValue();
restoreSelectionAfterChange = true;
- if (selectionOffset > 0 && (selectionOffset > paragraph.text().length() || paragraph.text()[selectionOffset - 1] == newlineCharacter))
- adjustSelectionForParagraphBoundaries = true;
if (selectionOffset > 0 && selectionOffset <= paragraph.text().length() && isAmbiguousBoundaryCharacter(paragraph.text()[selectionOffset - 1]))
useAmbiguousBoundaryOffset = true;
}
@@ -2915,8 +2912,6 @@
if (restoreSelectionAfterChange && selectionOffset <= extendedParagraph.rangeLength()) {
auto selectionRange = extendedParagraph.subrange({ 0, selectionOffset });
m_document.selection().moveTo(createLegacyEditingPosition(selectionRange.end), DOWNSTREAM);
- if (adjustSelectionForParagraphBoundaries)
- m_document.selection().modify(FrameSelection::AlterationMove, SelectionDirection::Forward, TextGranularity::CharacterGranularity);
} else {
// If this fails for any reason, the fallback is to go one position beyond the last replacement
m_document.selection().moveTo(m_document.selection().selection().end());
