When new content is inserted by DOM API or the exiting content gets newly rendered, TextManipulationController should inform of its clients.
Created attachment 385585 [details] WIP
<rdar://problem/56567020>
I need to do this in more than one piece.
Created attachment 385669 [details] Adds the feature
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
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 @@ >+2019-12-13 Ryosuke Niwa <rniwa@webkit.org> >+ >+ 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 <Hironori.Fujii@sony.com> > > [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 @@ >+2019-12-13 Ryosuke Niwa <rniwa@webkit.org> >+ >+ 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 <pvollan@apple.com> > > 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]);
Fixed typos. Thanks for the review!
Committed r253583: <https://trac.webkit.org/changeset/253583>
<rdar://problem/57985619>