Changeset 263563 in webkit


Ignore:
Timestamp:
Jun 26, 2020 9:41:15 AM (4 years ago)
Author:
sihui_liu@apple.com
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):

Location:
trunk
Files:
6 edited

Legend:

Unmodified
Added
Removed
  • trunk/Source/WebCore/ChangeLog

    r263562 r263563  
     12020-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
    1382020-06-26  Simon Fraser  <simon.fraser@apple.com>
    239
  • trunk/Source/WebCore/dom/Node.cpp

    r263302 r263563  
    7474#include "TemplateContentDocumentFragment.h"
    7575#include "TextEvent.h"
     76#include "TextManipulationController.h"
    7677#include "TouchEvent.h"
    7778#include "WheelEvent.h"
     
    361362        clearRareData();
    362363
     364    auto* textManipulationController = document().textManipulationControllerIfExists();
     365    if (UNLIKELY(textManipulationController))
     366        textManipulationController->removeNode(this);
     367
    363368    if (!isContainerNode())
    364369        willBeDeletedFrom(document());
  • trunk/Source/WebCore/editing/TextManipulationController.cpp

    r263527 r263563  
    446446        }
    447447
    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;
    451451        }
    452452
     
    503503void TextManipulationController::didCreateRendererForElement(Element& element)
    504504{
    505     if (m_manipulatedElements.contains(element))
     505    if (m_manipulatedNodes.contains(&element))
    506506        return;
    507507
     
    516516}
    517517
    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 
    529518void TextManipulationController::scheduleObservationUpdate()
    530519{
     
    537526            return;
    538527
    539         HashSet<Ref<Element>> mutatedElements;
     528        HashSet<Ref<Element>> elementsToObserve;
    540529        for (auto& weakElement : controller->m_elementsWithNewRenderer)
    541             mutatedElements.add(weakElement);
     530            elementsToObserve.add(weakElement);
    542531        controller->m_elementsWithNewRenderer.clear();
    543532
    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
    570554        controller->flushPendingItemsForCallback();
    571555    });
     
    645629}
    646630
    647 void TextManipulationController::updateInsertions(Vector<NodeEntry>& lastTopDownPath, const Vector<Ref<Node>>& currentTopDownPath, Node* currentNode, HashSet<Ref<Node>>& insertedNodes, Vector<NodeInsertion>& insertions, IsNodeManipulated isNodeManipulated)
     631void TextManipulationController::updateInsertions(Vector<NodeEntry>& lastTopDownPath, const Vector<Ref<Node>>& currentTopDownPath, Node* currentNode, HashSet<Ref<Node>>& insertedNodes, Vector<NodeInsertion>& insertions)
    648632{
    649633    size_t i = 0;
     
    669653
    670654    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 });
    672656}
    673657
     
    798782    if (node && node != endNode) {
    799783        auto topDownPath = getPath(commonAncestor.get(), node->parentNode());
    800         updateInsertions(lastTopDownPath, topDownPath, nullptr, reusedOriginalNodes, insertions, IsNodeManipulated::No);
     784        updateInsertions(lastTopDownPath, topDownPath, nullptr, reusedOriginalNodes, insertions);
    801785    }
    802786    while (node != endNode) {
     
    817801        node->remove();
    818802
    819     HashSet<Ref<Node>> parentNodesWithOnlyManipulatedChildren;
    820803    for (auto& insertion : insertions) {
    821         RefPtr<Node> parent;
    822804        if (!insertion.parentIfDifferentFromCommonAncestor) {
    823             parent = insertionPoint.containerNode();
    824             parent->insertBefore(insertion.child, insertionPoint.computeNodeAfterPosition());
     805            insertionPoint.containerNode()->insertBefore(insertion.child, insertionPoint.computeNodeAfterPosition());
    825806            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());
    845812    }
    846813
     
    848815}
    849816
     817void TextManipulationController::removeNode(Node* node)
     818{
     819    m_manipulatedNodes.remove(node);
     820}
     821
    850822} // namespace WebCore
  • trunk/Source/WebCore/editing/TextManipulationController.h

    r263527 r263563  
    117117
    118118    void didCreateRendererForElement(Element&);
     119    void removeNode(Node*);
    119120
    120121    enum class ManipulationFailureType : uint8_t {
     
    174175    using NodeEntry = std::pair<Ref<Node>, Ref<Node>>;
    175176    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>&);
    177178    Optional<ManipulationFailureType> replace(const ManipulationItemData&, const Vector<ManipulationToken>&);
    178179
    179180    WeakPtr<Document> m_document;
    180181    WeakHashSet<Element> m_elementsWithNewRenderer;
    181     WeakHashSet<Element> m_manipulatedElements;
     182    HashSet<Node*> m_manipulatedNodes;
    182183
    183184    HashMap<String, bool> m_cachedFontFamilyExclusionResults;
  • trunk/Tools/ChangeLog

    r263561 r263563  
     12020-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
    1112020-06-26  Jonathan Bedard  <jbedard@apple.com>
    212
  • trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/TextManipulation.mm

    r263527 r263563  
    336336    done = false;
    337337    delegate.get().itemCallback = ^(_WKTextManipulationItem *item) {
    338         if (items.count == 3)
    339             done = true;
     338        done = true;
    340339    };
    341340    [webView stringByEvaluatingJavaScript:@"document.querySelectorAll('span.hidden').forEach((span) => span.classList.remove('hidden'));"];
    342341    TestWebKitAPI::Util::run(&done);
    343342
    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);
    350347
    351348    // This has to happen separately in order to have a deterministic ordering.
    352349    done = false;
    353350    delegate.get().itemCallback = ^(_WKTextManipulationItem *item) {
    354         if (items.count == 4)
    355             done = true;
     351        done = true;
    356352    };
    357353    [webView stringByEvaluatingJavaScript:@"document.querySelectorAll('div.hidden').forEach((div) => div.classList.remove('hidden'));"];
    358354    TestWebKitAPI::Util::run(&done);
    359355
    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);
    363359}
    364360
     
    26342630    [webView _setTextManipulationDelegate:delegate.get()];
    26352631
    2636     [webView synchronouslyLoadHTMLString:@"<p>foo</p>"];
     2632    [webView synchronouslyLoadHTMLString:@"<p>foo<br>bar</p>"];
    26372633
    26382634    done = false;
     
    26432639
    26442640    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);
    26492646
    26502647    done = false;
    26512648    [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()
    26532651    ] completion:^(NSArray<NSError *> *errors) {
    26542652        EXPECT_EQ(errors, nil);
     
    26572655    TestWebKitAPI::Util::run(&done);
    26582656
    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"]);
    26602658
    26612659    __block bool itemCallbackFired = false;
     
    26652663
    26662664    [webView objectByEvaluatingJavaScript:@"document.querySelector('p').style.display = 'none'"];
    2667     [webView waitForNextPresentationUpdate];
    2668 
    26692665    [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);
    26732679}
    26742680
Note: See TracChangeset for help on using the changeset viewer.