REGRESSION(macOS Mojave): move-by-word-visually-multi-line.html fails
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 9 Jun 2018 20:15:28 +0000 (20:15 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 9 Jun 2018 20:15:28 +0000 (20:15 +0000)
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:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@232662 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/editing/selection/move-by-word-visually-mac-expected.txt
LayoutTests/editing/selection/move-by-word-visually-mac.html
LayoutTests/editing/selection/move-by-word-visually-multi-line-expected.txt
LayoutTests/editing/selection/move-by-word-visually-multi-line.html
Source/WebCore/ChangeLog
Source/WebCore/editing/VisibleUnits.cpp

index e4839f2..06230ad 100644 (file)
@@ -1,3 +1,17 @@
+2018-06-09  Ryosuke Niwa  <rniwa@webkit.org>
+
+        REGRESSION(macOS Mojave): move-by-word-visually-multi-line.html fails
+        https://bugs.webkit.org/show_bug.cgi?id=186454
+
+        Reviewed by Darin Adler.
+
+        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:
+
 2018-06-07  Jer Noble  <jer.noble@apple.com>
 
         REGRESSION:  Cannot listen to audio on Google Translate with side switch set to "vibrate"
index 5cedefd..dfae7d2 100644 (file)
@@ -92,30 +92,35 @@ Move right by one word
 "AAA kj AAA mn opq AAA AAA"[25, 22, 18, 14, 11, 7, 4, 0], " abc def AAA AAA hij AAA AAA uvw xyz "[32, 29, 25, 21, 17, 13, 9, 4, 1]
 Test 19, LTR:
 Move right by one word
+"abc"[0, 3], " def"[4]
+Move left by one word
+" def"[4, 1], "abc"[0]
+Test 20, LTR:
+Move right by one word
 "abc def    hij opq"[0, 3, 7, 14, 18]
 Move left by one word
 "abc def    hij opq"[18, 15, 8, 4, 0]
-Test 20, LTR:
+Test 21, LTR:
 Move right by one word
 "    abc    def    hij    opq    "[4, 7, 14, 21, 28]
 Move left by one word
 "    abc    def    hij    opq    "[28, 22, 15, 8, 4]
-Test 21, RTL:
+Test 22, RTL:
 Move left by one word
 "    abc    def    hij    ABW    DSU    EJH    opq    rst    uvw    "[0, 18, 11, 21, 28, 35, 42, 60, 53, 63, 67]
 Move right by one word
 "    abc    def    hij    ABW    DSU    EJH    opq    rst    uvw    "[67, 49, 56, 46, 39, 32, 25, 7, 14, 4, 0]
-Test 22, RTL:
+Test 23, RTL:
 Move left by one word
 "    ABW    DSU    HJH    FUX    "[0, 7, 14, 21, 28, 32]
 Move right by one word
 "    ABW    DSU    HJH    FUX    "[32, 25, 18, 11, 4, 0]
-Test 23, RTL:
+Test 24, RTL:
 Move left by one word
 "abc def "[0], " rst uvw"[5, 1], "hij opq"[4], "abc def "[8, 4], " rst uvw"[8]
 Move right by one word
 " rst uvw"[8], "abc def "[3, 7], "hij opq"[3, 7], " rst uvw"[4], "abc def "[0]
-Test 24, RTL:
+Test 25, RTL:
 Move left by one word
 "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]
 "abc uvw AAA def lmn"[4, 19]   FAIL expected "ABW hij xyz FXX"[ 3]
index 342a99c..d583aa9 100644 (file)
@@ -73,6 +73,8 @@ where child_node_index is optional default is the first child of the anchor node
 [ml_12, 25, 5][ml_12, 22, 5][ml_12, 18, 5][ml_12, 14, 5][ml_12, 11, 5][ml_12, 7, 5][ml_12, 4, 5][ml_12, 0, 5][ml_12, 32][ml_12, 29][ml_12, 25][ml_12, 21][ml_12, 17][ml_12, 13][ml_12, 9][ml_12, 4][ml_12, 1]|[ml_12, 1][ml_12, 5][ml_12, 8][ml_12, 12][ml_12, 16][ml_12, 20][ml_12, 24][ml_12, 28][ml_12, 33][ml_12, 36][ml_12, 3, 5][ml_12, 6, 5][ml_12, 10, 5][ml_12, 13, 5][ml_12, 17, 5][ml_12, 21, 5][ml_12, 25, 5]
 "> abc def אאא אאא hij אאא אאא uvw xyz <div><br/></div><div><br/></div><div><br/></div>אאא kj אאא mn opq אאא אאא</div>
 
+<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>
+
 <!-- test multispaces -->
 <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>
 
index 6ba17a7..918d0da 100644 (file)
@@ -77,15 +77,20 @@ Move left by one word
 "opq rst uvw xyz"[15, 12, 8, 4, 0], "abc def ghi jkl mn "[16, 12, 8, 4, 0]
 Test 16, LTR:
 Move right by one word
+"abc"[0], " def"[1, 4]
+Move left by one word
+" def"[4, 1], "abc"[0]
+Test 17, LTR:
+Move right by one word
 "abc def "[0, 4, 8]
 Move left by one word
 " hij opq"[8, 5, 1]
-Test 17, LTR:
+Test 18, LTR:
 Move right by one word
 <DIV>[0]
 Move left by one word
 <DIV>[0]
-Test 18, LTR:
+Test 19, LTR:
 Move right by one word
 "\n00"[3]
 Move left by one word
index 769821c..8a54c38 100644 (file)
@@ -74,6 +74,8 @@ where child_node_index is optional, default is the first child of the anchor nod
 
 <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>
 
+<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>
+
 <!-- mixed editability -->
 <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>
 
index e744f9b..16a85a1 100644 (file)
@@ -1,3 +1,39 @@
+2018-06-09  Ryosuke Niwa  <rniwa@webkit.org>
+
+        REGRESSION(macOS Mojave): move-by-word-visually-multi-line.html fails
+        https://bugs.webkit.org/show_bug.cgi?id=186454
+
+        Reviewed by Darin Adler.
+
+        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):
+
 2018-06-09  Zalan Bujtas  <zalan@apple.com>
 
         [LFC] MarginCollapse functions should be able to resolve non-fixed margin values
index 55f2b70..99fa90d 100644 (file)
@@ -125,15 +125,15 @@ class CachedLogicallyOrderedLeafBoxes {
 public:
     CachedLogicallyOrderedLeafBoxes();
 
-    const InlineBox* previousTextOrLineBreakBox(const RootInlineBox*, const InlineTextBox*);
-    const InlineBox* nextTextOrLineBreakBox(const RootInlineBox*, const InlineTextBox*);
+    const InlineBox* previousTextOrLineBreakBox(const RootInlineBox*, const InlineBox*);
+    const InlineBox* nextTextOrLineBreakBox(const RootInlineBox*, const InlineBox*);
 
     size_t size() const { return m_leafBoxes.size(); }
     const InlineBox* firstBox() const { return m_leafBoxes[0]; }
 
 private:
     const Vector<InlineBox*>& collectBoxes(const RootInlineBox*);
-    int boxIndexInLeaves(const InlineTextBox*) const;
+    int boxIndexInLeaves(const InlineBox*) const;
 
     const RootInlineBox* m_rootInlineBox { nullptr };
     Vector<InlineBox*> m_leafBoxes;
@@ -143,7 +143,7 @@ CachedLogicallyOrderedLeafBoxes::CachedLogicallyOrderedLeafBoxes()
 {
 }
 
-const InlineBox* CachedLogicallyOrderedLeafBoxes::previousTextOrLineBreakBox(const RootInlineBox* root, const InlineTextBox* box)
+const InlineBox* CachedLogicallyOrderedLeafBoxes::previousTextOrLineBreakBox(const RootInlineBox* root, const InlineBox* box)
 {
     if (!root)
         return nullptr;
@@ -164,7 +164,7 @@ const InlineBox* CachedLogicallyOrderedLeafBoxes::previousTextOrLineBreakBox(con
     return nullptr;
 }
 
-const InlineBox* CachedLogicallyOrderedLeafBoxes::nextTextOrLineBreakBox(const RootInlineBox* root, const InlineTextBox* box)
+const InlineBox* CachedLogicallyOrderedLeafBoxes::nextTextOrLineBreakBox(const RootInlineBox* root, const InlineBox* box)
 {
     if (!root)
         return nullptr;
@@ -196,7 +196,7 @@ const Vector<InlineBox*>& CachedLogicallyOrderedLeafBoxes::collectBoxes(const Ro
     return m_leafBoxes;
 }
 
-int CachedLogicallyOrderedLeafBoxes::boxIndexInLeaves(const InlineTextBox* box) const
+int CachedLogicallyOrderedLeafBoxes::boxIndexInLeaves(const InlineBox* box) const
 {
     for (size_t i = 0; i < m_leafBoxes.size(); ++i) {
         if (box == m_leafBoxes[i])
@@ -205,8 +205,8 @@ int CachedLogicallyOrderedLeafBoxes::boxIndexInLeaves(const InlineTextBox* box)
     return 0;
 }
 
-static const InlineBox* logicallyPreviousBox(const VisiblePosition& visiblePosition, const InlineTextBox* textBox,
-    bool& previousBoxInDifferentBlock, CachedLogicallyOrderedLeafBoxes& leafBoxes)
+static const InlineBox* logicallyPreviousBox(const VisiblePosition& visiblePosition, const InlineBox* textBox,
+    bool& previousBoxInDifferentLine, CachedLogicallyOrderedLeafBoxes& leafBoxes)
 {
     const InlineBox* startBox = textBox;
 
@@ -234,7 +234,7 @@ static const InlineBox* logicallyPreviousBox(const VisiblePosition& visiblePosit
 
         previousBox = leafBoxes.previousTextOrLineBreakBox(previousRoot, 0);
         if (previousBox) {
-            previousBoxInDifferentBlock = true;
+            previousBoxInDifferentLine = true;
             return previousBox;
         }
 
@@ -246,8 +246,8 @@ static const InlineBox* logicallyPreviousBox(const VisiblePosition& visiblePosit
 }
 
 
-static const InlineBox* logicallyNextBox(const VisiblePosition& visiblePosition, const InlineTextBox* textBox,
-    bool& nextBoxInDifferentBlock, CachedLogicallyOrderedLeafBoxes& leafBoxes)
+static const InlineBox* logicallyNextBox(const VisiblePosition& visiblePosition, const InlineBox* textBox,
+    bool& nextBoxInDifferentLine, CachedLogicallyOrderedLeafBoxes& leafBoxes)
 {
     const InlineBox* startBox = textBox;
 
@@ -275,7 +275,7 @@ static const InlineBox* logicallyNextBox(const VisiblePosition& visiblePosition,
 
         nextBox = leafBoxes.nextTextOrLineBreakBox(nextRoot, 0);
         if (nextBox) {
-            nextBoxInDifferentBlock = true;
+            nextBoxInDifferentLine = true;
             return nextBox;
         }
 
@@ -287,12 +287,16 @@ static const InlineBox* logicallyNextBox(const VisiblePosition& visiblePosition,
 }
 
 static UBreakIterator* wordBreakIteratorForMinOffsetBoundary(const VisiblePosition& visiblePosition, const InlineTextBox* textBox,
-    int& previousBoxLength, bool& previousBoxInDifferentBlock, Vector<UChar, 1024>& string, CachedLogicallyOrderedLeafBoxes& leafBoxes)
+    int& previousBoxLength, bool& previousBoxInDifferentLine, Vector<UChar, 1024>& string, CachedLogicallyOrderedLeafBoxes& leafBoxes)
 {
-    previousBoxInDifferentBlock = false;
+    previousBoxInDifferentLine = false;
 
-    // FIXME: Handle the case when we don't have an inline text box.
-    const InlineBox* previousBox = logicallyPreviousBox(visiblePosition, textBox, previousBoxInDifferentBlock, leafBoxes);
+    const InlineBox* previousBox = logicallyPreviousBox(visiblePosition, textBox, previousBoxInDifferentLine, leafBoxes);
+    while (previousBox && !is<InlineTextBox>(previousBox)) {
+        ASSERT(previousBox->renderer().isBR());
+        previousBoxInDifferentLine = true;
+        previousBox = logicallyPreviousBox(visiblePosition, previousBox, previousBoxInDifferentLine, leafBoxes);
+    }
 
     string.clear();
 
@@ -307,12 +311,16 @@ static UBreakIterator* wordBreakIteratorForMinOffsetBoundary(const VisiblePositi
 }
 
 static UBreakIterator* wordBreakIteratorForMaxOffsetBoundary(const VisiblePosition& visiblePosition, const InlineTextBox* textBox,
-    bool& nextBoxInDifferentBlock, Vector<UChar, 1024>& string, CachedLogicallyOrderedLeafBoxes& leafBoxes)
+    bool& nextBoxInDifferentLine, Vector<UChar, 1024>& string, CachedLogicallyOrderedLeafBoxes& leafBoxes)
 {
-    nextBoxInDifferentBlock = false;
+    nextBoxInDifferentLine = false;
 
-    // FIXME: Handle the case when we don't have an inline text box.
-    const InlineBox* nextBox = logicallyNextBox(visiblePosition, textBox, nextBoxInDifferentBlock, leafBoxes);
+    const InlineBox* nextBox = logicallyNextBox(visiblePosition, textBox, nextBoxInDifferentLine, leafBoxes);
+    while (nextBox && !is<InlineTextBox>(nextBox)) {
+        ASSERT(nextBox->renderer().isBR());
+        nextBoxInDifferentLine = true;
+        nextBox = logicallyNextBox(visiblePosition, nextBox, nextBoxInDifferentLine, leafBoxes);
+    }
 
     string.clear();
     append(string, StringView(textBox->renderer().text()).substring(textBox->start(), textBox->len()));
@@ -379,14 +387,14 @@ static VisiblePosition visualWordPosition(const VisiblePosition& visiblePosition
 
         InlineTextBox& textBox = downcast<InlineTextBox>(*box);
         int previousBoxLength = 0;
-        bool previousBoxInDifferentBlock = false;
-        bool nextBoxInDifferentBlock = false;
+        bool previousBoxInDifferentLine = false;
+        bool nextBoxInDifferentLine = false;
         bool movingIntoNewBox = previouslyVisitedBox != box;
 
         if (offsetInBox == box->caretMinOffset())
-            iter = wordBreakIteratorForMinOffsetBoundary(adjacentCharacterPosition, &textBox, previousBoxLength, previousBoxInDifferentBlock, string, leafBoxes);
+            iter = wordBreakIteratorForMinOffsetBoundary(adjacentCharacterPosition, &textBox, previousBoxLength, previousBoxInDifferentLine, string, leafBoxes);
         else if (offsetInBox == box->caretMaxOffset())
-            iter = wordBreakIteratorForMaxOffsetBoundary(adjacentCharacterPosition, &textBox, nextBoxInDifferentBlock, string, leafBoxes);
+            iter = wordBreakIteratorForMaxOffsetBoundary(adjacentCharacterPosition, &textBox, nextBoxInDifferentLine, string, leafBoxes);
         else if (movingIntoNewBox) {
             iter = wordBreakIterator(StringView(textBox.renderer().text()).substring(textBox.start(), textBox.len()));
             previouslyVisitedBox = box;
@@ -403,11 +411,15 @@ static VisiblePosition visualWordPosition(const VisiblePosition& visiblePosition
         bool movingBackward = (direction == MoveLeft && box->direction() == LTR) || (direction == MoveRight && box->direction() == RTL);
         if ((skipsSpaceWhenMovingRight && boxHasSameDirectionalityAsBlock)
             || (!skipsSpaceWhenMovingRight && movingBackward)) {
-            bool logicalStartInRenderer = offsetInBox == static_cast<int>(textBox.start()) && previousBoxInDifferentBlock;
+            bool logicalStartInRenderer = offsetInBox == static_cast<int>(textBox.start()) && previousBoxInDifferentLine;
             isWordBreak = isLogicalStartOfWord(iter, offsetInIterator, logicalStartInRenderer);
+            if (isWordBreak && offsetInBox == box->caretMaxOffset() && nextBoxInDifferentLine)
+                isWordBreak = false;
         } else {
-            bool logicalEndInRenderer = offsetInBox == static_cast<int>(textBox.start() + textBox.len()) && nextBoxInDifferentBlock;
+            bool logicalEndInRenderer = offsetInBox == static_cast<int>(textBox.start() + textBox.len()) && nextBoxInDifferentLine;
             isWordBreak = islogicalEndOfWord(iter, offsetInIterator, logicalEndInRenderer);
+            if (isWordBreak && offsetInBox == box->caretMinOffset() && previousBoxInDifferentLine)
+                isWordBreak = false;
         }      
 
         if (isWordBreak)