Changeset 232662 in webkit


Ignore:
Timestamp:
Jun 9, 2018 1:15:28 PM (6 years ago)
Author:
rniwa@webkit.org
Message:

REGRESSION(macOS Mojave): move-by-word-visually-multi-line.html fails
​https://bugs.webkit.org/show_bug.cgi?id=186454

Reviewed by Darin Adler.

Source/WebCore:

Like r232635, this patch fixes a selection test failure caused by the change in ICU's behavior in macOS Mojave,
which caused isWordTextBreak to return true in more cases.

In this particular failing test case, previousTextOrLineBreakBox and nextTextOrLineBreakBox were failing to find
an inline text box when it found an inline box for a BR, which was mentioned by an existing FIXME comment.
Consequently, visualWordPosition were erroneously detecting the end of a word followed by a blank line created by
a BR as a valid word boundary to move when the Windows editing behavior is enacted.

Addressed the FIXME comment by finding the next inline text box skipping all inline boxes for BRs. Renamed
misleadingly named previousBoxInDifferentBlock and nextBoxInDifferentBlock to previousBoxInDifferentLine and
nextBoxInDifferentLine respectively, and set them to true as they're really indicating whether line boxes
belong to a distinct line or not; whether an inline box belong to two (render) blocks or not is irrelevant.

Finally, this patch fixes a bug in visualWordPosition that it was failing to skip blank lines when a word break is
found as we traversed past a line break. In those cases, we must skip all line breaks before stopping.

Tests: editing/selection/move-by-word-visually-mac.html

editing/selection/move-by-word-visually-multi-line.htm

  • editing/VisibleUnits.cpp:

(WebCore::CachedLogicallyOrderedLeafBoxes::previousTextOrLineBreakBox):
(WebCore::CachedLogicallyOrderedLeafBoxes::nextTextOrLineBreakBox):
(WebCore::CachedLogicallyOrderedLeafBoxes::boxIndexInLeaves const):
(WebCore::logicallyPreviousBox):
(WebCore::logicallyNextBox):
(WebCore::wordBreakIteratorForMinOffsetBoundary):
(WebCore::wordBreakIteratorForMaxOffsetBoundary):
(WebCore::visualWordPosition):

LayoutTests:

Added a multi-line test case which causes a failure under Mac editing behavior. The test case is symmetric to ml_1.

  • editing/selection/move-by-word-visually-mac-expected.txt:
  • editing/selection/move-by-word-visually-mac.html:
  • editing/selection/move-by-word-visually-multi-line-expected.txt:
  • editing/selection/move-by-word-visually-multi-line.html:
Location:
trunk
Files:
7 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r232650 r232662  
     12018-06-09  Ryosuke Niwa  <rniwa@webkit.org>
     2
     3        REGRESSION(macOS Mojave): move-by-word-visually-multi-line.html fails
     4        https://bugs.webkit.org/show_bug.cgi?id=186454
     5
     6        Reviewed by Darin Adler.
     7
     8        Added a multi-line test case which causes a failure under Mac editing behavior. The test case is symmetric to ml_1.
     9
     10        * editing/selection/move-by-word-visually-mac-expected.txt:
     11        * editing/selection/move-by-word-visually-mac.html:
     12        * editing/selection/move-by-word-visually-multi-line-expected.txt:
     13        * editing/selection/move-by-word-visually-multi-line.html:
     14
    1152018-06-07  Jer Noble  <jer.noble@apple.com>
    216

  • trunk/LayoutTests/editing/selection/move-by-word-visually-mac-expected.txt

    r180159 r232662  
    9393Test 19, LTR:
    9494Move right by one word
     95"abc"[0, 3], " def"[4]
     96Move left by one word
     97" def"[4, 1], "abc"[0]
     98Test 20, LTR:
     99Move right by one word
    95100"abc def    hij opq"[0, 3, 7, 14, 18]
    96101Move left by one word
    97102"abc def    hij opq"[18, 15, 8, 4, 0]
    98 Test 20, LTR:
     103Test 21, LTR:
    99104Move right by one word
    100105"    abc    def    hij    opq    "[4, 7, 14, 21, 28]
    101106Move left by one word
    102107"    abc    def    hij    opq    "[28, 22, 15, 8, 4]
    103 Test 21, RTL:
     108Test 22, RTL:
    104109Move left by one word
    105110"    abc    def    hij    ABW    DSU    EJH    opq    rst    uvw    "[0, 18, 11, 21, 28, 35, 42, 60, 53, 63, 67]
    106111Move right by one word
    107112"    abc    def    hij    ABW    DSU    EJH    opq    rst    uvw    "[67, 49, 56, 46, 39, 32, 25, 7, 14, 4, 0]
    108 Test 22, RTL:
     113Test 23, RTL:
    109114Move left by one word
    110115"    ABW    DSU    HJH    FUX    "[0, 7, 14, 21, 28, 32]
    111116Move right by one word
    112117"    ABW    DSU    HJH    FUX    "[32, 25, 18, 11, 4, 0]
    113 Test 23, RTL:
     118Test 24, RTL:
    114119Move left by one word
    115120"abc def "[0], " rst uvw"[5, 1], "hij opq"[4], "abc def "[8, 4], " rst uvw"[8]
    116121Move right by one word
    117122" rst uvw"[8], "abc def "[3, 7], "hij opq"[3, 7], " rst uvw"[4], "abc def "[0]
    118 Test 24, RTL:
     123Test 25, RTL:
    119124Move left by one word
    120125"ABD opq rst DSU "[0, 3, 8, 11, 15], "abc uvw AAA def lmn"[16, 12, 11, 4, 19], "ABW hij xyz FXX"[3, 8, 11, 15]    FAIL expected: ["ABD opq rst DSU "[ 0,  3,  8,  11,  15, ]"abc uvw AAA def lmn"[ 16,  12,  11,  4, ]"ABW hij xyz FXX"[ 3,  8,  11,  15]

  • trunk/LayoutTests/editing/selection/move-by-word-visually-mac.html

    r180159 r232662  
    7474"> abc def אאא אאא hij אאא אאא uvw xyz <div><br/></div><div><br/></div><div><br/></div>אאא kj אאא mn opq אאא אאא</div>
    7575
     76<div contenteditable dir=ltr id="ml_16" class="test_move_by_word" title="[ml_16, 0][ml_16, 3][ml_16, 4, 4]|[ml_16, 4, 4][ml_16, 1, 4][ml_16, 0]">abc<br><br>&nbsp;def</div>
     77
    7678<!-- test multispaces -->
    7779<div dir=ltr class="test_move_by_word" title="0 3 7 14 18|18 15 8 4 0" contenteditable>abc def    hij opq</div>

  • trunk/LayoutTests/editing/selection/move-by-word-visually-multi-line-expected.txt

    r110965 r232662  
    7878Test 16, LTR:
    7979Move right by one word
     80"abc"[0], " def"[1, 4]
     81Move left by one word
     82" def"[4, 1], "abc"[0]
     83Test 17, LTR:
     84Move right by one word
    8085"abc def "[0, 4, 8]
    8186Move left by one word
    8287" hij opq"[8, 5, 1]
    83 Test 17, LTR:
     88Test 18, LTR:
    8489Move right by one word
    8590<DIV>[0]
    8691Move left by one word
    8792<DIV>[0]
    88 Test 18, LTR:
     93Test 19, LTR:
    8994Move right by one word
    9095"\n00"[3]

  • trunk/LayoutTests/editing/selection/move-by-word-visually-multi-line.html

    r120173 r232662  
    7575<div contenteditable dir=ltr id="ml_15" class="test_move_by_word fix_width" title="[ml_15, 0][ml_15, 4][ml_15, 8][ml_15, 12][ml_15, 16][ml_15, 0, 5][ml_15, 4, 5][ml_15, 8, 5][ml_15, 12, 5][ml_15, 15, 5]|[ml_15, 15, 5][ml_15, 12, 5][ml_15, 8, 5][ml_15, 4, 5][ml_15, 0, 5][ml_15, 16][ml_15, 12][ml_15, 8][ml_15, 4][ml_15, 0]">abc def ghi jkl mn <div><img src=../../accessibility/resources/cake.png></div><div></div><div></div>opq rst uvw xyz</div>
    7676
     77<div contenteditable dir=ltr id="ml_16" class="test_move_by_word" title="[ml_16, 0][ml_16, 1, 4][ml_16, 4, 4]|[ml_16, 4, 4][ml_16, 1, 4][ml_16, 0]">abc<br><br>&nbsp;def</div>
     78
    7779<!-- mixed editability -->
    7880<div dir=ltr class="test_move_by_word" title="0 4 8|8 5 1">abc def <span contenteditable> inside span </span> hij opq</div>

  • trunk/Source/WebCore/ChangeLog

    r232661 r232662  
     12018-06-09  Ryosuke Niwa  <rniwa@webkit.org>
     2
     3        REGRESSION(macOS Mojave): move-by-word-visually-multi-line.html fails
     4        https://bugs.webkit.org/show_bug.cgi?id=186454
     5
     6        Reviewed by Darin Adler.
     7
     8        Like r232635, this patch fixes a selection test failure caused by the change in ICU's behavior in macOS Mojave,
     9        which caused isWordTextBreak to return true in more cases.
     10
     11        In this particular failing test case, previousTextOrLineBreakBox and nextTextOrLineBreakBox were failing to find
     12        an inline text box when it found an inline box for a BR, which was mentioned by an existing FIXME comment.
     13        Consequently, visualWordPosition were erroneously detecting the end of a word followed by a blank line created by
     14        a BR as a valid word boundary to move when the Windows editing behavior is enacted.
     15
     16        Addressed the FIXME comment by finding the next inline text box skipping all inline boxes for BRs. Renamed
     17        misleadingly named previousBoxInDifferentBlock and nextBoxInDifferentBlock to previousBoxInDifferentLine and
     18        nextBoxInDifferentLine respectively, and set them to true as they're really indicating whether line boxes
     19        belong to a distinct line or not; whether an inline box belong to two (render) blocks or not is irrelevant.
     20
     21        Finally, this patch fixes a bug in visualWordPosition that it was failing to skip blank lines when a word break is
     22        found as we traversed past a line break. In those cases, we must skip all line breaks before stopping.
     23
     24        Tests: editing/selection/move-by-word-visually-mac.html
     25               editing/selection/move-by-word-visually-multi-line.htm
     26
     27        * editing/VisibleUnits.cpp:
     28        (WebCore::CachedLogicallyOrderedLeafBoxes::previousTextOrLineBreakBox):
     29        (WebCore::CachedLogicallyOrderedLeafBoxes::nextTextOrLineBreakBox):
     30        (WebCore::CachedLogicallyOrderedLeafBoxes::boxIndexInLeaves const):
     31        (WebCore::logicallyPreviousBox):
     32        (WebCore::logicallyNextBox):
     33        (WebCore::wordBreakIteratorForMinOffsetBoundary):
     34        (WebCore::wordBreakIteratorForMaxOffsetBoundary):
     35        (WebCore::visualWordPosition):
     36
    1372018-06-09  Zalan Bujtas  <zalan@apple.com>
    238

  • trunk/Source/WebCore/editing/VisibleUnits.cpp

    r232635 r232662  
    126126    CachedLogicallyOrderedLeafBoxes();
    127127
    128     const InlineBox* previousTextOrLineBreakBox(const RootInlineBox*, const InlineTextBox*);
    129     const InlineBox* nextTextOrLineBreakBox(const RootInlineBox*, const InlineTextBox*);
     128    const InlineBox* previousTextOrLineBreakBox(const RootInlineBox*, const InlineBox*);
     129    const InlineBox* nextTextOrLineBreakBox(const RootInlineBox*, const InlineBox*);
    130130
    131131    size_t size() const { return m_leafBoxes.size(); }
    … …  
    134134private:
    135135    const Vector<InlineBox*>& collectBoxes(const RootInlineBox*);
    136     int boxIndexInLeaves(const InlineTextBox*) const;
     136    int boxIndexInLeaves(const InlineBox*) const;
    137137
    138138    const RootInlineBox* m_rootInlineBox { nullptr };
    … …  
    144144}
    145145
    146 const InlineBox* CachedLogicallyOrderedLeafBoxes::previousTextOrLineBreakBox(const RootInlineBox* root, const InlineTextBox* box)
     146const InlineBox* CachedLogicallyOrderedLeafBoxes::previousTextOrLineBreakBox(const RootInlineBox* root, const InlineBox* box)
    147147{
    148148    if (!root)
    … …  
    165165}
    166166
    167 const InlineBox* CachedLogicallyOrderedLeafBoxes::nextTextOrLineBreakBox(const RootInlineBox* root, const InlineTextBox* box)
     167const InlineBox* CachedLogicallyOrderedLeafBoxes::nextTextOrLineBreakBox(const RootInlineBox* root, const InlineBox* box)
    168168{
    169169    if (!root)
    … …  
    197197}
    198198
    199 int CachedLogicallyOrderedLeafBoxes::boxIndexInLeaves(const InlineTextBox* box) const
     199int CachedLogicallyOrderedLeafBoxes::boxIndexInLeaves(const InlineBox* box) const
    200200{
    201201    for (size_t i = 0; i < m_leafBoxes.size(); ++i) {
    … …  
    206206}
    207207
    208 static const InlineBox* logicallyPreviousBox(const VisiblePosition& visiblePosition, const InlineTextBox* textBox,
    209     bool& previousBoxInDifferentBlock, CachedLogicallyOrderedLeafBoxes& leafBoxes)
     208static const InlineBox* logicallyPreviousBox(const VisiblePosition& visiblePosition, const InlineBox* textBox,
     209    bool& previousBoxInDifferentLine, CachedLogicallyOrderedLeafBoxes& leafBoxes)
    210210{
    211211    const InlineBox* startBox = textBox;
    … …  
    235235        previousBox = leafBoxes.previousTextOrLineBreakBox(previousRoot, 0);
    236236        if (previousBox) {
    237             previousBoxInDifferentBlock = true;
     237            previousBoxInDifferentLine = true;
    238238            return previousBox;
    239239        }
    … …  
    247247
    248248
    249 static const InlineBox* logicallyNextBox(const VisiblePosition& visiblePosition, const InlineTextBox* textBox,
    250     bool& nextBoxInDifferentBlock, CachedLogicallyOrderedLeafBoxes& leafBoxes)
     249static const InlineBox* logicallyNextBox(const VisiblePosition& visiblePosition, const InlineBox* textBox,
     250    bool& nextBoxInDifferentLine, CachedLogicallyOrderedLeafBoxes& leafBoxes)
    251251{
    252252    const InlineBox* startBox = textBox;
    … …  
    276276        nextBox = leafBoxes.nextTextOrLineBreakBox(nextRoot, 0);
    277277        if (nextBox) {
    278             nextBoxInDifferentBlock = true;
     278            nextBoxInDifferentLine = true;
    279279            return nextBox;
    280280        }
    … …  
    288288
    289289static UBreakIterator* wordBreakIteratorForMinOffsetBoundary(const VisiblePosition& visiblePosition, const InlineTextBox* textBox,
    290     int& previousBoxLength, bool& previousBoxInDifferentBlock, Vector<UChar, 1024>& string, CachedLogicallyOrderedLeafBoxes& leafBoxes)
    291 {
    292     previousBoxInDifferentBlock = false;
    293 
    294     // FIXME: Handle the case when we don't have an inline text box.
    295     const InlineBox* previousBox = logicallyPreviousBox(visiblePosition, textBox, previousBoxInDifferentBlock, leafBoxes);
     290    int& previousBoxLength, bool& previousBoxInDifferentLine, Vector<UChar, 1024>& string, CachedLogicallyOrderedLeafBoxes& leafBoxes)
     291{
     292    previousBoxInDifferentLine = false;
     293
     294    const InlineBox* previousBox = logicallyPreviousBox(visiblePosition, textBox, previousBoxInDifferentLine, leafBoxes);
     295    while (previousBox && !is<InlineTextBox>(previousBox)) {
     296        ASSERT(previousBox->renderer().isBR());
     297        previousBoxInDifferentLine = true;
     298        previousBox = logicallyPreviousBox(visiblePosition, previousBox, previousBoxInDifferentLine, leafBoxes);
     299    }
    296300
    297301    string.clear();
    … …  
    308312
    309313static UBreakIterator* wordBreakIteratorForMaxOffsetBoundary(const VisiblePosition& visiblePosition, const InlineTextBox* textBox,
    310     bool& nextBoxInDifferentBlock, Vector<UChar, 1024>& string, CachedLogicallyOrderedLeafBoxes& leafBoxes)
    311 {
    312     nextBoxInDifferentBlock = false;
    313 
    314     // FIXME: Handle the case when we don't have an inline text box.
    315     const InlineBox* nextBox = logicallyNextBox(visiblePosition, textBox, nextBoxInDifferentBlock, leafBoxes);
     314    bool& nextBoxInDifferentLine, Vector<UChar, 1024>& string, CachedLogicallyOrderedLeafBoxes& leafBoxes)
     315{
     316    nextBoxInDifferentLine = false;
     317
     318    const InlineBox* nextBox = logicallyNextBox(visiblePosition, textBox, nextBoxInDifferentLine, leafBoxes);
     319    while (nextBox && !is<InlineTextBox>(nextBox)) {
     320        ASSERT(nextBox->renderer().isBR());
     321        nextBoxInDifferentLine = true;
     322        nextBox = logicallyNextBox(visiblePosition, nextBox, nextBoxInDifferentLine, leafBoxes);
     323    }
    316324
    317325    string.clear();
    … …  
    380388        InlineTextBox& textBox = downcast<InlineTextBox>(*box);
    381389        int previousBoxLength = 0;
    382         bool previousBoxInDifferentBlock = false;
    383         bool nextBoxInDifferentBlock = false;
     390        bool previousBoxInDifferentLine = false;
     391        bool nextBoxInDifferentLine = false;
    384392        bool movingIntoNewBox = previouslyVisitedBox != box;
    385393
    386394        if (offsetInBox == box->caretMinOffset())
    387             iter = wordBreakIteratorForMinOffsetBoundary(adjacentCharacterPosition, &textBox, previousBoxLength, previousBoxInDifferentBlock, string, leafBoxes);
     395            iter = wordBreakIteratorForMinOffsetBoundary(adjacentCharacterPosition, &textBox, previousBoxLength, previousBoxInDifferentLine, string, leafBoxes);
    388396        else if (offsetInBox == box->caretMaxOffset())
    389             iter = wordBreakIteratorForMaxOffsetBoundary(adjacentCharacterPosition, &textBox, nextBoxInDifferentBlock, string, leafBoxes);
     397            iter = wordBreakIteratorForMaxOffsetBoundary(adjacentCharacterPosition, &textBox, nextBoxInDifferentLine, string, leafBoxes);
    390398        else if (movingIntoNewBox) {
    391399            iter = wordBreakIterator(StringView(textBox.renderer().text()).substring(textBox.start(), textBox.len()));
    … …  
    404412        if ((skipsSpaceWhenMovingRight && boxHasSameDirectionalityAsBlock)
    405413            || (!skipsSpaceWhenMovingRight && movingBackward)) {
    406             bool logicalStartInRenderer = offsetInBox == static_cast<int>(textBox.start()) && previousBoxInDifferentBlock;
     414            bool logicalStartInRenderer = offsetInBox == static_cast<int>(textBox.start()) && previousBoxInDifferentLine;
    407415            isWordBreak = isLogicalStartOfWord(iter, offsetInIterator, logicalStartInRenderer);
     416            if (isWordBreak && offsetInBox == box->caretMaxOffset() && nextBoxInDifferentLine)
     417                isWordBreak = false;
    408418        } else {
    409             bool logicalEndInRenderer = offsetInBox == static_cast<int>(textBox.start() + textBox.len()) && nextBoxInDifferentBlock;
     419            bool logicalEndInRenderer = offsetInBox == static_cast<int>(textBox.start() + textBox.len()) && nextBoxInDifferentLine;
    410420            isWordBreak = islogicalEndOfWord(iter, offsetInIterator, logicalEndInRenderer);
     421            if (isWordBreak && offsetInBox == box->caretMinOffset() && previousBoxInDifferentLine)
     422                isWordBreak = false;
    411423        }     
    412424
Note: See TracChangeset for help on using the changeset viewer.