Changeset 263563 in webkit
- Timestamp:
- Jun 26, 2020 9:41:15 AM (4 years ago)
- Location:
- trunk
- Files:
-
- 6 edited
Legend:
- Unmodified
- Added
- Removed
-
trunk/Source/WebCore/ChangeLog
r263562 r263563 1 2020-06-26 Sihui Liu <sihui_liu@apple.com> 2 3 Text manipulation should observe adjacent elements with new renderer together 4 https://bugs.webkit.org/show_bug.cgi?id=213333 5 6 Reviewed by Geoffrey Garen. 7 8 TextManipulationController only keeps track of manipulated Elements. For other types of Node, like Text, 9 TextManipulationController does not mark them as manipulated and may manipulate it again. r263132 tried to solve 10 the problem by marking Element with only manipulated children as manipulated, but it did not iterate all 11 children of Element. So, if one Element has children in different paragraphs, it may fail to mark the Element 12 manipulated. See updated test. 13 To fix this issue completely, TextManipulationController now tracks all manipulated Nodes, so it can skip the 14 manipulated Nodes during observation. 15 16 For elements with new renderer, TextManipulationController observes them one by one. However, adjacent elements 17 can be in the same paragraph, if there is no delimiter in their text, and should be observed together. To solve 18 this problem, TextManipulationController now starts observing from the common ancestor of these elements. If a 19 Node in range is manipulated, split the paragrph there. This makes sure content in paragraph is not manipulated. 20 Relevant test is updated for this new behavior. 21 22 Tests: TextManipulation.StartTextManipulationFindNewlyDisplayedParagraph 23 TextManipulation.CompleteTextManipulationAvoidExtractingManipulatedTextAfterManipulation 24 25 * dom/Node.cpp: 26 (WebCore::Node::~Node): 27 * editing/TextManipulationController.cpp: 28 (WebCore::TextManipulationController::observeParagraphs): 29 (WebCore::TextManipulationController::didCreateRendererForElement): 30 (WebCore::TextManipulationController::scheduleObservationUpdate): 31 (WebCore::TextManipulationController::updateInsertions): 32 (WebCore::TextManipulationController::replace): 33 (WebCore::TextManipulationController::removeNode): 34 (WebCore::makePositionTuple): Deleted. 35 (WebCore::makeHashablePositionRange): Deleted. 36 * editing/TextManipulationController.h: 37 1 38 2020-06-26 Simon Fraser <simon.fraser@apple.com> 2 39 -
trunk/Source/WebCore/dom/Node.cpp
r263302 r263563 74 74 #include "TemplateContentDocumentFragment.h" 75 75 #include "TextEvent.h" 76 #include "TextManipulationController.h" 76 77 #include "TouchEvent.h" 77 78 #include "WheelEvent.h" … … 361 362 clearRareData(); 362 363 364 auto* textManipulationController = document().textManipulationControllerIfExists(); 365 if (UNLIKELY(textManipulationController)) 366 textManipulationController->removeNode(this); 367 363 368 if (!isContainerNode()) 364 369 willBeDeletedFrom(document()); -
trunk/Source/WebCore/editing/TextManipulationController.cpp
r263527 r263563 446 446 } 447 447 448 if ( RefPtr<Element> currentElementAncestor = is<Element>(*contentNode) ? downcast<Element>(contentNode) : contentNode->parentOrShadowHostElement()) {449 if (m_manipulatedElements.contains(*currentElementAncestor))450 return;448 if (m_manipulatedNodes.contains(contentNode)) { 449 addItemIfPossible(std::exchange(unitsInCurrentParagraph, { })); 450 continue; 451 451 } 452 452 … … 503 503 void TextManipulationController::didCreateRendererForElement(Element& element) 504 504 { 505 if (m_manipulated Elements.contains(element))505 if (m_manipulatedNodes.contains(&element)) 506 506 return; 507 507 … … 516 516 } 517 517 518 using PositionTuple = std::tuple<RefPtr<Node>, unsigned, unsigned>;519 static const PositionTuple makePositionTuple(const Position& position)520 {521 return { position.anchorNode(), static_cast<unsigned>(position.anchorType()), position.anchorType() == Position::PositionIsOffsetInAnchor ? position.offsetInContainerNode() : 0 };522 }523 524 static const std::pair<PositionTuple, PositionTuple> makeHashablePositionRange(const Position& start, const Position& end)525 {526 return { makePositionTuple(start), makePositionTuple(end) };527 }528 529 518 void TextManipulationController::scheduleObservationUpdate() 530 519 { … … 537 526 return; 538 527 539 HashSet<Ref<Element>> mutatedElements;528 HashSet<Ref<Element>> elementsToObserve; 540 529 for (auto& weakElement : controller->m_elementsWithNewRenderer) 541 mutatedElements.add(weakElement);530 elementsToObserve.add(weakElement); 542 531 controller->m_elementsWithNewRenderer.clear(); 543 532 544 HashSet<Ref<Element>> filteredElements; 545 for (auto& element : mutatedElements) { 546 auto* parentElement = element->parentElement(); 547 if (!parentElement || !mutatedElements.contains(parentElement)) 548 filteredElements.add(element.copyRef()); 549 } 550 mutatedElements.clear(); 551 552 HashSet<std::pair<PositionTuple, PositionTuple>> paragraphSets; 553 for (auto& element : filteredElements) { 554 auto start = firstPositionInOrBeforeNode(element.ptr()); 555 auto end = lastPositionInOrAfterNode(element.ptr()); 556 557 if (start.isNull() || end.isNull()) 558 continue; 559 560 auto key = makeHashablePositionRange(start, end); 561 if (!paragraphSets.add(key).isNewEntry) 562 continue; 563 564 auto* controller = weakThis.get(); 565 if (!controller) 566 return; 567 568 controller->observeParagraphs(start, end); 569 } 533 if (elementsToObserve.isEmpty()) 534 return; 535 536 RefPtr<Node> commonAncestor; 537 for (auto& element : elementsToObserve) { 538 if (!commonAncestor) 539 commonAncestor = makeRefPtr(element.get()); 540 else if (!element->isDescendantOf(commonAncestor.get())) { 541 commonAncestor = commonInclusiveAncestor(*commonAncestor, element.get()); 542 ASSERT(commonAncestor); 543 } 544 } 545 auto start = firstPositionInOrBeforeNode(commonAncestor.get()); 546 auto end = lastPositionInOrAfterNode(commonAncestor.get()); 547 controller->observeParagraphs(start, end); 548 549 if (controller->m_items.isEmpty()) { 550 controller->m_manipulatedNodes.add(commonAncestor.get()); 551 return; 552 } 553 570 554 controller->flushPendingItemsForCallback(); 571 555 }); … … 645 629 } 646 630 647 void TextManipulationController::updateInsertions(Vector<NodeEntry>& lastTopDownPath, const Vector<Ref<Node>>& currentTopDownPath, Node* currentNode, HashSet<Ref<Node>>& insertedNodes, Vector<NodeInsertion>& insertions , IsNodeManipulated isNodeManipulated)631 void TextManipulationController::updateInsertions(Vector<NodeEntry>& lastTopDownPath, const Vector<Ref<Node>>& currentTopDownPath, Node* currentNode, HashSet<Ref<Node>>& insertedNodes, Vector<NodeInsertion>& insertions) 648 632 { 649 633 size_t i = 0; … … 669 653 670 654 if (currentNode) 671 insertions.append(NodeInsertion { lastTopDownPath.size() ? lastTopDownPath.last().second.ptr() : nullptr, *currentNode , isNodeManipulated});655 insertions.append(NodeInsertion { lastTopDownPath.size() ? lastTopDownPath.last().second.ptr() : nullptr, *currentNode }); 672 656 } 673 657 … … 798 782 if (node && node != endNode) { 799 783 auto topDownPath = getPath(commonAncestor.get(), node->parentNode()); 800 updateInsertions(lastTopDownPath, topDownPath, nullptr, reusedOriginalNodes, insertions , IsNodeManipulated::No);784 updateInsertions(lastTopDownPath, topDownPath, nullptr, reusedOriginalNodes, insertions); 801 785 } 802 786 while (node != endNode) { … … 817 801 node->remove(); 818 802 819 HashSet<Ref<Node>> parentNodesWithOnlyManipulatedChildren;820 803 for (auto& insertion : insertions) { 821 RefPtr<Node> parent;822 804 if (!insertion.parentIfDifferentFromCommonAncestor) { 823 parent = insertionPoint.containerNode(); 824 parent->insertBefore(insertion.child, insertionPoint.computeNodeAfterPosition()); 805 insertionPoint.containerNode()->insertBefore(insertion.child, insertionPoint.computeNodeAfterPosition()); 825 806 insertionPoint = positionInParentAfterNode(insertion.child.ptr()); 826 } else { 827 parent = insertion.parentIfDifferentFromCommonAncestor; 828 parent->appendChild(insertion.child); 829 } 830 831 if (insertion.isChildManipulated == IsNodeManipulated::No) { 832 parentNodesWithOnlyManipulatedChildren.remove(*parent); 833 continue; 834 } 835 836 if (is<Element>(insertion.child.get())) 837 m_manipulatedElements.add(downcast<Element>(insertion.child.get())); 838 else if (parent->firstChild() == parent->lastChild()) 839 parentNodesWithOnlyManipulatedChildren.add(*parent); 840 } 841 842 for (auto& node : parentNodesWithOnlyManipulatedChildren) { 843 if (is<Element>(node.get())) 844 m_manipulatedElements.add(downcast<Element>(node.get())); 807 } else 808 insertion.parentIfDifferentFromCommonAncestor->appendChild(insertion.child); 809 810 if (insertion.isChildManipulated == IsNodeManipulated::Yes) 811 m_manipulatedNodes.add(insertion.child.ptr()); 845 812 } 846 813 … … 848 815 } 849 816 817 void TextManipulationController::removeNode(Node* node) 818 { 819 m_manipulatedNodes.remove(node); 820 } 821 850 822 } // namespace WebCore -
trunk/Source/WebCore/editing/TextManipulationController.h
r263527 r263563 117 117 118 118 void didCreateRendererForElement(Element&); 119 void removeNode(Node*); 119 120 120 121 enum class ManipulationFailureType : uint8_t { … … 174 175 using NodeEntry = std::pair<Ref<Node>, Ref<Node>>; 175 176 Vector<Ref<Node>> getPath(Node*, Node*); 176 void updateInsertions(Vector<NodeEntry>&, const Vector<Ref<Node>>&, Node*, HashSet<Ref<Node>>&, Vector<NodeInsertion>& , IsNodeManipulated = IsNodeManipulated::Yes);177 void updateInsertions(Vector<NodeEntry>&, const Vector<Ref<Node>>&, Node*, HashSet<Ref<Node>>&, Vector<NodeInsertion>&); 177 178 Optional<ManipulationFailureType> replace(const ManipulationItemData&, const Vector<ManipulationToken>&); 178 179 179 180 WeakPtr<Document> m_document; 180 181 WeakHashSet<Element> m_elementsWithNewRenderer; 181 WeakHashSet<Element> m_manipulatedElements;182 HashSet<Node*> m_manipulatedNodes; 182 183 183 184 HashMap<String, bool> m_cachedFontFamilyExclusionResults; -
trunk/Tools/ChangeLog
r263561 r263563 1 2020-06-26 Sihui Liu <sihui_liu@apple.com> 2 3 Text manipulation should observe adjacent elements with new renderer together 4 https://bugs.webkit.org/show_bug.cgi?id=213333 5 6 Reviewed by Geoffrey Garen. 7 8 * TestWebKitAPI/Tests/WebKitCocoa/TextManipulation.mm: 9 (TestWebKitAPI::TEST): 10 1 11 2020-06-26 Jonathan Bedard <jbedard@apple.com> 2 12 -
trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/TextManipulation.mm
r263527 r263563 336 336 done = false; 337 337 delegate.get().itemCallback = ^(_WKTextManipulationItem *item) { 338 if (items.count == 3) 339 done = true; 338 done = true; 340 339 }; 341 340 [webView stringByEvaluatingJavaScript:@"document.querySelectorAll('span.hidden').forEach((span) => span.classList.remove('hidden'));"]; 342 341 TestWebKitAPI::Util::run(&done); 343 342 344 EXPECT_EQ(items.count, 3UL); 345 EXPECT_EQ(items[1].tokens.count, 1UL); 346 EXPECT_EQ(items[2].tokens.count, 1UL); 347 RetainPtr<NSSet> tokens = adoptNS([[NSSet alloc] initWithObjects:items[1].tokens[0].content, items[2].tokens[0].content, nil]); 348 EXPECT_TRUE([tokens.get() containsObject:@"Web"]); 349 EXPECT_TRUE([tokens.get() containsObject:@"Kit"]); 343 EXPECT_EQ(items.count, 2UL); 344 EXPECT_EQ(items[1].tokens.count, 2UL); 345 EXPECT_STREQ("Web", items[1].tokens[0].content.UTF8String); 346 EXPECT_STREQ("Kit", items[1].tokens[1].content.UTF8String); 350 347 351 348 // This has to happen separately in order to have a deterministic ordering. 352 349 done = false; 353 350 delegate.get().itemCallback = ^(_WKTextManipulationItem *item) { 354 if (items.count == 4) 355 done = true; 351 done = true; 356 352 }; 357 353 [webView stringByEvaluatingJavaScript:@"document.querySelectorAll('div.hidden').forEach((div) => div.classList.remove('hidden'));"]; 358 354 TestWebKitAPI::Util::run(&done); 359 355 360 EXPECT_EQ(items.count, 4UL);361 EXPECT_EQ(items[ 3].tokens.count, 1UL);362 EXPECT_STREQ("hey", items[ 3].tokens[0].content.UTF8String);356 EXPECT_EQ(items.count, 3UL); 357 EXPECT_EQ(items[2].tokens.count, 1UL); 358 EXPECT_STREQ("hey", items[2].tokens[0].content.UTF8String); 363 359 } 364 360 … … 2634 2630 [webView _setTextManipulationDelegate:delegate.get()]; 2635 2631 2636 [webView synchronouslyLoadHTMLString:@"<p>foo< /p>"];2632 [webView synchronouslyLoadHTMLString:@"<p>foo<br>bar</p>"]; 2637 2633 2638 2634 done = false; … … 2643 2639 2644 2640 auto items = [delegate items]; 2645 auto item = items[0]; 2646 EXPECT_EQ(items.count, 1UL); 2647 EXPECT_EQ(item.tokens.count, 1UL); 2648 EXPECT_WK_STREQ("foo", item.tokens[0].content); 2641 EXPECT_EQ(items.count, 2UL); 2642 EXPECT_EQ(items[0].tokens.count, 1UL); 2643 EXPECT_WK_STREQ("foo", items[0].tokens[0].content); 2644 EXPECT_EQ(items[1].tokens.count, 1UL); 2645 EXPECT_WK_STREQ("bar", items[1].tokens[0].content); 2649 2646 2650 2647 done = false; 2651 2648 [webView _completeTextManipulationForItems:@[ 2652 createItem(item.identifier, {{ item.tokens[0].identifier, @"bar" }}).get() 2649 createItem(items[0].identifier, {{ items[0].tokens[0].identifier, @"FOO" }}).get(), 2650 createItem(items[1].identifier, {{ items[1].tokens[0].identifier, @"BAR" }}).get() 2653 2651 ] completion:^(NSArray<NSError *> *errors) { 2654 2652 EXPECT_EQ(errors, nil); … … 2657 2655 TestWebKitAPI::Util::run(&done); 2658 2656 2659 EXPECT_WK_STREQ("<p> bar</p>", [webView stringByEvaluatingJavaScript:@"document.body.innerHTML"]);2657 EXPECT_WK_STREQ("<p>FOO<br>BAR</p>", [webView stringByEvaluatingJavaScript:@"document.body.innerHTML"]); 2660 2658 2661 2659 __block bool itemCallbackFired = false; … … 2665 2663 2666 2664 [webView objectByEvaluatingJavaScript:@"document.querySelector('p').style.display = 'none'"]; 2667 [webView waitForNextPresentationUpdate];2668 2669 2665 [webView objectByEvaluatingJavaScript:@"document.querySelector('p').style.display = ''"]; 2670 [webView waitForNextPresentationUpdate]; 2671 2672 EXPECT_FALSE(itemCallbackFired); 2666 [webView stringByEvaluatingJavaScript:@"var element = document.createElement('span');" 2667 "element.innerHTML='end';" 2668 "document.querySelector('p').after(element)"]; 2669 2670 done = false; 2671 delegate.get().itemCallback = ^(_WKTextManipulationItem *item) { 2672 done = true; 2673 }; 2674 TestWebKitAPI::Util::run(&done); 2675 2676 EXPECT_EQ(items.count, 3UL); 2677 EXPECT_EQ(items[2].tokens.count, 1UL); 2678 EXPECT_WK_STREQ("end", items[2].tokens[0].content); 2673 2679 } 2674 2680
Note: See TracChangeset
for help on using the changeset viewer.