Attachment 342363 Details for Bug 186454 – Fixes the bug

[patch] Fixes the bug

bug-186454-20180608233305.patch (text/plain), 17.33 KB, created by Ryosuke Niwa on 2018-06-08 23:33:06 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Ryosuke Niwa

Created: 2018-06-08 23:33:06 PDT

Size: 17.33 KB

patch

obsolete

>Index: Source/WebCore/ChangeLog
>===================================================================
>--- Source/WebCore/ChangeLog	(revision 232659)
>+++ Source/WebCore/ChangeLog	(working copy)
>@@ -1,3 +1,39 @@
>+2018-06-08  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 NOBODY (OOPS!).
>+
>+        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-08  Wenson Hsieh  <wenson_hsieh@apple.com>
> 
>         [WebKit on watchOS] Upstream watchOS source additions to OpenSource (Part 1)
>Index: Source/WebCore/editing/VisibleUnits.cpp
>===================================================================
>--- Source/WebCore/editing/VisibleUnits.cpp	(revision 232635)
>+++ Source/WebCore/editing/VisibleUnits.cpp	(working copy)
>@@ -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::CachedL
> {
> }
> 
>-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* CachedLogicallyOrderedL
>     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*>& CachedLogicall
>     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::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* logicallyPreviou
> 
>         previousBox = leafBoxes.previousTextOrLineBreakBox(previousRoot, 0);
>         if (previousBox) {
>-            previousBoxInDifferentBlock = true;
>+            previousBoxInDifferentLine = true;
>             return previousBox;
>         }
> 
>@@ -246,8 +246,8 @@ static const InlineBox* logicallyPreviou
> }
> 
> 
>-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
> 
>         nextBox = leafBoxes.nextTextOrLineBreakBox(nextRoot, 0);
>         if (nextBox) {
>-            nextBoxInDifferentBlock = true;
>+            nextBoxInDifferentLine = true;
>             return nextBox;
>         }
> 
>@@ -287,12 +287,16 @@ static const InlineBox* logicallyNextBox
> }
> 
> 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* wordBreakIterator
> }
> 
> 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 visualWordPositio
> 
>         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 visualWordPositio
>         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)
>Index: LayoutTests/ChangeLog
>===================================================================
>--- LayoutTests/ChangeLog	(revision 232634)
>+++ LayoutTests/ChangeLog	(working copy)
>@@ -1,3 +1,17 @@
>+2018-06-08  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 NOBODY (OOPS!).
>+
>+        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  Mark Lam  <mark.lam@apple.com>
> 
>         Enhance run-jsc-stress-tests to allow a test to specify test specific options required for it to run.
>Index: LayoutTests/editing/selection/move-by-word-visually-mac-expected.txt
>===================================================================
>--- LayoutTests/editing/selection/move-by-word-visually-mac-expected.txt	(revision 232634)
>+++ LayoutTests/editing/selection/move-by-word-visually-mac-expected.txt	(working copy)
>@@ -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: LayoutTests/editing/selection/move-by-word-visually-mac.html
>===================================================================
>--- LayoutTests/editing/selection/move-by-word-visually-mac.html	(revision 232634)
>+++ LayoutTests/editing/selection/move-by-word-visually-mac.html	(working copy)
>@@ -73,6 +73,8 @@ where child_node_index is optional defau
> [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>
>+
> 
> <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: LayoutTests/editing/selection/move-by-word-visually-multi-line-expected.txt
>===================================================================
>--- LayoutTests/editing/selection/move-by-word-visually-multi-line-expected.txt	(revision 232634)
>+++ LayoutTests/editing/selection/move-by-word-visually-multi-line-expected.txt	(working copy)
>@@ -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: LayoutTests/editing/selection/move-by-word-visually-multi-line.html
>===================================================================
>--- LayoutTests/editing/selection/move-by-word-visually-multi-line.html	(revision 232634)
>+++ LayoutTests/editing/selection/move-by-word-visually-multi-line.html	(working copy)
>@@ -74,6 +74,8 @@ where child_node_index is optional, defa
> 
> <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>
>+
> 
> <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>
>

Actions: View | Formatted Diff | Diff

Attachments on bug 186454: 342359 | 342361 | 342362 | 342363