- Revision
- 248563
- Author
- [email protected]
- Date
- 2019-08-12 16:42:02 -0700 (Mon, 12 Aug 2019)
Log Message
Cherry-pick r248368. rdar://problem/54037153
Extra space inserted at start of line when inserting a newline in Mail compose
https://bugs.webkit.org/show_bug.cgi?id=200490
<rdar://problem/53501354>
Reviewed by Antti Koivisto.
Source/WebCore:
This started happening after r244494, which deferred editor state computation until the next layer tree flush
when changing selection. After inserting a paragraph, the act of computing an editor state ensured that the text
node containing the caret drops out of simple line layout, while grabbing the characters near the selection
(i.e., calling charactersAroundPosition). This meant that when we subsequently ask positionAfterSplit whether it
isRenderedCharacter() at the end of the command, we are guaranteed to have line boxes, so we get a meaningful
answer and avoid inserting an extra non-breaking space.
However, after r244494, we defer the editor state computation until the end of the edit command; this means that
we may not have line boxes for positionAfterSplit's text node renderer, due to remaining in simple line layout.
In turn, this means that we end up hitting the assertion in containsRenderedCharacterOffset in debug builds; on
release builds, we simply return false from containsRenderedCharacterOffset, which causes us to insert an extra
space.
To fix this, we educate RenderText::containsRenderedCharacterOffset about simple line layout.
Test: editing/inserting/insert-paragraph-in-designmode-document.html
* rendering/RenderText.cpp:
(WebCore::RenderText::containsRenderedCharacterOffset const):
(WebCore::RenderText::containsCaretOffset const):
Changed to use SimpleLineLayout::containsOffset.
* rendering/SimpleLineLayoutFunctions.h:
(WebCore::SimpleLineLayout::containsOffset):
I first contrasted the behavior of RenderTextLineBoxes::containsOffset in the cases where the OffsetType is
CaretOffset or CharacterOffset, and found that the only interesting differences were:
1. The caret offset type case has special handling for line breaks.
2. Both offset types have handling for reversed text.
3. The end offset of a line box contains a caret offset, but not a character offset.
For the purposes of OffsetType CharacterOffset, (1) is irrelevant; furthermore, (2) is already not handled by
logic in containsCaretOffset(). Thus, the only major difference in the CharacterOffset case should be (3), which
we handle by only allowing the case where the given offset is equal to the very end of a text run for caret
offsets, and not character offsets.
(WebCore::SimpleLineLayout::containsCaretOffset): Deleted.
Renamed to just containsOffset.
LayoutTests:
Add a new test to verify that inserting a newline in the middle of text in a document with designMode "on"
doesn't insert an extra space at the beginning of the newly inserted line.
* editing/inserting/insert-paragraph-in-designmode-document-expected.txt: Added.
* editing/inserting/insert-paragraph-in-designmode-document.html: Added.
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@248368 268f45cc-cd09-0410-ab3c-d52691b4dbfc
Modified Paths
Added Paths
Diff
Modified: branches/safari-608-branch/LayoutTests/ChangeLog (248562 => 248563)
--- branches/safari-608-branch/LayoutTests/ChangeLog 2019-08-12 23:41:59 UTC (rev 248562)
+++ branches/safari-608-branch/LayoutTests/ChangeLog 2019-08-12 23:42:02 UTC (rev 248563)
@@ -1,5 +1,84 @@
2019-08-12 Alan Coon <[email protected]>
+ Cherry-pick r248368. rdar://problem/54037153
+
+ Extra space inserted at start of line when inserting a newline in Mail compose
+ https://bugs.webkit.org/show_bug.cgi?id=200490
+ <rdar://problem/53501354>
+
+ Reviewed by Antti Koivisto.
+
+ Source/WebCore:
+
+ This started happening after r244494, which deferred editor state computation until the next layer tree flush
+ when changing selection. After inserting a paragraph, the act of computing an editor state ensured that the text
+ node containing the caret drops out of simple line layout, while grabbing the characters near the selection
+ (i.e., calling charactersAroundPosition). This meant that when we subsequently ask positionAfterSplit whether it
+ isRenderedCharacter() at the end of the command, we are guaranteed to have line boxes, so we get a meaningful
+ answer and avoid inserting an extra non-breaking space.
+
+ However, after r244494, we defer the editor state computation until the end of the edit command; this means that
+ we may not have line boxes for positionAfterSplit's text node renderer, due to remaining in simple line layout.
+ In turn, this means that we end up hitting the assertion in containsRenderedCharacterOffset in debug builds; on
+ release builds, we simply return false from containsRenderedCharacterOffset, which causes us to insert an extra
+ space.
+
+ To fix this, we educate RenderText::containsRenderedCharacterOffset about simple line layout.
+
+ Test: editing/inserting/insert-paragraph-in-designmode-document.html
+
+ * rendering/RenderText.cpp:
+ (WebCore::RenderText::containsRenderedCharacterOffset const):
+ (WebCore::RenderText::containsCaretOffset const):
+
+ Changed to use SimpleLineLayout::containsOffset.
+
+ * rendering/SimpleLineLayoutFunctions.h:
+ (WebCore::SimpleLineLayout::containsOffset):
+
+ I first contrasted the behavior of RenderTextLineBoxes::containsOffset in the cases where the OffsetType is
+ CaretOffset or CharacterOffset, and found that the only interesting differences were:
+
+ 1. The caret offset type case has special handling for line breaks.
+ 2. Both offset types have handling for reversed text.
+ 3. The end offset of a line box contains a caret offset, but not a character offset.
+
+ For the purposes of OffsetType CharacterOffset, (1) is irrelevant; furthermore, (2) is already not handled by
+ logic in containsCaretOffset(). Thus, the only major difference in the CharacterOffset case should be (3), which
+ we handle by only allowing the case where the given offset is equal to the very end of a text run for caret
+ offsets, and not character offsets.
+
+ (WebCore::SimpleLineLayout::containsCaretOffset): Deleted.
+
+ Renamed to just containsOffset.
+
+ LayoutTests:
+
+ Add a new test to verify that inserting a newline in the middle of text in a document with designMode "on"
+ doesn't insert an extra space at the beginning of the newly inserted line.
+
+ * editing/inserting/insert-paragraph-in-designmode-document-expected.txt: Added.
+ * editing/inserting/insert-paragraph-in-designmode-document.html: Added.
+
+
+ git-svn-id: https://svn.webkit.org/repository/webkit/trunk@248368 268f45cc-cd09-0410-ab3c-d52691b4dbfc
+
+ 2019-08-07 Wenson Hsieh <[email protected]>
+
+ Extra space inserted at start of line when inserting a newline in Mail compose
+ https://bugs.webkit.org/show_bug.cgi?id=200490
+ <rdar://problem/53501354>
+
+ Reviewed by Antti Koivisto.
+
+ Add a new test to verify that inserting a newline in the middle of text in a document with designMode "on"
+ doesn't insert an extra space at the beginning of the newly inserted line.
+
+ * editing/inserting/insert-paragraph-in-designmode-document-expected.txt: Added.
+ * editing/inserting/insert-paragraph-in-designmode-document.html: Added.
+
+2019-08-12 Alan Coon <[email protected]>
+
Cherry-pick r248265. rdar://problem/54017842
Ping loads should not prevent page caching
Added: branches/safari-608-branch/LayoutTests/editing/inserting/insert-paragraph-in-designmode-document-expected.txt (0 => 248563)
--- branches/safari-608-branch/LayoutTests/editing/inserting/insert-paragraph-in-designmode-document-expected.txt (rev 0)
+++ branches/safari-608-branch/LayoutTests/editing/inserting/insert-paragraph-in-designmode-document-expected.txt 2019-08-12 23:42:02 UTC (rev 248563)
@@ -0,0 +1,25 @@
+Verifies that after inserting a newline after a period doesn't insert an extra space in front of the newly inserted line.
+| <!DOCTYPE html>
+| <html>
+| <head>
+| "
+ "
+| "
+ "
+| "
+"
+| "
+"
+| <body>
+| "
+ "
+| <div>
+| class="container"
+| "Hello."
+| <div>
+| class="container"
+| "<#selection-caret>This is a test."
+| "
+
+
+"
Added: branches/safari-608-branch/LayoutTests/editing/inserting/insert-paragraph-in-designmode-document.html (0 => 248563)
--- branches/safari-608-branch/LayoutTests/editing/inserting/insert-paragraph-in-designmode-document.html (rev 0)
+++ branches/safari-608-branch/LayoutTests/editing/inserting/insert-paragraph-in-designmode-document.html 2019-08-12 23:42:02 UTC (rev 248563)
@@ -0,0 +1,21 @@
+<!DOCTYPE html>
+<html>
+<head>
+ <script src=""
+ <script>
+ addEventListener("DOMContentLoaded", () => {
+ document.designMode = "on";
+
+ const container = document.querySelector(".container");
+ getSelection().setPosition(container.childNodes[0], 6);
+ document.execCommand("InsertParagraph");
+ document.querySelectorAll("script").forEach(script => script.remove());
+ Markup.description("Verifies that after inserting a newline after a period doesn't insert an extra space in front of the newly inserted line.");
+ Markup.dump("document.body");
+ });
+ </script>
+</head>
+<body>
+ <div class="container">Hello.This is a test.</div>
+</body>
+</html>
Modified: branches/safari-608-branch/Source/WebCore/ChangeLog (248562 => 248563)
--- branches/safari-608-branch/Source/WebCore/ChangeLog 2019-08-12 23:41:59 UTC (rev 248562)
+++ branches/safari-608-branch/Source/WebCore/ChangeLog 2019-08-12 23:42:02 UTC (rev 248563)
@@ -1,5 +1,120 @@
2019-08-12 Alan Coon <[email protected]>
+ Cherry-pick r248368. rdar://problem/54037153
+
+ Extra space inserted at start of line when inserting a newline in Mail compose
+ https://bugs.webkit.org/show_bug.cgi?id=200490
+ <rdar://problem/53501354>
+
+ Reviewed by Antti Koivisto.
+
+ Source/WebCore:
+
+ This started happening after r244494, which deferred editor state computation until the next layer tree flush
+ when changing selection. After inserting a paragraph, the act of computing an editor state ensured that the text
+ node containing the caret drops out of simple line layout, while grabbing the characters near the selection
+ (i.e., calling charactersAroundPosition). This meant that when we subsequently ask positionAfterSplit whether it
+ isRenderedCharacter() at the end of the command, we are guaranteed to have line boxes, so we get a meaningful
+ answer and avoid inserting an extra non-breaking space.
+
+ However, after r244494, we defer the editor state computation until the end of the edit command; this means that
+ we may not have line boxes for positionAfterSplit's text node renderer, due to remaining in simple line layout.
+ In turn, this means that we end up hitting the assertion in containsRenderedCharacterOffset in debug builds; on
+ release builds, we simply return false from containsRenderedCharacterOffset, which causes us to insert an extra
+ space.
+
+ To fix this, we educate RenderText::containsRenderedCharacterOffset about simple line layout.
+
+ Test: editing/inserting/insert-paragraph-in-designmode-document.html
+
+ * rendering/RenderText.cpp:
+ (WebCore::RenderText::containsRenderedCharacterOffset const):
+ (WebCore::RenderText::containsCaretOffset const):
+
+ Changed to use SimpleLineLayout::containsOffset.
+
+ * rendering/SimpleLineLayoutFunctions.h:
+ (WebCore::SimpleLineLayout::containsOffset):
+
+ I first contrasted the behavior of RenderTextLineBoxes::containsOffset in the cases where the OffsetType is
+ CaretOffset or CharacterOffset, and found that the only interesting differences were:
+
+ 1. The caret offset type case has special handling for line breaks.
+ 2. Both offset types have handling for reversed text.
+ 3. The end offset of a line box contains a caret offset, but not a character offset.
+
+ For the purposes of OffsetType CharacterOffset, (1) is irrelevant; furthermore, (2) is already not handled by
+ logic in containsCaretOffset(). Thus, the only major difference in the CharacterOffset case should be (3), which
+ we handle by only allowing the case where the given offset is equal to the very end of a text run for caret
+ offsets, and not character offsets.
+
+ (WebCore::SimpleLineLayout::containsCaretOffset): Deleted.
+
+ Renamed to just containsOffset.
+
+ LayoutTests:
+
+ Add a new test to verify that inserting a newline in the middle of text in a document with designMode "on"
+ doesn't insert an extra space at the beginning of the newly inserted line.
+
+ * editing/inserting/insert-paragraph-in-designmode-document-expected.txt: Added.
+ * editing/inserting/insert-paragraph-in-designmode-document.html: Added.
+
+
+ git-svn-id: https://svn.webkit.org/repository/webkit/trunk@248368 268f45cc-cd09-0410-ab3c-d52691b4dbfc
+
+ 2019-08-07 Wenson Hsieh <[email protected]>
+
+ Extra space inserted at start of line when inserting a newline in Mail compose
+ https://bugs.webkit.org/show_bug.cgi?id=200490
+ <rdar://problem/53501354>
+
+ Reviewed by Antti Koivisto.
+
+ This started happening after r244494, which deferred editor state computation until the next layer tree flush
+ when changing selection. After inserting a paragraph, the act of computing an editor state ensured that the text
+ node containing the caret drops out of simple line layout, while grabbing the characters near the selection
+ (i.e., calling charactersAroundPosition). This meant that when we subsequently ask positionAfterSplit whether it
+ isRenderedCharacter() at the end of the command, we are guaranteed to have line boxes, so we get a meaningful
+ answer and avoid inserting an extra non-breaking space.
+
+ However, after r244494, we defer the editor state computation until the end of the edit command; this means that
+ we may not have line boxes for positionAfterSplit's text node renderer, due to remaining in simple line layout.
+ In turn, this means that we end up hitting the assertion in containsRenderedCharacterOffset in debug builds; on
+ release builds, we simply return false from containsRenderedCharacterOffset, which causes us to insert an extra
+ space.
+
+ To fix this, we educate RenderText::containsRenderedCharacterOffset about simple line layout.
+
+ Test: editing/inserting/insert-paragraph-in-designmode-document.html
+
+ * rendering/RenderText.cpp:
+ (WebCore::RenderText::containsRenderedCharacterOffset const):
+ (WebCore::RenderText::containsCaretOffset const):
+
+ Changed to use SimpleLineLayout::containsOffset.
+
+ * rendering/SimpleLineLayoutFunctions.h:
+ (WebCore::SimpleLineLayout::containsOffset):
+
+ I first contrasted the behavior of RenderTextLineBoxes::containsOffset in the cases where the OffsetType is
+ CaretOffset or CharacterOffset, and found that the only interesting differences were:
+
+ 1. The caret offset type case has special handling for line breaks.
+ 2. Both offset types have handling for reversed text.
+ 3. The end offset of a line box contains a caret offset, but not a character offset.
+
+ For the purposes of OffsetType CharacterOffset, (1) is irrelevant; furthermore, (2) is already not handled by
+ logic in containsCaretOffset(). Thus, the only major difference in the CharacterOffset case should be (3), which
+ we handle by only allowing the case where the given offset is equal to the very end of a text run for caret
+ offsets, and not character offsets.
+
+ (WebCore::SimpleLineLayout::containsCaretOffset): Deleted.
+
+ Renamed to just containsOffset.
+
+2019-08-12 Alan Coon <[email protected]>
+
Cherry-pick r248265. rdar://problem/54017842
Ping loads should not prevent page caching
Modified: branches/safari-608-branch/Source/WebCore/rendering/RenderText.cpp (248562 => 248563)
--- branches/safari-608-branch/Source/WebCore/rendering/RenderText.cpp 2019-08-12 23:41:59 UTC (rev 248562)
+++ branches/safari-608-branch/Source/WebCore/rendering/RenderText.cpp 2019-08-12 23:42:02 UTC (rev 248563)
@@ -1489,7 +1489,8 @@
bool RenderText::containsRenderedCharacterOffset(unsigned offset) const
{
- ASSERT(!simpleLineLayout());
+ if (auto* layout = simpleLineLayout())
+ return SimpleLineLayout::containsOffset(*this, *layout, offset, SimpleLineLayout::OffsetType::CharacterOffset);
return m_lineBoxes.containsOffset(*this, offset, RenderTextLineBoxes::CharacterOffset);
}
@@ -1496,7 +1497,7 @@
bool RenderText::containsCaretOffset(unsigned offset) const
{
if (auto* layout = simpleLineLayout())
- return SimpleLineLayout::containsCaretOffset(*this, *layout, offset);
+ return SimpleLineLayout::containsOffset(*this, *layout, offset, SimpleLineLayout::OffsetType::CaretOffset);
return m_lineBoxes.containsOffset(*this, offset, RenderTextLineBoxes::CaretOffset);
}
Modified: branches/safari-608-branch/Source/WebCore/rendering/SimpleLineLayoutFunctions.h (248562 => 248563)
--- branches/safari-608-branch/Source/WebCore/rendering/SimpleLineLayoutFunctions.h 2019-08-12 23:41:59 UTC (rev 248562)
+++ branches/safari-608-branch/Source/WebCore/rendering/SimpleLineLayoutFunctions.h 2019-08-12 23:42:02 UTC (rev 248563)
@@ -50,7 +50,8 @@
void collectFlowOverflow(RenderBlockFlow&, const Layout&);
bool isTextRendered(const RenderText&, const Layout&);
-bool containsCaretOffset(const RenderObject&, const Layout&, unsigned);
+enum class OffsetType { CaretOffset, CharacterOffset };
+bool containsOffset(const RenderText&, const Layout&, unsigned, OffsetType);
unsigned findCaretMinimumOffset(const RenderObject&, const Layout&);
unsigned findCaretMaximumOffset(const RenderObject&, const Layout&);
IntRect computeBoundingBox(const RenderObject&, const Layout&);
@@ -116,13 +117,13 @@
return last.end;
}
-inline bool containsCaretOffset(const RenderText&, const Layout& layout, unsigned offset)
+inline bool containsOffset(const RenderText&, const Layout& layout, unsigned offset, OffsetType offsetType)
{
for (unsigned i = 0; i < layout.runCount(); ++i) {
auto& run = layout.runAt(i);
if (offset < run.start)
return false;
- if (offset <= run.end)
+ if (offset < run.end || (offsetType == OffsetType::CaretOffset && offset == run.end))
return true;
}
return false;
