Changeset 260678 in webkit

Timestamp:

Apr 24, 2020 4:18:05 PM (4 years ago)

Author:

Wenson Hsieh

Message:

Make some more adjustments to TextManipulationController's paragraph boundary heuristic
​https://bugs.webkit.org/show_bug.cgi?id=210993
<rdar://problem/61571299>

Reviewed by Tim Horton.

Source/WebCore:

Adjust the heuristic added in r260583 to account for a few more common scenarios where we currently consider
text as a part of the same paragraph. This can lead to many issues during text manipulation where text is moved
between these elements, when it should not be.

The new scenarios include block and inline-block links, as well as button elements.

Test: TextManipulation.StartTextManipulationTreatsInlineBlockLinksAndButtonsAsParagraphs

• editing/TextManipulationController.cpp:

(WebCore::TextManipulationController::observeParagraphs):

Additionally rename "paragraph boundary element" to "item boundary element", to avoid colliding with the
existing notion of paragraph boundaries in editing code.

Tools:

Add a new API test with buttons and links styled with display: inline-block;. Additionally, rebaseline an
existing API test that exercises inline-block list items.

• TestWebKitAPI/Tests/WebKitCocoa/TextManipulation.mm:

Location:

trunk

Files:

• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/editing/TextManipulationController.cpp (modified) (3 diffs)
• Tools/ChangeLog (modified) (1 diff)
• Tools/TestWebKitAPI/Tests/WebKitCocoa/TextManipulation.mm (modified) (2 diffs)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2020-04-24  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        Make some more adjustments to TextManipulationController's paragraph boundary heuristic
+        https://bugs.webkit.org/show_bug.cgi?id=210993
+        <rdar://problem/61571299>
+
+        Reviewed by Tim Horton.
+
+        Adjust the heuristic added in r260583 to account for a few more common scenarios where we currently consider
+        text as a part of the same paragraph. This can lead to many issues during text manipulation where text is moved
+        between these elements, when it should not be.
+
+        The new scenarios include block and inline-block links, as well as button elements.
+
+        Test: TextManipulation.StartTextManipulationTreatsInlineBlockLinksAndButtonsAsParagraphs
+
+        * editing/TextManipulationController.cpp:
+        (WebCore::TextManipulationController::observeParagraphs):
+
+        Additionally rename "paragraph boundary element" to "item boundary element", to avoid colliding with the
+        existing notion of paragraph boundaries in editing code.
+
 2020-04-24  Christopher Reid  <chris.reid@sony.com>
 

trunk/Source/WebCore/editing/TextManipulationController.cpp

@@ -275 +275 @@
     Position startOfCurrentParagraph;
     Position endOfCurrentParagraph;
-    RefPtr<Element> enclosingParagraphElement;
-
-    auto isEnclosingParagraphElement = [](const Element& element) {
-        if (element.hasTagName(HTMLNames::liTag)) {
-            auto* renderer = element.renderer();
-            return renderer && renderer->style().display() == DisplayType::Block;
-        }
+    RefPtr<Element> enclosingItemBoundaryElement;
+
+    auto isEnclosingItemBoundaryElement = [](const Element& element) {
+        auto* renderer = element.renderer();
+        if (!renderer)
+            return false;
+
+        if (element.hasTagName(HTMLNames::buttonTag))
+            return true;
+
+        if (element.hasTagName(HTMLNames::liTag) || element.hasTagName(HTMLNames::aTag)) {
+            auto displayType = renderer->style().display();
+            return displayType == DisplayType::Block || displayType == DisplayType::InlineBlock;
+        }
+
         return false;
     };
@@ -288 +296 @@
         auto content = iterator.currentContent();
         if (content.node) {
-            if (enclosingParagraphElement && !enclosingParagraphElement->contains(content.node.get())) {
+            if (enclosingItemBoundaryElement && !enclosingItemBoundaryElement->contains(content.node.get())) {
                 if (!tokensInCurrentParagraph.isEmpty())
                     addItem(ManipulationItemData { startOfCurrentParagraph, endOfCurrentParagraph, nullptr, nullQName(), std::exchange(tokensInCurrentParagraph, { }) });
-                enclosingParagraphElement = nullptr;
-            }
+                enclosingItemBoundaryElement = nullptr;
+            }
+
             if (RefPtr<Element> currentElementAncestor = is<Element>(*content.node) ? downcast<Element>(content.node.get()) : content.node->parentOrShadowHostElement()) {
                 if (isInManipulatedElement(*currentElementAncestor))
@@ -312 +321 @@
                     }
                 }
-                if (isEnclosingParagraphElement(currentElement))
-                    enclosingParagraphElement = &currentElement;
+                if (!enclosingItemBoundaryElement && isEnclosingItemBoundaryElement(currentElement))
+                    enclosingItemBoundaryElement = &currentElement;
             }
         }

trunk/Tools/ChangeLog

@@ +1 @@
+2020-04-24  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        Make some more adjustments to TextManipulationController's paragraph boundary heuristic
+        https://bugs.webkit.org/show_bug.cgi?id=210993
+        <rdar://problem/61571299>
+
+        Reviewed by Tim Horton.
+
+        Add a new API test with buttons and links styled with `display: inline-block;`. Additionally, rebaseline an
+        existing API test that exercises inline-block list items.
+
+        * TestWebKitAPI/Tests/WebKitCocoa/TextManipulation.mm:
+
 2020-04-24  Chris Dumez  <cdumez@apple.com>
 

trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/TextManipulation.mm

@@ -617 +617 @@
 }
 
+TEST(TextManipulation, StartTextManipulationTreatsInlineBlockLinksAndButtonsAsParagraphs)
+{
+    auto delegate = adoptNS([[TextManipulationDelegate alloc] init]);
+    auto webView = adoptNS([[TestWKWebView alloc] initWithFrame:NSMakeRect(0, 0, 400, 400)]);
+    [webView _setTextManipulationDelegate:delegate.get()];
+
+    [webView synchronouslyLoadHTMLString:@"<!DOCTYPE html>"
+        "<head>"
+        "    <style>"
+        "        a {"
+        "            display: inline-block;"
+        "            border: 1px blue solid;"
+        "            margin-left: 1em;"
+        "        }"
+        "    </style>"
+        "</head>"
+        "<body>"
+        "    <button>One</button><button>Two</button>"
+        "    <div><br></div>"
+        "    <a href='#'>Three</a><a href='#'>Four</a>"
+        "</body>"];
+
+    done = false;
+    [webView _startTextManipulationsWithConfiguration:nil completion:^{
+        done = true;
+    }];
+    TestWebKitAPI::Util::run(&done);
+
+    NSArray<_WKTextManipulationItem *> *items = [delegate items];
+    EXPECT_EQ(items.count, 4UL);
+    EXPECT_EQ(items[0].tokens.count, 1UL);
+    EXPECT_EQ(items[1].tokens.count, 1UL);
+    EXPECT_EQ(items[2].tokens.count, 1UL);
+    EXPECT_EQ(items[3].tokens.count, 1UL);
+    EXPECT_WK_STREQ("One", items[0].tokens[0].content);
+    EXPECT_WK_STREQ("Two", items[1].tokens[0].content);
+    EXPECT_WK_STREQ("Three", items[2].tokens[0].content);
+    EXPECT_WK_STREQ("Four", items[3].tokens[0].content);
+}
+
 struct Token {
     NSString *identifier;
@@ -1779 +1819 @@
 
     auto *items = [delegate items];
-    EXPECT_EQ(items.count, 1UL);
-    EXPECT_EQ(items[0].tokens.count, 2UL);
-    EXPECT_STREQ("holle", items[0].tokens[0].content.UTF8String);
-    EXPECT_STREQ("wdrlo", items[0].tokens[1].content.UTF8String);
-
-    done = false;
-    [webView _completeTextManipulationForItems:@[(_WKTextManipulationItem *)createItem(items[0].identifier, {
-        { items[0].tokens[0].identifier, @"hello" },
-        { items[0].tokens[1].identifier, @"world" },
-    })] completion:^(NSArray<NSError *> *errors) {
+    EXPECT_EQ(items.count, 2UL);
+    EXPECT_EQ(items[0].tokens.count, 1UL);
+    EXPECT_EQ(items[1].tokens.count, 1UL);
+    EXPECT_WK_STREQ("holle", items[0].tokens[0].content);
+    EXPECT_WK_STREQ("wdrlo", items[1].tokens[0].content);
+
+    done = false;
+    [webView _completeTextManipulationForItems:@[
+        createItem(items[0].identifier, {{ items[0].tokens[0].identifier, @"hello" }}).autorelease(),
+        createItem(items[1].identifier, {{ items[1].tokens[0].identifier, @"world" }}).autorelease()
+    ] completion:^(NSArray<NSError *> *errors) {
         EXPECT_EQ(errors, nil);
         done = true;