Extra space inserted at start of line when inserting a newline in Mail compose
authorwenson_hsieh@apple.com <wenson_hsieh@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 7 Aug 2019 16:23:52 +0000 (16:23 +0000)
committerwenson_hsieh@apple.com <wenson_hsieh@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 7 Aug 2019 16:23:52 +0000 (16:23 +0000)
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

LayoutTests/ChangeLog
LayoutTests/editing/inserting/insert-paragraph-in-designmode-document-expected.txt [new file with mode: 0644]
LayoutTests/editing/inserting/insert-paragraph-in-designmode-document.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/rendering/RenderText.cpp
Source/WebCore/rendering/SimpleLineLayoutFunctions.h

index d534d8a..b178ef6 100644 (file)
@@ -1,3 +1,17 @@
+2019-08-07  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        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-06  Ryosuke Niwa  <rniwa@webkit.org>
 
         [iPadOS] slides.google.com: tapping near cursor in a slide title focuses the speaker notes
diff --git a/LayoutTests/editing/inserting/insert-paragraph-in-designmode-document-expected.txt b/LayoutTests/editing/inserting/insert-paragraph-in-designmode-document-expected.txt
new file mode 100644 (file)
index 0000000..fab4634
--- /dev/null
@@ -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."
+|     "
+
+
+"
diff --git a/LayoutTests/editing/inserting/insert-paragraph-in-designmode-document.html b/LayoutTests/editing/inserting/insert-paragraph-in-designmode-document.html
new file mode 100644 (file)
index 0000000..208f367
--- /dev/null
@@ -0,0 +1,21 @@
+<!DOCTYPE html>
+<html>
+<head>
+    <script src="../../resources/dump-as-markup.js"></script>
+    <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>
index 1933df4..dfd72f4 100644 (file)
@@ -1,3 +1,53 @@
+2019-08-07  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        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-07  Youenn Fablet  <youenn@apple.com>
 
         ASSERT that a sessionID is valid when encoding it
index 24d21f6..68c3a15 100644 (file)
@@ -1489,14 +1489,15 @@ unsigned RenderText::countRenderedCharacterOffsetsUntil(unsigned offset) const
 
 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);
 }
 
 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);
 }
 
index 6f9ae74..a12af73 100644 (file)
@@ -50,7 +50,8 @@ bool hitTestFlow(const RenderBlockFlow&, const Layout&, const HitTestRequest&, H
 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 @@ inline unsigned findCaretMaximumOffset(const RenderText& renderer, const Layout&
     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;