[Webpage Translation] Avoid removing elements with no children during…

… text manipulation

https://bugs.webkit.org/show_bug.cgi?id=240287
rdar://91882797

Reviewed by Tim Horton.

After invoking webpage translation on a particular website, the entire page becomes blank and unusable when
scrolling. This happens because of an uncaught JavaScript exception that's thrown when this page's script
attempts to remove an element from its parent using `p.removeChild(c)`, where the node `p` is not a parent of
the given node `c`; this, in turn, happens because `TextManipulationController` has already unparented `c` from
`p` while performing text replacement during translation.

In this particular case, the former child node `c` is an empty `div` element with no text or children. As such,
it's unnecessary to flag this element for removal in the first place, since doing so isn't necessary to fill in
translated text.

We can avoid this issue by simply skipping over such nodes (i.e. containers that contain no text, no child
elements, and also are not replaced elements) to avoid this and similar compatibility issues that arise when
the DOM is mutated underneath the page, during translation.

Test: TextManipulation.CompleteTextManipulationSkipsEmptyContainers

* editing/TextManipulationController.cpp:
(WebCore::TextManipulationController::replace):
[Webpage Translation] Avoid removing elements with no children during text manipulation
https://bugs.webkit.org/show_bug.cgi?id=240287
rdar://91882797

Reviewed by Tim Horton.

Add a new API test to exercise the change.

* TestWebKitAPI/Tests/WebKitCocoa/TextManipulation.mm:
(TestWebKitAPI::TEST):

Canonical link: https://commits.webkit.org/250467@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@294063 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog

@@ -1,3 +1,30 @@
+2022-05-10  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        [Webpage Translation] Avoid removing elements with no children during text manipulation
+        https://bugs.webkit.org/show_bug.cgi?id=240287
+        rdar://91882797
+
+        Reviewed by Tim Horton.
+
+        After invoking webpage translation on a particular website, the entire page becomes blank and unusable when
+        scrolling. This happens because of an uncaught JavaScript exception that's thrown when this page's script
+        attempts to remove an element from its parent using `p.removeChild(c)`, where the node `p` is not a parent of
+        the given node `c`; this, in turn, happens because `TextManipulationController` has already unparented `c` from
+        `p` while performing text replacement during translation.
+
+        In this particular case, the former child node `c` is an empty `div` element with no text or children. As such,
+        it's unnecessary to flag this element for removal in the first place, since doing so isn't necessary to fill in
+        translated text.
+
+        We can avoid this issue by simply skipping over such nodes (i.e. containers that contain no text, no child
+        elements, and also are not replaced elements) to avoid this and similar compatibility issues that arise when
+        the DOM is mutated underneath the page, during translation.
+
+        Test: TextManipulation.CompleteTextManipulationSkipsEmptyContainers
+
+        * editing/TextManipulationController.cpp:
+        (WebCore::TextManipulationController::replace):
+
 2022-05-11  Youenn Fablet  <youenn@apple.com>
 
         Add a webshare quirk for youtube

Source/WebCore/editing/TextManipulationController.cpp

@@ -791,10 +791,14 @@ auto TextManipulationController::replace(const ManipulationItemData& item, const
         auto content = iterator.currentContent();
         ASSERT(content.node);
 
+        bool isReplacedOrTextContent = content.isReplacedContent || content.isTextContent;
+        if (!isReplacedOrTextContent && is<ContainerNode>(*content.node) && !content.node->hasChildNodes() && content.text.isEmpty())
+            continue;
+
         lastChildOfCommonAncestorInRange = content.node;
         nodesToRemove.add(*content.node);
 
-        if (!content.isReplacedContent && !content.isTextContent)
+        if (!isReplacedOrTextContent)
             continue;
 
         Vector<ManipulationToken> tokensInCurrentNode;

Tools/ChangeLog

@@ -1,3 +1,16 @@
+2022-05-10  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        [Webpage Translation] Avoid removing elements with no children during text manipulation
+        https://bugs.webkit.org/show_bug.cgi?id=240287
+        rdar://91882797
+
+        Reviewed by Tim Horton.
+
+        Add a new API test to exercise the change.
+
+        * TestWebKitAPI/Tests/WebKitCocoa/TextManipulation.mm:
+        (TestWebKitAPI::TEST):
+
 2022-05-10  Jonathan Bedard  <jbedard@apple.com>
 
         [git-webkit] Fail quickly for invalid bugzilla credentials

Tools/TestWebKitAPI/Tests/WebKitCocoa/TextManipulation.mm

@@ -3414,6 +3414,46 @@ - (void)_webView:(WKWebView *)webView didFindTextManipulationItem:(_WKTextManipu
     EXPECT_WK_STREQ("<span class=\"hidden\">hello</span>", [webView stringByEvaluatingJavaScript:@"document.body.innerHTML"]);
 }
 
+TEST(TextManipulation, CompleteTextManipulationSkipsEmptyContainers)
+{
+    auto delegate = adoptNS([[TextManipulationDelegate alloc] init]);
+    auto webView = adoptNS([[TestWKWebView alloc] initWithFrame:NSMakeRect(0, 0, 400, 400)]);
+    [webView _setTextManipulationDelegate:delegate.get()];
+    [webView synchronouslyLoadHTMLString:@"<section><a href='#'>Guten<img width='50' src='icon.png'></section>"
+        "<section><div id='target'></div><div><a href='#'>Tag</a></div></section>"];
+
+    done = false;
+    [webView _startTextManipulationsWithConfiguration:nil completion:^{
+        done = true;
+    }];
+    TestWebKitAPI::Util::run(&done);
+
+    auto items = [delegate items];
+    EXPECT_EQ(items.count, 1UL);
+    EXPECT_EQ(items[0].tokens.count, 3UL);
+    EXPECT_WK_STREQ("Guten", items[0].tokens[0].content);
+    EXPECT_WK_STREQ("[]", items[0].tokens[1].content);
+    EXPECT_WK_STREQ("Tag", items[0].tokens[2].content);
+
+    auto replacement = createItem(items[0].identifier, {
+        { items[0].tokens[0].identifier, @"Good" },
+        { items[0].tokens[2].identifier, @"Day" }
+    });
+
+    done = false;
+    [webView _completeTextManipulationForItems:@[replacement.get()] completion:^(NSArray<NSError *> *errors) {
+        EXPECT_EQ(errors.count, 0UL);
+        done = true;
+    }];
+    TestWebKitAPI::Util::run(&done);
+
+    EXPECT_WK_STREQ("SECTION", [webView stringByEvaluatingJavaScript:@"document.getElementById('target').parentElement.tagName"]);
+    NSArray<NSString *> *textContents = [webView objectByEvaluatingJavaScript:@"Array.from(document.links).map(a => a.textContent)"];
+    EXPECT_EQ(textContents.count, 2U);
+    EXPECT_WK_STREQ("Good", textContents.firstObject);
+    EXPECT_WK_STREQ("Day", textContents.lastObject);
+}
+
 TEST(TextManipulation, TextManipulationTokenDebugDescription)
 {
     auto token = adoptNS([[_WKTextManipulationToken alloc] init]);