Changeset 248368 in webkit


Ignore:
Timestamp:
Aug 7, 2019 9:23:52 AM (5 years ago)
Author:
Wenson Hsieh
Message:

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.
Location:
trunk
Files:
2 added
4 edited

Legend:

Unmodified
Added
Removed
  • trunk/LayoutTests/ChangeLog

    r248338 r248368  
     12019-08-07  Wenson Hsieh  <wenson_hsieh@apple.com>
     2
     3        Extra space inserted at start of line when inserting a newline in Mail compose
     4        https://bugs.webkit.org/show_bug.cgi?id=200490
     5        <rdar://problem/53501354>
     6
     7        Reviewed by Antti Koivisto.
     8
     9        Add a new test to verify that inserting a newline in the middle of text in a document with designMode "on"
     10        doesn't insert an extra space at the beginning of the newly inserted line.
     11
     12        * editing/inserting/insert-paragraph-in-designmode-document-expected.txt: Added.
     13        * editing/inserting/insert-paragraph-in-designmode-document.html: Added.
     14
    1152019-08-06  Ryosuke Niwa  <rniwa@webkit.org>
    216
  • trunk/Source/WebCore/ChangeLog

    r248366 r248368  
     12019-08-07  Wenson Hsieh  <wenson_hsieh@apple.com>
     2
     3        Extra space inserted at start of line when inserting a newline in Mail compose
     4        https://bugs.webkit.org/show_bug.cgi?id=200490
     5        <rdar://problem/53501354>
     6
     7        Reviewed by Antti Koivisto.
     8
     9        This started happening after r244494, which deferred editor state computation until the next layer tree flush
     10        when changing selection. After inserting a paragraph, the act of computing an editor state ensured that the text
     11        node containing the caret drops out of simple line layout, while grabbing the characters near the selection
     12        (i.e., calling charactersAroundPosition). This meant that when we subsequently ask positionAfterSplit whether it
     13        isRenderedCharacter() at the end of the command, we are guaranteed to have line boxes, so we get a meaningful
     14        answer and avoid inserting an extra non-breaking space.
     15
     16        However, after r244494, we defer the editor state computation until the end of the edit command; this means that
     17        we may not have line boxes for positionAfterSplit's text node renderer, due to remaining in simple line layout.
     18        In turn, this means that we end up hitting the assertion in containsRenderedCharacterOffset in debug builds; on
     19        release builds, we simply return false from containsRenderedCharacterOffset, which causes us to insert an extra
     20        space.
     21
     22        To fix this, we educate RenderText::containsRenderedCharacterOffset about simple line layout.
     23
     24        Test: editing/inserting/insert-paragraph-in-designmode-document.html
     25
     26        * rendering/RenderText.cpp:
     27        (WebCore::RenderText::containsRenderedCharacterOffset const):
     28        (WebCore::RenderText::containsCaretOffset const):
     29
     30        Changed to use SimpleLineLayout::containsOffset.
     31
     32        * rendering/SimpleLineLayoutFunctions.h:
     33        (WebCore::SimpleLineLayout::containsOffset):
     34
     35        I first contrasted the behavior of RenderTextLineBoxes::containsOffset in the cases where the OffsetType is
     36        CaretOffset or CharacterOffset, and found that the only interesting differences were:
     37
     38        1. The caret offset type case has special handling for line breaks.
     39        2. Both offset types have handling for reversed text.
     40        3. The end offset of a line box contains a caret offset, but not a character offset.
     41
     42        For the purposes of OffsetType CharacterOffset, (1) is irrelevant; furthermore, (2) is already not handled by
     43        logic in containsCaretOffset(). Thus, the only major difference in the CharacterOffset case should be (3), which
     44        we handle by only allowing the case where the given offset is equal to the very end of a text run for caret
     45        offsets, and not character offsets.
     46
     47        (WebCore::SimpleLineLayout::containsCaretOffset): Deleted.
     48
     49        Renamed to just containsOffset.
     50
    1512019-08-07  Youenn Fablet  <youenn@apple.com>
    252
  • trunk/Source/WebCore/rendering/RenderText.cpp

    r246951 r248368  
    14901490bool RenderText::containsRenderedCharacterOffset(unsigned offset) const
    14911491{
    1492     ASSERT(!simpleLineLayout());
     1492    if (auto* layout = simpleLineLayout())
     1493        return SimpleLineLayout::containsOffset(*this, *layout, offset, SimpleLineLayout::OffsetType::CharacterOffset);
    14931494    return m_lineBoxes.containsOffset(*this, offset, RenderTextLineBoxes::CharacterOffset);
    14941495}
     
    14971498{
    14981499    if (auto* layout = simpleLineLayout())
    1499         return SimpleLineLayout::containsCaretOffset(*this, *layout, offset);
     1500        return SimpleLineLayout::containsOffset(*this, *layout, offset, SimpleLineLayout::OffsetType::CaretOffset);
    15001501    return m_lineBoxes.containsOffset(*this, offset, RenderTextLineBoxes::CaretOffset);
    15011502}
  • trunk/Source/WebCore/rendering/SimpleLineLayoutFunctions.h

    r230914 r248368  
    5151
    5252bool isTextRendered(const RenderText&, const Layout&);
    53 bool containsCaretOffset(const RenderObject&, const Layout&, unsigned);
     53enum class OffsetType { CaretOffset, CharacterOffset };
     54bool containsOffset(const RenderText&, const Layout&, unsigned, OffsetType);
    5455unsigned findCaretMinimumOffset(const RenderObject&, const Layout&);
    5556unsigned findCaretMaximumOffset(const RenderObject&, const Layout&);
     
    117118}
    118119
    119 inline bool containsCaretOffset(const RenderText&, const Layout& layout, unsigned offset)
     120inline bool containsOffset(const RenderText&, const Layout& layout, unsigned offset, OffsetType offsetType)
    120121{
    121122    for (unsigned i = 0; i < layout.runCount(); ++i) {
     
    123124        if (offset < run.start)
    124125            return false;
    125         if (offset <= run.end)
     126        if (offset < run.end || (offsetType == OffsetType::CaretOffset && offset == run.end))
    126127            return true;
    127128    }
Note: See TracChangeset for help on using the changeset viewer.