⚠ Archived content — this site is no longer maintained.   Current WebKit documentation is at docs.webkit.org.

Changeset 248375 in webkit


Ignore:
Timestamp:
Aug 7, 2019, 10:25:37 AM (7 years ago)
Author:
Alan Coon
Message:

Cherry-pick r248368. rdar://problem/54037160

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

Location:
branches/safari-608.1-branch
Files:
2 added
4 edited

Legend:

Unmodified
Added
Removed

  • branches/safari-608.1-branch/LayoutTests/ChangeLog

    r248361 r248375  
     12019-08-07  Kocsen Chung  <kocsen_chung@apple.com>
     2
     3        Cherry-pick r248368. rdar://problem/54037160
     4
     5    Extra space inserted at start of line when inserting a newline in Mail compose
     6    https://bugs.webkit.org/show_bug.cgi?id=200490
     7    <rdar://problem/53501354>
     8   
     9    Reviewed by Antti Koivisto.
     10   
     11    Source/WebCore:
     12   
     13    This started happening after r244494, which deferred editor state computation until the next layer tree flush
     14    when changing selection. After inserting a paragraph, the act of computing an editor state ensured that the text
     15    node containing the caret drops out of simple line layout, while grabbing the characters near the selection
     16    (i.e., calling charactersAroundPosition). This meant that when we subsequently ask positionAfterSplit whether it
     17    isRenderedCharacter() at the end of the command, we are guaranteed to have line boxes, so we get a meaningful
     18    answer and avoid inserting an extra non-breaking space.
     19   
     20    However, after r244494, we defer the editor state computation until the end of the edit command; this means that
     21    we may not have line boxes for positionAfterSplit's text node renderer, due to remaining in simple line layout.
     22    In turn, this means that we end up hitting the assertion in containsRenderedCharacterOffset in debug builds; on
     23    release builds, we simply return false from containsRenderedCharacterOffset, which causes us to insert an extra
     24    space.
     25   
     26    To fix this, we educate RenderText::containsRenderedCharacterOffset about simple line layout.
     27   
     28    Test: editing/inserting/insert-paragraph-in-designmode-document.html
     29   
     30    * rendering/RenderText.cpp:
     31    (WebCore::RenderText::containsRenderedCharacterOffset const):
     32    (WebCore::RenderText::containsCaretOffset const):
     33   
     34    Changed to use SimpleLineLayout::containsOffset.
     35   
     36    * rendering/SimpleLineLayoutFunctions.h:
     37    (WebCore::SimpleLineLayout::containsOffset):
     38   
     39    I first contrasted the behavior of RenderTextLineBoxes::containsOffset in the cases where the OffsetType is
     40    CaretOffset or CharacterOffset, and found that the only interesting differences were:
     41   
     42    1. The caret offset type case has special handling for line breaks.
     43    2. Both offset types have handling for reversed text.
     44    3. The end offset of a line box contains a caret offset, but not a character offset.
     45   
     46    For the purposes of OffsetType CharacterOffset, (1) is irrelevant; furthermore, (2) is already not handled by
     47    logic in containsCaretOffset(). Thus, the only major difference in the CharacterOffset case should be (3), which
     48    we handle by only allowing the case where the given offset is equal to the very end of a text run for caret
     49    offsets, and not character offsets.
     50   
     51    (WebCore::SimpleLineLayout::containsCaretOffset): Deleted.
     52   
     53    Renamed to just containsOffset.
     54   
     55    LayoutTests:
     56   
     57    Add a new test to verify that inserting a newline in the middle of text in a document with designMode "on"
     58    doesn't insert an extra space at the beginning of the newly inserted line.
     59   
     60    * editing/inserting/insert-paragraph-in-designmode-document-expected.txt: Added.
     61    * editing/inserting/insert-paragraph-in-designmode-document.html: Added.
     62   
     63   
     64    git-svn-id: https://svn.webkit.org/repository/webkit/trunk@248368 268f45cc-cd09-0410-ab3c-d52691b4dbfc
     65
     66    2019-08-07  Wenson Hsieh  <wenson_hsieh@apple.com>
     67
     68            Extra space inserted at start of line when inserting a newline in Mail compose
     69            https://bugs.webkit.org/show_bug.cgi?id=200490
     70            <rdar://problem/53501354>
     71
     72            Reviewed by Antti Koivisto.
     73
     74            Add a new test to verify that inserting a newline in the middle of text in a document with designMode "on"
     75            doesn't insert an extra space at the beginning of the newly inserted line.
     76
     77            * editing/inserting/insert-paragraph-in-designmode-document-expected.txt: Added.
     78            * editing/inserting/insert-paragraph-in-designmode-document.html: Added.
     79
    1802019-08-06  Kocsen Chung  <kocsen_chung@apple.com>
    281

  • branches/safari-608.1-branch/Source/WebCore/ChangeLog

    r248369 r248375  
     12019-08-07  Kocsen Chung  <kocsen_chung@apple.com>
     2
     3        Cherry-pick r248368. rdar://problem/54037160
     4
     5    Extra space inserted at start of line when inserting a newline in Mail compose
     6    https://bugs.webkit.org/show_bug.cgi?id=200490
     7    <rdar://problem/53501354>
     8   
     9    Reviewed by Antti Koivisto.
     10   
     11    Source/WebCore:
     12   
     13    This started happening after r244494, which deferred editor state computation until the next layer tree flush
     14    when changing selection. After inserting a paragraph, the act of computing an editor state ensured that the text
     15    node containing the caret drops out of simple line layout, while grabbing the characters near the selection
     16    (i.e., calling charactersAroundPosition). This meant that when we subsequently ask positionAfterSplit whether it
     17    isRenderedCharacter() at the end of the command, we are guaranteed to have line boxes, so we get a meaningful
     18    answer and avoid inserting an extra non-breaking space.
     19   
     20    However, after r244494, we defer the editor state computation until the end of the edit command; this means that
     21    we may not have line boxes for positionAfterSplit's text node renderer, due to remaining in simple line layout.
     22    In turn, this means that we end up hitting the assertion in containsRenderedCharacterOffset in debug builds; on
     23    release builds, we simply return false from containsRenderedCharacterOffset, which causes us to insert an extra
     24    space.
     25   
     26    To fix this, we educate RenderText::containsRenderedCharacterOffset about simple line layout.
     27   
     28    Test: editing/inserting/insert-paragraph-in-designmode-document.html
     29   
     30    * rendering/RenderText.cpp:
     31    (WebCore::RenderText::containsRenderedCharacterOffset const):
     32    (WebCore::RenderText::containsCaretOffset const):
     33   
     34    Changed to use SimpleLineLayout::containsOffset.
     35   
     36    * rendering/SimpleLineLayoutFunctions.h:
     37    (WebCore::SimpleLineLayout::containsOffset):
     38   
     39    I first contrasted the behavior of RenderTextLineBoxes::containsOffset in the cases where the OffsetType is
     40    CaretOffset or CharacterOffset, and found that the only interesting differences were:
     41   
     42    1. The caret offset type case has special handling for line breaks.
     43    2. Both offset types have handling for reversed text.
     44    3. The end offset of a line box contains a caret offset, but not a character offset.
     45   
     46    For the purposes of OffsetType CharacterOffset, (1) is irrelevant; furthermore, (2) is already not handled by
     47    logic in containsCaretOffset(). Thus, the only major difference in the CharacterOffset case should be (3), which
     48    we handle by only allowing the case where the given offset is equal to the very end of a text run for caret
     49    offsets, and not character offsets.
     50   
     51    (WebCore::SimpleLineLayout::containsCaretOffset): Deleted.
     52   
     53    Renamed to just containsOffset.
     54   
     55    LayoutTests:
     56   
     57    Add a new test to verify that inserting a newline in the middle of text in a document with designMode "on"
     58    doesn't insert an extra space at the beginning of the newly inserted line.
     59   
     60    * editing/inserting/insert-paragraph-in-designmode-document-expected.txt: Added.
     61    * editing/inserting/insert-paragraph-in-designmode-document.html: Added.
     62   
     63   
     64    git-svn-id: https://svn.webkit.org/repository/webkit/trunk@248368 268f45cc-cd09-0410-ab3c-d52691b4dbfc
     65
     66    2019-08-07  Wenson Hsieh  <wenson_hsieh@apple.com>
     67
     68            Extra space inserted at start of line when inserting a newline in Mail compose
     69            https://bugs.webkit.org/show_bug.cgi?id=200490
     70            <rdar://problem/53501354>
     71
     72            Reviewed by Antti Koivisto.
     73
     74            This started happening after r244494, which deferred editor state computation until the next layer tree flush
     75            when changing selection. After inserting a paragraph, the act of computing an editor state ensured that the text
     76            node containing the caret drops out of simple line layout, while grabbing the characters near the selection
     77            (i.e., calling charactersAroundPosition). This meant that when we subsequently ask positionAfterSplit whether it
     78            isRenderedCharacter() at the end of the command, we are guaranteed to have line boxes, so we get a meaningful
     79            answer and avoid inserting an extra non-breaking space.
     80
     81            However, after r244494, we defer the editor state computation until the end of the edit command; this means that
     82            we may not have line boxes for positionAfterSplit's text node renderer, due to remaining in simple line layout.
     83            In turn, this means that we end up hitting the assertion in containsRenderedCharacterOffset in debug builds; on
     84            release builds, we simply return false from containsRenderedCharacterOffset, which causes us to insert an extra
     85            space.
     86
     87            To fix this, we educate RenderText::containsRenderedCharacterOffset about simple line layout.
     88
     89            Test: editing/inserting/insert-paragraph-in-designmode-document.html
     90
     91            * rendering/RenderText.cpp:
     92            (WebCore::RenderText::containsRenderedCharacterOffset const):
     93            (WebCore::RenderText::containsCaretOffset const):
     94
     95            Changed to use SimpleLineLayout::containsOffset.
     96
     97            * rendering/SimpleLineLayoutFunctions.h:
     98            (WebCore::SimpleLineLayout::containsOffset):
     99
     100            I first contrasted the behavior of RenderTextLineBoxes::containsOffset in the cases where the OffsetType is
     101            CaretOffset or CharacterOffset, and found that the only interesting differences were:
     102
     103            1. The caret offset type case has special handling for line breaks.
     104            2. Both offset types have handling for reversed text.
     105            3. The end offset of a line box contains a caret offset, but not a character offset.
     106
     107            For the purposes of OffsetType CharacterOffset, (1) is irrelevant; furthermore, (2) is already not handled by
     108            logic in containsCaretOffset(). Thus, the only major difference in the CharacterOffset case should be (3), which
     109            we handle by only allowing the case where the given offset is equal to the very end of a text run for caret
     110            offsets, and not character offsets.
     111
     112            (WebCore::SimpleLineLayout::containsCaretOffset): Deleted.
     113
     114            Renamed to just containsOffset.
     115
    11162019-08-07  Kocsen Chung  <kocsen_chung@apple.com>
    2117

  • branches/safari-608.1-branch/Source/WebCore/rendering/RenderText.cpp

    r246951 r248375  
    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}

  • branches/safari-608.1-branch/Source/WebCore/rendering/SimpleLineLayoutFunctions.h

    r230914 r248375  
    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.