- Revision
- 263563
- Author
- [email protected]
- Date
- 2020-06-26 09:41:15 -0700 (Fri, 26 Jun 2020)
Log Message
Text manipulation should observe adjacent elements with new renderer together
https://bugs.webkit.org/show_bug.cgi?id=213333
Reviewed by Geoffrey Garen.
Source/WebCore:
TextManipulationController only keeps track of manipulated Elements. For other types of Node, like Text,
TextManipulationController does not mark them as manipulated and may manipulate it again. r263132 tried to solve
the problem by marking Element with only manipulated children as manipulated, but it did not iterate all
children of Element. So, if one Element has children in different paragraphs, it may fail to mark the Element
manipulated. See updated test.
To fix this issue completely, TextManipulationController now tracks all manipulated Nodes, so it can skip the
manipulated Nodes during observation.
For elements with new renderer, TextManipulationController observes them one by one. However, adjacent elements
can be in the same paragraph, if there is no delimiter in their text, and should be observed together. To solve
this problem, TextManipulationController now starts observing from the common ancestor of these elements. If a
Node in range is manipulated, split the paragrph there. This makes sure content in paragraph is not manipulated.
Relevant test is updated for this new behavior.
Tests: TextManipulation.StartTextManipulationFindNewlyDisplayedParagraph
TextManipulation.CompleteTextManipulationAvoidExtractingManipulatedTextAfterManipulation
* dom/Node.cpp:
(WebCore::Node::~Node):
* editing/TextManipulationController.cpp:
(WebCore::TextManipulationController::observeParagraphs):
(WebCore::TextManipulationController::didCreateRendererForElement):
(WebCore::TextManipulationController::scheduleObservationUpdate):
(WebCore::TextManipulationController::updateInsertions):
(WebCore::TextManipulationController::replace):
(WebCore::TextManipulationController::removeNode):
(WebCore::makePositionTuple): Deleted.
(WebCore::makeHashablePositionRange): Deleted.
* editing/TextManipulationController.h:
Tools:
* TestWebKitAPI/Tests/WebKitCocoa/TextManipulation.mm:
(TestWebKitAPI::TEST):
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (263562 => 263563)
--- trunk/Source/WebCore/ChangeLog 2020-06-26 16:38:45 UTC (rev 263562)
+++ trunk/Source/WebCore/ChangeLog 2020-06-26 16:41:15 UTC (rev 263563)
@@ -1,3 +1,40 @@
+2020-06-26 Sihui Liu <[email protected]>
+
+ Text manipulation should observe adjacent elements with new renderer together
+ https://bugs.webkit.org/show_bug.cgi?id=213333
+
+ Reviewed by Geoffrey Garen.
+
+ TextManipulationController only keeps track of manipulated Elements. For other types of Node, like Text,
+ TextManipulationController does not mark them as manipulated and may manipulate it again. r263132 tried to solve
+ the problem by marking Element with only manipulated children as manipulated, but it did not iterate all
+ children of Element. So, if one Element has children in different paragraphs, it may fail to mark the Element
+ manipulated. See updated test.
+ To fix this issue completely, TextManipulationController now tracks all manipulated Nodes, so it can skip the
+ manipulated Nodes during observation.
+
+ For elements with new renderer, TextManipulationController observes them one by one. However, adjacent elements
+ can be in the same paragraph, if there is no delimiter in their text, and should be observed together. To solve
+ this problem, TextManipulationController now starts observing from the common ancestor of these elements. If a
+ Node in range is manipulated, split the paragrph there. This makes sure content in paragraph is not manipulated.
+ Relevant test is updated for this new behavior.
+
+ Tests: TextManipulation.StartTextManipulationFindNewlyDisplayedParagraph
+ TextManipulation.CompleteTextManipulationAvoidExtractingManipulatedTextAfterManipulation
+
+ * dom/Node.cpp:
+ (WebCore::Node::~Node):
+ * editing/TextManipulationController.cpp:
+ (WebCore::TextManipulationController::observeParagraphs):
+ (WebCore::TextManipulationController::didCreateRendererForElement):
+ (WebCore::TextManipulationController::scheduleObservationUpdate):
+ (WebCore::TextManipulationController::updateInsertions):
+ (WebCore::TextManipulationController::replace):
+ (WebCore::TextManipulationController::removeNode):
+ (WebCore::makePositionTuple): Deleted.
+ (WebCore::makeHashablePositionRange): Deleted.
+ * editing/TextManipulationController.h:
+
2020-06-26 Simon Fraser <[email protected]>
Clean up some PaintBehavior-related code in RenderLayer
Modified: trunk/Source/WebCore/dom/Node.cpp (263562 => 263563)
--- trunk/Source/WebCore/dom/Node.cpp 2020-06-26 16:38:45 UTC (rev 263562)
+++ trunk/Source/WebCore/dom/Node.cpp 2020-06-26 16:41:15 UTC (rev 263563)
@@ -73,6 +73,7 @@
#include "StyleSheetContents.h"
#include "TemplateContentDocumentFragment.h"
#include "TextEvent.h"
+#include "TextManipulationController.h"
#include "TouchEvent.h"
#include "WheelEvent.h"
#include "XMLNSNames.h"
@@ -360,6 +361,10 @@
if (hasRareData())
clearRareData();
+ auto* textManipulationController = document().textManipulationControllerIfExists();
+ if (UNLIKELY(textManipulationController))
+ textManipulationController->removeNode(this);
+
if (!isContainerNode())
willBeDeletedFrom(document());
Modified: trunk/Source/WebCore/editing/TextManipulationController.cpp (263562 => 263563)
--- trunk/Source/WebCore/editing/TextManipulationController.cpp 2020-06-26 16:38:45 UTC (rev 263562)
+++ trunk/Source/WebCore/editing/TextManipulationController.cpp 2020-06-26 16:41:15 UTC (rev 263563)
@@ -445,9 +445,9 @@
enclosingItemBoundaryElements.removeLast();
}
- if (RefPtr<Element> currentElementAncestor = is<Element>(*contentNode) ? downcast<Element>(contentNode) : contentNode->parentOrShadowHostElement()) {
- if (m_manipulatedElements.contains(*currentElementAncestor))
- return;
+ if (m_manipulatedNodes.contains(contentNode)) {
+ addItemIfPossible(std::exchange(unitsInCurrentParagraph, { }));
+ continue;
}
if (is<Element>(*contentNode)) {
@@ -502,7 +502,7 @@
void TextManipulationController::didCreateRendererForElement(Element& element)
{
- if (m_manipulatedElements.contains(element))
+ if (m_manipulatedNodes.contains(&element))
return;
if (m_elementsWithNewRenderer.computesEmpty())
@@ -515,17 +515,6 @@
m_elementsWithNewRenderer.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 Position& start, const Position& end)
-{
- return { makePositionTuple(start), makePositionTuple(end) };
-}
-
void TextManipulationController::scheduleObservationUpdate()
{
if (!m_document)
@@ -536,37 +525,32 @@
if (!controller)
return;
- HashSet<Ref<Element>> mutatedElements;
+ HashSet<Ref<Element>> elementsToObserve;
for (auto& weakElement : controller->m_elementsWithNewRenderer)
- mutatedElements.add(weakElement);
+ elementsToObserve.add(weakElement);
controller->m_elementsWithNewRenderer.clear();
- HashSet<Ref<Element>> filteredElements;
- for (auto& element : mutatedElements) {
- auto* parentElement = element->parentElement();
- if (!parentElement || !mutatedElements.contains(parentElement))
- filteredElements.add(element.copyRef());
+ if (elementsToObserve.isEmpty())
+ return;
+
+ RefPtr<Node> commonAncestor;
+ for (auto& element : elementsToObserve) {
+ if (!commonAncestor)
+ commonAncestor = makeRefPtr(element.get());
+ else if (!element->isDescendantOf(commonAncestor.get())) {
+ commonAncestor = commonInclusiveAncestor(*commonAncestor, element.get());
+ ASSERT(commonAncestor);
+ }
}
- mutatedElements.clear();
+ auto start = firstPositionInOrBeforeNode(commonAncestor.get());
+ auto end = lastPositionInOrAfterNode(commonAncestor.get());
+ controller->observeParagraphs(start, end);
- HashSet<std::pair<PositionTuple, PositionTuple>> paragraphSets;
- for (auto& element : filteredElements) {
- auto start = firstPositionInOrBeforeNode(element.ptr());
- auto end = lastPositionInOrAfterNode(element.ptr());
+ if (controller->m_items.isEmpty()) {
+ controller->m_manipulatedNodes.add(commonAncestor.get());
+ return;
+ }
- if (start.isNull() || end.isNull())
- continue;
-
- auto key = makeHashablePositionRange(start, end);
- if (!paragraphSets.add(key).isNewEntry)
- continue;
-
- auto* controller = weakThis.get();
- if (!controller)
- return;
-
- controller->observeParagraphs(start, end);
- }
controller->flushPendingItemsForCallback();
});
}
@@ -644,7 +628,7 @@
return path;
}
-void TextManipulationController::updateInsertions(Vector<NodeEntry>& lastTopDownPath, const Vector<Ref<Node>>& currentTopDownPath, Node* currentNode, HashSet<Ref<Node>>& insertedNodes, Vector<NodeInsertion>& insertions, IsNodeManipulated isNodeManipulated)
+void TextManipulationController::updateInsertions(Vector<NodeEntry>& lastTopDownPath, const Vector<Ref<Node>>& currentTopDownPath, Node* currentNode, HashSet<Ref<Node>>& insertedNodes, Vector<NodeInsertion>& insertions)
{
size_t i = 0;
while (i < lastTopDownPath.size() && i < currentTopDownPath.size() && lastTopDownPath[i].first.ptr() == currentTopDownPath[i].ptr())
@@ -668,7 +652,7 @@
}
if (currentNode)
- insertions.append(NodeInsertion { lastTopDownPath.size() ? lastTopDownPath.last().second.ptr() : nullptr, *currentNode, isNodeManipulated });
+ insertions.append(NodeInsertion { lastTopDownPath.size() ? lastTopDownPath.last().second.ptr() : nullptr, *currentNode });
}
auto TextManipulationController::replace(const ManipulationItemData& item, const Vector<ManipulationToken>& replacementTokens) -> Optional<ManipulationFailureType>
@@ -797,7 +781,7 @@
RefPtr<Node> endNode = end.firstNode();
if (node && node != endNode) {
auto topDownPath = getPath(commonAncestor.get(), node->parentNode());
- updateInsertions(lastTopDownPath, topDownPath, nullptr, reusedOriginalNodes, insertions, IsNodeManipulated::No);
+ updateInsertions(lastTopDownPath, topDownPath, nullptr, reusedOriginalNodes, insertions);
}
while (node != endNode) {
Ref<Node> parentNode = *node->parentNode();
@@ -816,35 +800,23 @@
for (auto& node : nodesToRemove)
node->remove();
- HashSet<Ref<Node>> parentNodesWithOnlyManipulatedChildren;
for (auto& insertion : insertions) {
- RefPtr<Node> parent;
if (!insertion.parentIfDifferentFromCommonAncestor) {
- parent = insertionPoint.containerNode();
- parent->insertBefore(insertion.child, insertionPoint.computeNodeAfterPosition());
+ insertionPoint.containerNode()->insertBefore(insertion.child, insertionPoint.computeNodeAfterPosition());
insertionPoint = positionInParentAfterNode(insertion.child.ptr());
- } else {
- parent = insertion.parentIfDifferentFromCommonAncestor;
- parent->appendChild(insertion.child);
- }
+ } else
+ insertion.parentIfDifferentFromCommonAncestor->appendChild(insertion.child);
- if (insertion.isChildManipulated == IsNodeManipulated::No) {
- parentNodesWithOnlyManipulatedChildren.remove(*parent);
- continue;
- }
-
- if (is<Element>(insertion.child.get()))
- m_manipulatedElements.add(downcast<Element>(insertion.child.get()));
- else if (parent->firstChild() == parent->lastChild())
- parentNodesWithOnlyManipulatedChildren.add(*parent);
+ if (insertion.isChildManipulated == IsNodeManipulated::Yes)
+ m_manipulatedNodes.add(insertion.child.ptr());
}
- for (auto& node : parentNodesWithOnlyManipulatedChildren) {
- if (is<Element>(node.get()))
- m_manipulatedElements.add(downcast<Element>(node.get()));
- }
-
return WTF::nullopt;
}
+void TextManipulationController::removeNode(Node* node)
+{
+ m_manipulatedNodes.remove(node);
+}
+
} // namespace WebCore
Modified: trunk/Source/WebCore/editing/TextManipulationController.h (263562 => 263563)
--- trunk/Source/WebCore/editing/TextManipulationController.h 2020-06-26 16:38:45 UTC (rev 263562)
+++ trunk/Source/WebCore/editing/TextManipulationController.h 2020-06-26 16:41:15 UTC (rev 263563)
@@ -116,6 +116,7 @@
WEBCORE_EXPORT void startObservingParagraphs(ManipulationItemCallback&&, Vector<ExclusionRule>&& = { });
void didCreateRendererForElement(Element&);
+ void removeNode(Node*);
enum class ManipulationFailureType : uint8_t {
ContentChanged,
@@ -173,12 +174,12 @@
};
using NodeEntry = std::pair<Ref<Node>, Ref<Node>>;
Vector<Ref<Node>> getPath(Node*, Node*);
- void updateInsertions(Vector<NodeEntry>&, const Vector<Ref<Node>>&, Node*, HashSet<Ref<Node>>&, Vector<NodeInsertion>&, IsNodeManipulated = IsNodeManipulated::Yes);
+ void updateInsertions(Vector<NodeEntry>&, const Vector<Ref<Node>>&, Node*, HashSet<Ref<Node>>&, Vector<NodeInsertion>&);
Optional<ManipulationFailureType> replace(const ManipulationItemData&, const Vector<ManipulationToken>&);
WeakPtr<Document> m_document;
WeakHashSet<Element> m_elementsWithNewRenderer;
- WeakHashSet<Element> m_manipulatedElements;
+ HashSet<Node*> m_manipulatedNodes;
HashMap<String, bool> m_cachedFontFamilyExclusionResults;
Modified: trunk/Tools/ChangeLog (263562 => 263563)
--- trunk/Tools/ChangeLog 2020-06-26 16:38:45 UTC (rev 263562)
+++ trunk/Tools/ChangeLog 2020-06-26 16:41:15 UTC (rev 263563)
@@ -1,3 +1,13 @@
+2020-06-26 Sihui Liu <[email protected]>
+
+ Text manipulation should observe adjacent elements with new renderer together
+ https://bugs.webkit.org/show_bug.cgi?id=213333
+
+ Reviewed by Geoffrey Garen.
+
+ * TestWebKitAPI/Tests/WebKitCocoa/TextManipulation.mm:
+ (TestWebKitAPI::TEST):
+
2020-06-26 Jonathan Bedard <[email protected]>
[Big Sur] Handle baseline search path
Modified: trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/TextManipulation.mm (263562 => 263563)
--- trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/TextManipulation.mm 2020-06-26 16:38:45 UTC (rev 263562)
+++ trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/TextManipulation.mm 2020-06-26 16:41:15 UTC (rev 263563)
@@ -335,31 +335,27 @@
done = false;
delegate.get().itemCallback = ^(_WKTextManipulationItem *item) {
- if (items.count == 3)
- done = true;
+ done = true;
};
[webView stringByEvaluatingJavaScript:@"document.querySelectorAll('span.hidden').forEach((span) => span.classList.remove('hidden'));"];
TestWebKitAPI::Util::run(&done);
- EXPECT_EQ(items.count, 3UL);
- EXPECT_EQ(items[1].tokens.count, 1UL);
- EXPECT_EQ(items[2].tokens.count, 1UL);
- RetainPtr<NSSet> tokens = adoptNS([[NSSet alloc] initWithObjects:items[1].tokens[0].content, items[2].tokens[0].content, nil]);
- EXPECT_TRUE([tokens.get() containsObject:@"Web"]);
- EXPECT_TRUE([tokens.get() containsObject:@"Kit"]);
+ 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 == 4)
- done = true;
+ done = true;
};
[webView stringByEvaluatingJavaScript:@"document.querySelectorAll('div.hidden').forEach((div) => div.classList.remove('hidden'));"];
TestWebKitAPI::Util::run(&done);
- EXPECT_EQ(items.count, 4UL);
- EXPECT_EQ(items[3].tokens.count, 1UL);
- EXPECT_STREQ("hey", items[3].tokens[0].content.UTF8String);
+ EXPECT_EQ(items.count, 3UL);
+ EXPECT_EQ(items[2].tokens.count, 1UL);
+ EXPECT_STREQ("hey", items[2].tokens[0].content.UTF8String);
}
TEST(TextManipulation, StartTextManipulationFindSameParagraphWithNewContent)
@@ -2633,7 +2629,7 @@
auto webView = adoptNS([[TestWKWebView alloc] initWithFrame:NSMakeRect(0, 0, 400, 400)]);
[webView _setTextManipulationDelegate:delegate.get()];
- [webView synchronouslyLoadHTMLString:@"<p>foo</p>"];
+ [webView synchronouslyLoadHTMLString:@"<p>foo<br>bar</p>"];
done = false;
[webView _startTextManipulationsWithConfiguration:nil completion:^{
@@ -2642,14 +2638,16 @@
TestWebKitAPI::Util::run(&done);
auto items = [delegate items];
- auto item = items[0];
- EXPECT_EQ(items.count, 1UL);
- EXPECT_EQ(item.tokens.count, 1UL);
- EXPECT_WK_STREQ("foo", item.tokens[0].content);
+ EXPECT_EQ(items.count, 2UL);
+ EXPECT_EQ(items[0].tokens.count, 1UL);
+ EXPECT_WK_STREQ("foo", items[0].tokens[0].content);
+ EXPECT_EQ(items[1].tokens.count, 1UL);
+ EXPECT_WK_STREQ("bar", items[1].tokens[0].content);
done = false;
[webView _completeTextManipulationForItems:@[
- createItem(item.identifier, {{ item.tokens[0].identifier, @"bar" }}).get()
+ createItem(items[0].identifier, {{ items[0].tokens[0].identifier, @"FOO" }}).get(),
+ createItem(items[1].identifier, {{ items[1].tokens[0].identifier, @"BAR" }}).get()
] completion:^(NSArray<NSError *> *errors) {
EXPECT_EQ(errors, nil);
done = true;
@@ -2656,7 +2654,7 @@
}];
TestWebKitAPI::Util::run(&done);
- EXPECT_WK_STREQ("<p>bar</p>", [webView stringByEvaluatingJavaScript:@"document.body.innerHTML"]);
+ EXPECT_WK_STREQ("<p>FOO<br>BAR</p>", [webView stringByEvaluatingJavaScript:@"document.body.innerHTML"]);
__block bool itemCallbackFired = false;
[delegate setItemCallback:^(_WKTextManipulationItem *) {
@@ -2664,12 +2662,20 @@
}];
[webView objectByEvaluatingJavaScript:@"document.querySelector('p').style.display = 'none'"];
- [webView waitForNextPresentationUpdate];
-
[webView objectByEvaluatingJavaScript:@"document.querySelector('p').style.display = ''"];
- [webView waitForNextPresentationUpdate];
+ [webView stringByEvaluatingJavaScript:@"var element = document.createElement('span');"
+ "element.innerHTML='end';"
+ "document.querySelector('p').after(element)"];
- EXPECT_FALSE(itemCallbackFired);
+ done = false;
+ delegate.get().itemCallback = ^(_WKTextManipulationItem *item) {
+ done = true;
+ };
+ TestWebKitAPI::Util::run(&done);
+
+ EXPECT_EQ(items.count, 3UL);
+ EXPECT_EQ(items[2].tokens.count, 1UL);
+ EXPECT_WK_STREQ("end", items[2].tokens[0].content);
}
TEST(TextManipulation, TextManipulationTokenDebugDescription)
