Title: [254314] trunk
- Revision
- 254314
- Author
- wenson_hs...@apple.com
- Date
- 2020-01-09 16:40:14 -0800 (Thu, 09 Jan 2020)
Log Message
Text manipulation controller should not observe changes in new replacement elements
https://bugs.webkit.org/show_bug.cgi?id=206015
<rdar://problem/58353667>
Reviewed by Tim Horton.
Source/WebCore:
TextManipulationController may insert elements in the process of completing text replacement operations. When
renderers are created for these elements (after the next layout pass), the controller is notified via
TextManipulationController::didCreateRendererForElement, which causes it to begin observing the newly inserted
elements. For certain clients, this may lead to an unending cycle of text manipulation updates as the new
text manipulation items' tokens will be replaced with new tokens, and we never reach a stable state.
To mitigate this, we avoid adding newly visible elements to m_mutatedElements in the case where the newly
visible elements were recently inserted by text replacement. See below for more details.
Test: TextManipulation.CompleteTextManipulationDoesNotCreateMoreTextManipulationItems
* editing/TextManipulationController.cpp:
(WebCore::TextManipulationController::didCreateRendererForElement):
Avoid considering an element that has a new renderer, if it is an element we had just inserted using text
manipulation APIs.
(WebCore::TextManipulationController::replace):
As we iterate over and apply each text replacement, remember the elements we've inserted using a WeakHashSet;
this set is cleared in a queued async task, after which layout should be up to date.
* editing/TextManipulationController.h:
Tools:
* TestWebKitAPI/Tests/WebKitCocoa/TextManipulation.mm:
Add a new API test to verify that we don't fire text manipulation item callbacks due to inserting elements when
completing text manipulation.
(-[TextManipulationDelegate initWithItemCallback]): Deleted.
Remove a stray initializer that was unused (and also doesn't take an ItemCallback, as its name might suggest).
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (254313 => 254314)
--- trunk/Source/WebCore/ChangeLog 2020-01-10 00:19:26 UTC (rev 254313)
+++ trunk/Source/WebCore/ChangeLog 2020-01-10 00:40:14 UTC (rev 254314)
@@ -1,3 +1,35 @@
+2020-01-09 Wenson Hsieh <wenson_hs...@apple.com>
+
+ Text manipulation controller should not observe changes in new replacement elements
+ https://bugs.webkit.org/show_bug.cgi?id=206015
+ <rdar://problem/58353667>
+
+ Reviewed by Tim Horton.
+
+ TextManipulationController may insert elements in the process of completing text replacement operations. When
+ renderers are created for these elements (after the next layout pass), the controller is notified via
+ TextManipulationController::didCreateRendererForElement, which causes it to begin observing the newly inserted
+ elements. For certain clients, this may lead to an unending cycle of text manipulation updates as the new
+ text manipulation items' tokens will be replaced with new tokens, and we never reach a stable state.
+
+ To mitigate this, we avoid adding newly visible elements to m_mutatedElements in the case where the newly
+ visible elements were recently inserted by text replacement. See below for more details.
+
+ Test: TextManipulation.CompleteTextManipulationDoesNotCreateMoreTextManipulationItems
+
+ * editing/TextManipulationController.cpp:
+ (WebCore::TextManipulationController::didCreateRendererForElement):
+
+ Avoid considering an element that has a new renderer, if it is an element we had just inserted using text
+ manipulation APIs.
+
+ (WebCore::TextManipulationController::replace):
+
+ As we iterate over and apply each text replacement, remember the elements we've inserted using a WeakHashSet;
+ this set is cleared in a queued async task, after which layout should be up to date.
+
+ * editing/TextManipulationController.h:
+
2020-01-09 Brent Fulgham <bfulg...@apple.com>
REGRESSION (r253662): Large Data URLs are not being handled properly
Modified: trunk/Source/WebCore/editing/TextManipulationController.cpp (254313 => 254314)
--- trunk/Source/WebCore/editing/TextManipulationController.cpp 2020-01-10 00:19:26 UTC (rev 254313)
+++ trunk/Source/WebCore/editing/TextManipulationController.cpp 2020-01-10 00:40:14 UTC (rev 254314)
@@ -178,6 +178,9 @@
void TextManipulationController::didCreateRendererForElement(Element& element)
{
+ if (m_recentlyInsertedElements.contains(element))
+ return;
+
if (m_mutatedElements.computesEmpty())
scheduleObservartionUpdate();
@@ -398,7 +401,13 @@
insertionPoint.containerNode()->insertBefore(insertion.child, insertionPoint.computeNodeBeforePosition());
else
insertion.parentIfDifferentFromCommonAncestor->appendChild(insertion.child);
+ if (is<Element>(insertion.child.get()))
+ m_recentlyInsertedElements.add(downcast<Element>(insertion.child.get()));
}
+ m_document->eventLoop().queueTask(TaskSource::InternalAsyncTask, [weakThis = makeWeakPtr(*this)] {
+ if (auto strongThis = weakThis.get())
+ strongThis->m_recentlyInsertedElements.clear();
+ });
return ManipulationResult::Success;
}
Modified: trunk/Source/WebCore/editing/TextManipulationController.h (254313 => 254314)
--- trunk/Source/WebCore/editing/TextManipulationController.h 2020-01-10 00:19:26 UTC (rev 254313)
+++ trunk/Source/WebCore/editing/TextManipulationController.h 2020-01-10 00:40:14 UTC (rev 254314)
@@ -122,6 +122,7 @@
WeakPtr<Document> m_document;
WeakHashSet<Element> m_mutatedElements;
+ WeakHashSet<Element> m_recentlyInsertedElements;
ManipulationItemCallback m_callback;
Vector<ExclusionRule> m_exclusionRules;
HashMap<ItemIdentifier, ManipulationItem> m_items;
Modified: trunk/Tools/ChangeLog (254313 => 254314)
--- trunk/Tools/ChangeLog 2020-01-10 00:19:26 UTC (rev 254313)
+++ trunk/Tools/ChangeLog 2020-01-10 00:40:14 UTC (rev 254314)
@@ -1,3 +1,20 @@
+2020-01-09 Wenson Hsieh <wenson_hs...@apple.com>
+
+ Text manipulation controller should not observe changes in new replacement elements
+ https://bugs.webkit.org/show_bug.cgi?id=206015
+ <rdar://problem/58353667>
+
+ Reviewed by Tim Horton.
+
+ * TestWebKitAPI/Tests/WebKitCocoa/TextManipulation.mm:
+
+ Add a new API test to verify that we don't fire text manipulation item callbacks due to inserting elements when
+ completing text manipulation.
+
+ (-[TextManipulationDelegate initWithItemCallback]): Deleted.
+
+ Remove a stray initializer that was unused (and also doesn't take an ItemCallback, as its name might suggest).
+
2020-01-09 Ryan Haddad <ryanhad...@apple.com>
Move macOS Test262, JSC, and perf queues to Catalina
Modified: trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/TextManipulation.mm (254313 => 254314)
--- trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/TextManipulation.mm 2020-01-10 00:19:26 UTC (rev 254313)
+++ trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/TextManipulation.mm 2020-01-10 00:40:14 UTC (rev 254314)
@@ -62,14 +62,6 @@
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];
@@ -905,6 +897,50 @@
EXPECT_WK_STREQ("<p>Hello, <em>WebKitten</em></p>", [webView stringByEvaluatingJavaScript:@"document.body.innerHTML"]);
}
+TEST(TextManipulation, CompleteTextManipulationDoesNotCreateMoreTextManipulationItems)
+{
+ 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>Foo <strong>bar</strong> baz</p></body></html>"];
+
+ auto configuration = adoptNS([[_WKTextManipulationConfiguration alloc] init]);
+
+ done = false;
+ [webView _startTextManipulationsWithConfiguration:configuration.get() completion:^{
+ done = true;
+ }];
+ TestWebKitAPI::Util::run(&done);
+
+ auto *items = [delegate items];
+ EXPECT_EQ(items.count, 1UL);
+
+ auto* firstItem = items.firstObject;
+ EXPECT_EQ(firstItem.tokens.count, 3UL);
+
+ __block BOOL foundNewItemAfterCompletingTextManipulation = false;
+ [delegate setItemCallback:^(_WKTextManipulationItem *) {
+ foundNewItemAfterCompletingTextManipulation = true;
+ }];
+
+ done = false;
+ [webView _completeTextManipulation:(_WKTextManipulationItem *)createItem(firstItem.identifier, {
+ { firstItem.tokens[0].identifier, @"bar " },
+ { firstItem.tokens[1].identifier, @"garply" },
+ { firstItem.tokens[2].identifier, @" foo" },
+ }).get() completion:^(BOOL success) {
+ EXPECT_TRUE(success);
+ done = true;
+ }];
+
+ TestWebKitAPI::Util::run(&done);
+ [webView waitForNextPresentationUpdate];
+
+ EXPECT_FALSE(foundNewItemAfterCompletingTextManipulation);
+ EXPECT_WK_STREQ("<p>bar <strong>garply</strong> foo</p>", [webView stringByEvaluatingJavaScript:@"document.body.innerHTML"]);
+}
+
TEST(TextManipulation, TextManipulationTokenDebugDescription)
{
auto token = adoptNS([[_WKTextManipulationToken alloc] init]);
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes
