Bug 205203 – TextManipulationController should observe newly inserted or displayed contents

• Component: HTML Editing
• Status: RESOLVED FIXED

[Ryosuke Niwa]

When new content is inserted by DOM API or the exiting content gets newly rendered,
TextManipulationController should inform of its clients.

[Ryosuke Niwa]

Created attachment 385585 [details]
WIP

[Ryosuke Niwa]

<rdar://problem/56567020>

[Ryosuke Niwa]

I need to do this in more than one piece.

[Ryosuke Niwa]

Created attachment 385669 [details]
Adds the feature

[Wenson Hsieh]

Comment on attachment 385669 [details]
Adds the feature

View in context: https://bugs.webkit.org/attachment.cgi?id=385669&action=review

> Source/WebCore/ChangeLog:14
> +        a newly inserted content, we scheudle a new event loop task to do this work.

Nit - scheudle

> Source/WebCore/editing/TextManipulationController.cpp:131
> +    if (document != start.deepEquivalent().document() || document != end.deepEquivalent().document())

Is it worth keeping the null check for the case where start and end positions are null?

> Tools/ChangeLog:9
> +        Added tests for detectign newly inserted or displayed contents in WKTextManipulation SPI.

Nit - detectign

[Louie Livon-Bemel]

Comment on attachment 385669 [details]
Adds the feature

>Subversion Revision: 253360
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 7c2d81ccf1bf23a204a5c07ace81710a33b39ef3..947c75e2eb18ef84b46972d7a1927bd9de0ccadf 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,41 @@
>[phone]  Ryosuke Niwa  <[e-mail]>
>+
>+        TextManipulationController should observe newly inserted or displayed contents
>+        https://bugs.webkit.org/show_bug.cgi?id=205203
>+        <rdar://problem/56567020>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        This patch makes TextManipulationController detect newly inserted or displayed contents and invoke
>+        the callbacks with the newly found items.
>+
>+        To do this, we add a new WeakHashSet to TextManipulationController to which an element is added
>+        whenever its renderer is created. Because it's expensive (and not safe) to find paragraphs around
>+        a newly inserted content, we scheudle a new event loop task to do this work.
Typo: scheudle -> schedule
>+
>+        To find newly inserted paragraphs, we first expand the element's boundary to its start and end of
>+        paragraphs. Because each element in this paragraph could have been added in the weak hash set, we
>+        use hash map to de-duplicate start and end positions. We also filter out any element whose parent
>+        is also in the weak hash set since they would simply find inner paragraphs.
>+
>+        Tests: TextManipulation.StartTextManipulationFindNewlyInsertedParagraph
>+               TextManipulation.StartTextManipulationFindNewlyDisplayedParagraph
>+               TextManipulation.StartTextManipulationFindSameParagraphWithNewContent
>+
>+        * dom/TaskSource.h:
>+        (WebCore::TaskSource::InternalAsyncTask): Added.
>+        * editing/TextManipulationController.cpp:
>+        (WebCore::TextManipulationController::startObservingParagraphs):
>+        (WebCore::TextManipulationController::observeParagraphs): Extracted out of startObservingParagraphs.
>+        (WebCore::TextManipulationController::didCreateRendererForElement): Added. Gets called whenever
>+        a new RenderElement is created.
>+        (WebCore::makePositionTuple): Added.
>+        (WebCore::makeHashablePositionRange): Added.
>+        (WebCore::TextManipulationController::scheduleObservartionUpdate): Added.
>+        * editing/TextManipulationController.h:
>+        * rendering/updating/RenderTreeUpdater.cpp:
>+        (WebCore::RenderTreeUpdater::createRenderer):
>+
> 2019-12-10  Fujii Hironori  <[e-mail]>
> 
>         [Win] Fix MSVC warning C4701: potentially uninitialized local variable 'x' used
>diff --git a/Source/WebCore/dom/TaskSource.h b/Source/WebCore/dom/TaskSource.h
>index 143885c5b8844e2e8060ed7779508a4e61c1b752..c974d94db870e8c2f0c77d1142dd1e9973ae8619 100644
>--- a/Source/WebCore/dom/TaskSource.h
>+++ b/Source/WebCore/dom/TaskSource.h
>@@ -36,7 +36,10 @@ enum class TaskSource : uint8_t {
>     Microtask,
>     Networking,
>     PostedMessageQueue,
>-    UserInteraction
>+    UserInteraction,
>+
>+    // Internal to WebCore
>+    InternalAsyncTask, // Safe to re-order or delay.
> };
> 
> } // namespace WebCore
>diff --git a/Source/WebCore/editing/TextManipulationController.cpp b/Source/WebCore/editing/TextManipulationController.cpp
>index d2e39f7bdf3e5a6bf8b0090cec49a0c0f018143e..e17bdec145b0875bc57666b25b6e4fc2771bfd09 100644
>--- a/Source/WebCore/editing/TextManipulationController.cpp
>+++ b/Source/WebCore/editing/TextManipulationController.cpp
>@@ -29,6 +29,7 @@
> #include "CharacterData.h"
> #include "Editing.h"
> #include "ElementAncestorIterator.h"
>+#include "EventLoop.h"
> #include "ScriptDisallowedScope.h"
> #include "TextIterator.h"
> #include "VisibleUnits.h"
>@@ -118,9 +119,17 @@ void TextManipulationController::startObservingParagraphs(ManipulationItemCallba
> 
>     VisiblePosition start = firstPositionInNode(m_document.get());
>     VisiblePosition end = lastPositionInNode(m_document.get());
>+
>+    observeParagraphs(start, end);
>+}
>+
>+void TextManipulationController::observeParagraphs(VisiblePosition& start, VisiblePosition& end)
>+{
>+    auto document = makeRefPtr(start.deepEquivalent().document());
>+    ASSERT(document);
>     TextIterator iterator { start.deepEquivalent(), end.deepEquivalent() };
>-    if (!document)
>-        return; // VisiblePosition or TextIterator's constructor may have updated the layout and executed arbitrary scripts.
>+    if (document != start.deepEquivalent().document() || document != end.deepEquivalent().document())
>+        return; // TextIterator's constructor may have updated the layout and executed arbitrary scripts.
> 
>     ExclusionRuleMatcher exclusionRuleMatcher(m_exclusionRules);
>     Vector<ManipulationToken> tokensInCurrentParagraph;
>@@ -163,6 +172,65 @@ void TextManipulationController::startObservingParagraphs(ManipulationItemCallba
>         addItem(startOfCurrentParagraph, end.deepEquivalent(), WTFMove(tokensInCurrentParagraph));
> }
> 
>+void TextManipulationController::didCreateRendererForElement(Element& element)
>+{
>+    if (m_mutatedElements.computesEmpty())
>+        scheduleObservartionUpdate();
>+    m_mutatedElements.add(element);
>+}
>+
>+using PositionTuple = std::tuple<RefPtr<Node>, unsigned, unsigned>;
>+static const PositionTuple makePositionTuple(const Position& position)
>+{
>+    return { position.anchorNode(), static_cast<unsigned>(position.anchorType()), position.anchorType() == Position::PositionIsOffsetInAnchor ? position.offsetInContainerNode() : 0 }; 
>+}
>+
>+static const std::pair<PositionTuple, PositionTuple> makeHashablePositionRange(const VisiblePosition& start, const VisiblePosition& end)
>+{
>+    return { makePositionTuple(start.deepEquivalent()), makePositionTuple(end.deepEquivalent()) };
>+}
>+
>+void TextManipulationController::scheduleObservartionUpdate()
>+{
>+    if (!m_document)
>+        return;
>+
>+    m_document->eventLoop().queueTask(TaskSource::InternalAsyncTask, [weakThis = makeWeakPtr(*this)] {
>+        auto* controller = weakThis.get();
>+        if (!controller)
>+            return;
>+
>+        HashSet<Ref<Element>> mutatedElements;
>+        for (auto& weakElement : controller->m_mutatedElements)
>+            mutatedElements.add(weakElement);
>+        controller->m_mutatedElements.clear();
>+
>+        HashSet<Ref<Element>> filteredElements;
>+        for (auto& element : mutatedElements) {
>+            auto* parentElement = element->parentElement();
>+            if (!parentElement || !mutatedElements.contains(parentElement))
>+                filteredElements.add(element.copyRef());
>+        }
>+        mutatedElements.clear();
>+
>+        HashSet<std::pair<PositionTuple, PositionTuple>> paragraphSets;
>+        for (auto& element : filteredElements) {
>+            auto start = startOfParagraph(firstPositionInOrBeforeNode(element.ptr()));
>+            auto end = endOfParagraph(lastPositionInOrAfterNode(element.ptr()));
>+
>+            auto key = makeHashablePositionRange(start, end);
>+            if (!paragraphSets.add(key).isNewEntry)
>+                continue;
>+
>+            auto* controller = weakThis.get();
>+            if (!controller)
>+                return; // Finding the start/end of paragraph may have updated layout & executed arbitrary scripts.
>+
>+            controller->observeParagraphs(start, end);
>+        }
>+    });
>+}
>+
> void TextManipulationController::addItem(const Position& startOfParagraph, const Position& endOfParagraph, Vector<ManipulationToken>&& tokens)
> {
>     ASSERT(m_document);
>diff --git a/Source/WebCore/editing/TextManipulationController.h b/Source/WebCore/editing/TextManipulationController.h
>index 372cd64ea4fdc6aea60ac78007a392c4e6dbdf85..78a187e7c3d3b0693df2616bd7617beb2bd38550 100644
>--- a/Source/WebCore/editing/TextManipulationController.h
>+++ b/Source/WebCore/editing/TextManipulationController.h
>@@ -30,11 +30,14 @@
> #include <wtf/EnumTraits.h>
> #include <wtf/ObjectIdentifier.h>
> #include <wtf/Optional.h>
>+#include <wtf/WeakHashSet.h>
> #include <wtf/WeakPtr.h>
> 
> namespace WebCore {
> 
> class Document;
>+class Element;
>+class VisiblePosition;
> 
> class TextManipulationController : public CanMakeWeakPtr<TextManipulationController> {
>     WTF_MAKE_FAST_ALLOCATED;
>@@ -93,6 +96,8 @@ public:
>     using ManipulationItemCallback = WTF::Function<void(Document&, ItemIdentifier, const Vector<ManipulationToken>&)>;
>     WEBCORE_EXPORT void startObservingParagraphs(ManipulationItemCallback&&, Vector<ExclusionRule>&& = { });
> 
>+    void didCreateRendererForElement(Element&);
>+
>     enum class ManipulationResult : uint8_t {
>         Success,
>         ContentChanged,
>@@ -103,6 +108,9 @@ public:
>     WEBCORE_EXPORT ManipulationResult completeManipulation(ItemIdentifier, const Vector<ManipulationToken>&);
> 
> private:
>+    void observeParagraphs(VisiblePosition& start, VisiblePosition& end);
>+    void scheduleObservartionUpdate();
>+
>     struct ManipulationItem {
>         Position start;
>         Position end;
>@@ -113,6 +121,7 @@ private:
>     ManipulationResult replace(const ManipulationItem&, const Vector<ManipulationToken>&);
> 
>     WeakPtr<Document> m_document;
>+    WeakHashSet<Element> m_mutatedElements;
>     ManipulationItemCallback m_callback;
>     Vector<ExclusionRule> m_exclusionRules;
>     HashMap<ItemIdentifier, ManipulationItem> m_items;
>diff --git a/Source/WebCore/rendering/updating/RenderTreeUpdater.cpp b/Source/WebCore/rendering/updating/RenderTreeUpdater.cpp
>index 3656f3bfaaff3549b6830040a8e2b86c2cafc009..231bb08736441b7f208f7373d9200d200a3a4a33 100644
>--- a/Source/WebCore/rendering/updating/RenderTreeUpdater.cpp
>+++ b/Source/WebCore/rendering/updating/RenderTreeUpdater.cpp
>@@ -49,6 +49,7 @@
> #include "RuntimeEnabledFeatures.h"
> #include "StyleResolver.h"
> #include "StyleTreeResolver.h"
>+#include "TextManipulationController.h"
> #include <wtf/SystemTracing.h>
> 
> #if ENABLE(LAYOUT_FORMATTING_CONTEXT)
>@@ -397,6 +398,10 @@ void RenderTreeUpdater::createRenderer(Element& element, RenderStyle&& style)
> 
>     m_builder.attach(insertionPosition, WTFMove(newRenderer));
> 
>+    auto* textManipulationController = m_document.textManipulationControllerIfExists();
>+    if (UNLIKELY(textManipulationController))
>+        textManipulationController->didCreateRendererForElement(element);
>+
>     if (AXObjectCache* cache = m_document.axObjectCache())
>         cache->updateCacheAfterNodeIsAttached(&element);
> }
>diff --git a/Tools/ChangeLog b/Tools/ChangeLog
>index 00dabe73f48c3cee3417d3a8675f40f9838481d2..786e8d14261c05f0fffa043230ee293dd10fdba6 100644
>--- a/Tools/ChangeLog
>+++ b/Tools/ChangeLog
>@@ -1,3 +1,18 @@
>[phone]  Ryosuke Niwa  <[e-mail]>
>+
>+        TextManipulationController should observe newly inserted or displayed contents
>+        https://bugs.webkit.org/show_bug.cgi?id=205203
>+        <rdar://problem/56567020>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Added tests for detectign newly inserted or displayed contents in WKTextManipulation SPI.
Typo: detectign -> detecting
>+
>+        * TestWebKitAPI/Tests/WebKitCocoa/TextManipulation.mm:
>+        (-[TextManipulationDelegate initWithItemCallback]):
>+        (-[TextManipulationDelegate _webView:didFindTextManipulationItem:]):
>+        (TestWebKitAPI::TEST):
>+
> 2019-12-10  Per Arne Vollan  <[e-mail]>
> 
>         Fix API test failure after r253351
>diff --git a/Tools/TestWebKitAPI/Tests/WebKitCocoa/TextManipulation.mm b/Tools/TestWebKitAPI/Tests/WebKitCocoa/TextManipulation.mm
>index d707598fdc7e8822bcf314f98a1d39f995504c75..94f9cdddc776da1f4edfe2cef41a238ed228e59b 100644
>--- a/Tools/TestWebKitAPI/Tests/WebKitCocoa/TextManipulation.mm
>+++ b/Tools/TestWebKitAPI/Tests/WebKitCocoa/TextManipulation.mm
>@@ -39,11 +39,14 @@
> 
> static bool done = false;
> 
>+typedef void(^ItemCallback)(_WKTextManipulationItem *);
>+
> @interface TextManipulationDelegate : NSObject <_WKTextManipulationDelegate>
> 
> - (void)_webView:(WKWebView *)webView didFindTextManipulationItem:(_WKTextManipulationItem *)item;
> 
> @property (nonatomic, readonly, copy) NSArray<_WKTextManipulationItem *> *items;
>+@property (strong) ItemCallback itemCallback;
> 
> @end
> 
>@@ -59,9 +62,19 @@ static bool done = false;
>     return self;
> }
> 
>+- (instancetype)initWithItemCallback
>+{
>+    if (!(self = [super init]))
>+        return nil;
>+    _items = adoptNS([[NSMutableArray alloc] init]);
>+    return self;
>+}
>+
> - (void)_webView:(WKWebView *)webView didFindTextManipulationItem:(_WKTextManipulationItem *)item
> {
>     [_items addObject:item];
>+    if (self.itemCallback)
>+        self.itemCallback(item);
> }
> 
> - (NSArray<_WKTextManipulationItem *> *)items
>@@ -170,6 +183,125 @@ TEST(TextManipulation, StartTextManipulationFindParagraphsWithMultileTokens)
>     EXPECT_STREQ("Kit", items[1].tokens[1].content.UTF8String);
> }
> 
>+TEST(TextManipulation, StartTextManipulationFindNewlyInsertedParagraph)
>+{
>+    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><html><body><p>hello</p></body></html>"];
>+
>+    done = false;
>+    [webView _startTextManipulationsWithConfiguration:nil completion:^{
>+        done = true;
>+    }];
>+    TestWebKitAPI::Util::run(&done);
>+
>+    __block auto *items = [delegate items];
>+    EXPECT_EQ(items.count, 1UL);
>+    EXPECT_EQ(items[0].tokens.count, 1UL);
>+    EXPECT_STREQ("hello", items[0].tokens[0].content.UTF8String);
>+
>+    done = false;
>+    delegate.get().itemCallback = ^(_WKTextManipulationItem *item) {
>+        if (items.count == 3)
>+            done = true;
>+    };
>+    [webView stringByEvaluatingJavaScript:@"document.body.appendChild(document.createElement('div')).innerHTML = 'world<br><b>Web</b>Kit';"];
>+    TestWebKitAPI::Util::run(&done);
>+
>+    EXPECT_EQ(items.count, 3UL);
>+    EXPECT_EQ(items[1].tokens.count, 1UL);
>+    EXPECT_STREQ("world", items[1].tokens[0].content.UTF8String);
>+    EXPECT_EQ(items[2].tokens.count, 2UL);
>+    EXPECT_STREQ("Web", items[2].tokens[0].content.UTF8String);
>+    EXPECT_STREQ("Kit", items[2].tokens[1].content.UTF8String);
>+}
>+
>+TEST(TextManipulation, StartTextManipulationFindNewlyDisplayedParagraph)
>+{
>+    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><html><body>"
>+        "<style> .hidden { display: none; } </style>"
>+        "<p>hello</p><div><span class='hidden'>Web</span><span class='hidden'>Kit</span></div><div class='hidden'>hey</div></body></html>"];
>+
>+    done = false;
>+    [webView _startTextManipulationsWithConfiguration:nil completion:^{
>+        done = true;
>+    }];
>+    TestWebKitAPI::Util::run(&done);
>+
>+    __block auto *items = [delegate items];
>+    EXPECT_EQ(items.count, 1UL);
>+    EXPECT_EQ(items[0].tokens.count, 1UL);
>+    EXPECT_STREQ("hello", items[0].tokens[0].content.UTF8String);
>+
>+    done = false;
>+    delegate.get().itemCallback = ^(_WKTextManipulationItem *item) {
>+        if (items.count == 2)
>+            done = true;
>+    };
>+    [webView stringByEvaluatingJavaScript:@"document.querySelectorAll('span.hidden').forEach((span) => span.classList.remove('hidden'));"];
>+    TestWebKitAPI::Util::run(&done);
>+
>+    EXPECT_EQ(items.count, 2UL);
>+    EXPECT_EQ(items[1].tokens.count, 2UL);
>+    EXPECT_STREQ("Web", items[1].tokens[0].content.UTF8String);
>+    EXPECT_STREQ("Kit", items[1].tokens[1].content.UTF8String);
>+
>+    // This has to happen separately in order to have a deterministic ordering.
>+    done = false;
>+    delegate.get().itemCallback = ^(_WKTextManipulationItem *item) {
>+        if (items.count == 3)
>+            done = true;
>+    };
>+    [webView stringByEvaluatingJavaScript:@"document.querySelectorAll('div.hidden').forEach((div) => div.classList.remove('hidden'));"];
>+    TestWebKitAPI::Util::run(&done);
>+
>+    EXPECT_EQ(items.count, 3UL);
>+    EXPECT_EQ(items[2].tokens.count, 1UL);
>+    EXPECT_STREQ("hey", items[2].tokens[0].content.UTF8String);
>+}
>+
>+TEST(TextManipulation, StartTextManipulationFindSameParagraphWithNewContent)
>+{
>+    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><html><body><p>hello</p></body></html>"];
>+
>+    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, 1UL);
>+    EXPECT_STREQ("hello", items[0].tokens[0].content.UTF8String);
>+
>+    done = false;
>+    delegate.get().itemCallback = ^(_WKTextManipulationItem *item) {
>+        done = true;
>+    };
>+
>+    [webView stringByEvaluatingJavaScript:@"b = document.createElement('b');"
>+        "b.textContent = ' world';"
>+        "document.querySelector('p').appendChild(b); ''"];
>+
>+    TestWebKitAPI::Util::run(&done);
>+
>+    EXPECT_EQ(items.count, 2UL);
>+    EXPECT_EQ(items[1].tokens.count, 2UL);
>+    EXPECT_STREQ("hello", items[1].tokens[0].content.UTF8String);
>+    EXPECT_STREQ(" world", items[1].tokens[1].content.UTF8String);
>+}
>+
> TEST(TextManipulation, StartTextManipulationApplySingleExcluionRuleForElement)
> {
>     auto delegate = adoptNS([[TextManipulationDelegate alloc] init]);
>@@ -488,8 +620,6 @@ TEST(TextManipulation, CompleteTextManipulationFailWhenExclusionIsViolated)
>     EXPECT_WK_STREQ("<p>hi, <em>WebKitten</em></p>", [webView stringByEvaluatingJavaScript:@"document.body.innerHTML"]);
> }
> 
>-// MARK: - _WKTextManipulationToken Tests
>-
> TEST(TextManipulation, TextManipulationTokenDebugDescription)
> {
>     auto token = adoptNS([[_WKTextManipulationToken alloc] init]);
>@@ -772,8 +902,6 @@ TEST(TextManipulation, TextManipulationTokenNSObjectEqualityWithNonToken)
>     EXPECT_FALSE([token isEqual:nil]);
> }
> 
>-// MARK: - _WKTextManipulationItem Tests
>-
> static RetainPtr<_WKTextManipulationToken> createTextManipulationToken(NSString *identifier, BOOL excluded, NSString *content)
> {
>     auto token = adoptNS([[_WKTextManipulationToken alloc] init]);

[Ryosuke Niwa]

Fixed typos. Thanks for the review!

[Ryosuke Niwa]

Committed r253583: <https://trac.webkit.org/changeset/253583>

[Radar WebKit Bug Importer]

<rdar://problem/57985619>