Changeset 260578 in webkit


Ignore:
Timestamp:
Apr 23, 2020 10:23:54 AM (4 years ago)
Author:
sihui_liu@apple.com
Message:

TextManipulationController should set range of paragraph using token's positions
​https://bugs.webkit.org/show_bug.cgi?id=210866
<rdar://problem/60646283>

Reviewed by Wenson Hsieh.

Source/WebCore:

Set the range of paragraph using positions of first token and last token in the paragraph because:

  1. Accurate range makes token matching in TextManipulationController::replace() easier, as TextIterator could

visit different positions with different ranges or different conditions. For example, in our previous
implementation, start of a paragraph can be set as the first visible position of document, while position of
first token is after that. Then in replace(), TextManipulationController may extract a word before the position
of first token and return error. See added test TextManipulation.CompleteTextManipulationCorrectParagraphRange.

  1. TextManipulationController can handle fewer content and this is less error-prone. For example, svg elements

before/after the paragraph text will not be identified as tokens [] in a paragraph now. See updated API tests
for example.

New test: TextManipulation.CompleteTextManipulationCorrectParagraphRange

  • editing/TextManipulationController.cpp:

(WebCore::ParagraphContentIterator::moveCurrentNodeForward): m_currentNodeForFindingInvisibleContent should not
be advanced if it is already at the end.
(WebCore::containsOnlyHTMLSpaces):
(WebCore::TextManipulationController::observeParagraphs):Set the paragraph start as the position of the first
token and end as the position of last token. If the paragraph is split with <br>, the end will be extended to
position of <br> so that we can add this node back later; otherwise, <br> can be removed after original
text of paragraph is removed in TextManipulationController::replace(). Also, stop identifying spaces as tokens
because non-text Node can emit spaces.
(WebCore::TextManipulationController::replace): Only identify tokens from content with meaningful text.

Tools:

  • TestWebKitAPI/Tests/WebKitCocoa/TextManipulation.mm:

(TestWebKitAPI::TEST):

Location:
trunk
Files:
4 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebCore/ChangeLog

    r260576 r260578  
     12020-04-23  Sihui Liu  <sihui_liu@apple.com>
     2
     3        TextManipulationController should set range of paragraph using token's positions
     4        https://bugs.webkit.org/show_bug.cgi?id=210866
     5        <rdar://problem/60646283>
     6
     7        Reviewed by Wenson Hsieh.
     8
     9        Set the range of paragraph using positions of first token and last token in the paragraph because:
     10        1. Accurate range makes token matching in TextManipulationController::replace() easier, as TextIterator could
     11        visit different positions with different ranges or different conditions. For example, in our previous
     12        implementation, start of a paragraph can be set as the first visible position of document, while position of
     13        first token is after that. Then in replace(), TextManipulationController may extract a word before the position
     14        of first token and return error. See added test TextManipulation.CompleteTextManipulationCorrectParagraphRange.
     15        2. TextManipulationController can handle fewer content and this is less error-prone. For example, svg elements
     16        before/after the paragraph text will not be identified as tokens [] in a paragraph now. See updated API tests
     17        for example.
     18
     19        New test: TextManipulation.CompleteTextManipulationCorrectParagraphRange
     20
     21        * editing/TextManipulationController.cpp:
     22        (WebCore::ParagraphContentIterator::moveCurrentNodeForward): m_currentNodeForFindingInvisibleContent should not
     23        be advanced if it is already at the end.
     24        (WebCore::containsOnlyHTMLSpaces):
     25        (WebCore::TextManipulationController::observeParagraphs):Set the paragraph start as the position of the first
     26        token and end as the position of last token. If the paragraph is split with <br>, the end will be extended to
     27        position of <br> so that we can add this node back later; otherwise, <br> can be removed after original
     28        text of paragraph is removed in TextManipulationController::replace(). Also, stop identifying spaces as tokens
     29        because non-text Node can emit spaces.
     30        (WebCore::TextManipulationController::replace): Only identify tokens from content with meaningful text.
     31
    1322020-04-23  Chris Dumez  <cdumez@apple.com>
    233

  • trunk/Source/WebCore/editing/TextManipulationController.cpp

    r260393 r260578  
    3131#include "ElementAncestorIterator.h"
    3232#include "EventLoop.h"
     33#include "HTMLBRElement.h"
    3334#include "HTMLElement.h"
    3435#include "HTMLNames.h"
     36#include "HTMLParserIdioms.h"
    3537#include "NodeTraversal.h"
    3638#include "PseudoElement.h"
    … …  
    208210    void moveCurrentNodeForward()
    209211    {
     212        if (m_currentNodeForFindingInvisibleContent == m_pastEndNode)
     213            return;
     214
    210215        m_currentNodeForFindingInvisibleContent = NodeTraversal::next(*m_currentNodeForFindingInvisibleContent);
    211216        if (!m_currentNodeForFindingInvisibleContent)
    … …  
    243248}
    244249
     250static bool containsOnlyHTMLSpaces(StringView text)
     251{
     252    for (unsigned index = 0; index < text.length(); ++index) {
     253        if (isNotHTMLSpace(text[index]))
     254            return false;
     255    }
     256
     257    return true;
     258}
     259
    245260void TextManipulationController::observeParagraphs(const Position& start, const Position& end)
    246261{
    … …  
    258273    ExclusionRuleMatcher exclusionRuleMatcher(m_exclusionRules);
    259274    Vector<ManipulationToken> tokensInCurrentParagraph;
    260     Position startOfCurrentParagraph = visibleStart.deepEquivalent();
     275    Position startOfCurrentParagraph;
     276    Position endOfCurrentParagraph;
    261277    for (; !iterator.atEnd(); iterator.advance()) {
    262278        auto content = iterator.currentContent();
    … …  
    282298                }
    283299            }
    284 
    285             if (startOfCurrentParagraph.isNull() && content.isTextContent)
    286                 startOfCurrentParagraph = iterator.startPosition();
    287300        }
    288301
    289302        if (content.isReplacedContent) {
    290             if (startOfCurrentParagraph.isNull())
    291                 startOfCurrentParagraph = positionBeforeNode(content.node.get());
    292             tokensInCurrentParagraph.append(ManipulationToken { m_tokenIdentifier.generate(), "[]", true /* isExcluded */});
     303            if (tokensInCurrentParagraph.isEmpty())
     304                continue;
     305
     306            auto currentEndOfCurrentParagraph = positionAfterNode(content.node.get());
     307            // This is at the same Node as last token, so it is already included in current range.
     308            if (!is<Text>(content.node) && currentEndOfCurrentParagraph.equals(endOfCurrentParagraph))
     309                continue;
     310
     311            endOfCurrentParagraph = currentEndOfCurrentParagraph;
     312            tokensInCurrentParagraph.append(ManipulationToken { m_tokenIdentifier.generate(), "[]", true });
     313
    293314            continue;
    294315        }
    … …  
    301322        StringView currentText = content.text;
    302323        while ((offsetOfNextNewLine = currentText.find('\n', startOfCurrentLine)) != notFound) {
    303             if (startOfCurrentLine < offsetOfNextNewLine) {
     324            if (is<Text>(content.node) && startOfCurrentLine < offsetOfNextNewLine) {
    304325                auto stringUntilEndOfLine = currentText.substring(startOfCurrentLine, offsetOfNextNewLine - startOfCurrentLine).toString();
     326                auto& textNode = downcast<Text>(*content.node);
     327                endOfCurrentParagraph = Position(&textNode, offsetOfNextNewLine);
     328                if (tokensInCurrentParagraph.isEmpty())
     329                    startOfCurrentParagraph = Position(&textNode, startOfCurrentLine);
     330
    305331                tokensInCurrentParagraph.append(ManipulationToken { m_tokenIdentifier.generate(), stringUntilEndOfLine, exclusionRuleMatcher.isExcluded(content.node.get()) });
    306332            }
    307333
    308334            if (!tokensInCurrentParagraph.isEmpty()) {
    309                 Position endOfCurrentParagraph = iterator.endPosition();
    310                 if (is<Text>(content.node)) {
    311                     auto& textNode = downcast<Text>(*content.node);
    312                     endOfCurrentParagraph = Position(&textNode, offsetOfNextNewLine);
    313                     startOfCurrentParagraph = Position(&textNode, offsetOfNextNewLine + 1);
    314                 }
     335                if (is<HTMLBRElement>(content.node))
     336                    endOfCurrentParagraph = positionAfterNode(content.node.get());
    315337                addItem(ManipulationItemData { startOfCurrentParagraph, endOfCurrentParagraph, nullptr, nullQName(), std::exchange(tokensInCurrentParagraph, { }) });
    316                 startOfCurrentParagraph.clear();
    317338            }
    318339            startOfCurrentLine = offsetOfNextNewLine + 1;
    … …  
    320341
    321342        auto remainingText = currentText.substring(startOfCurrentLine);
    322         if (remainingText.length())
     343        if (!containsOnlyHTMLSpaces(remainingText)) {
     344            if (tokensInCurrentParagraph.isEmpty()) {
     345                if (startOfCurrentLine && is<Text>(content.node))
     346                    startOfCurrentParagraph = Position(&downcast<Text>(*content.node), startOfCurrentLine);
     347                else
     348                    startOfCurrentParagraph = iterator.startPosition();
     349            }
     350            endOfCurrentParagraph = iterator.endPosition();
    323351            tokensInCurrentParagraph.append(ManipulationToken { m_tokenIdentifier.generate(), remainingText.toString(), exclusionRuleMatcher.isExcluded(content.node.get()) });
     352        }
    324353    }
    325354
    326355    if (!tokensInCurrentParagraph.isEmpty())
    327         addItem(ManipulationItemData { startOfCurrentParagraph, visibleEnd.deepEquivalent(), nullptr, nullQName(), WTFMove(tokensInCurrentParagraph) });
     356        addItem(ManipulationItemData { startOfCurrentParagraph, endOfCurrentParagraph, nullptr, nullQName(), WTFMove(tokensInCurrentParagraph) });
    328357}
    329358
    … …  
    520549        }
    521550
     551        if (content.isTextContent && containsOnlyHTMLSpaces(content.text)) {
     552            // <br> should not exist in the middle of a paragraph.
     553            if (is<HTMLBRElement>(content.node))
     554                return ManipulationFailureType::ContentChanged;
     555            continue;
     556        }
     557
    522558        auto& currentToken = item.tokens[currentTokenIndex];
    523559        if (!content.isReplacedContent && content.text != currentToken.content)

  • trunk/Tools/ChangeLog

    r260566 r260578  
     12020-04-23  Sihui Liu  <sihui_liu@apple.com>
     2
     3        TextManipulationController should set range of paragraph using token's positions
     4        https://bugs.webkit.org/show_bug.cgi?id=210866
     5        <rdar://problem/60646283>
     6
     7        Reviewed by Wenson Hsieh.
     8
     9        * TestWebKitAPI/Tests/WebKitCocoa/TextManipulation.mm:
     10        (TestWebKitAPI::TEST):
     11
    1122020-04-23  Emilio Cobos Álvarez  <emilio@crisal.io>
    213

  • trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/TextManipulation.mm

    r260530 r260578  
    989989
    990990    auto *items = [delegate items];
    991     EXPECT_EQ(items.count, 2UL);
    992     EXPECT_EQ(items[0].tokens.count, 2UL);
    993     EXPECT_STREQ("[]", items[0].tokens[0].content.UTF8String);
    994     EXPECT_TRUE(items[0].tokens[0].isExcluded);
    995     EXPECT_STREQ("[]", items[0].tokens[1].content.UTF8String);
    996     EXPECT_TRUE(items[0].tokens[1].isExcluded);
    997 
    998     auto *tokens = items[1].tokens;
    999     EXPECT_EQ(tokens.count, 1UL);
    1000     EXPECT_STREQ("helllo world", tokens[0].content.UTF8String);
    1001     EXPECT_FALSE(tokens[0].isExcluded);
     991    EXPECT_EQ(items.count, 1UL);
     992    EXPECT_EQ(items[0].tokens.count, 1UL);
     993    EXPECT_STREQ("helllo world", items[0].tokens[0].content.UTF8String);
     994    EXPECT_TRUE(!items[0].tokens[0].isExcluded);
    1002995
    1003996    done = false;
    1004997    [webView _completeTextManipulationForItems:@[
    1005         (_WKTextManipulationItem *)createItem(items[0].identifier, { { items[0].tokens[0].identifier, nil } }),
    1006         (_WKTextManipulationItem *)createItem(items[1].identifier, { { items[1].tokens[0].identifier, @"hello, world" } }),
     998        (_WKTextManipulationItem *)createItem(items[0].identifier, { { items[0].tokens[0].identifier, @"hello, world" } }),
    1007999    ] completion:^(NSArray<NSError *> *errors) {
    10081000        EXPECT_EQ(errors, nil);
    … …  
    10771069
    10781070    auto *items = [delegate items];
    1079     EXPECT_EQ(items.count, 2UL);
    1080     EXPECT_EQ(items[1].tokens.count, 1UL);
    1081     EXPECT_STREQ("[]", items[0].tokens[0].content.UTF8String);
     1071    EXPECT_EQ(items.count, 1UL);
    10821072    EXPECT_EQ(items[0].tokens.count, 1UL);
    1083     EXPECT_STREQ("hello world", items[1].tokens[0].content.UTF8String);
     1073    EXPECT_STREQ("hello world", items[0].tokens[0].content.UTF8String);
    10841074
    10851075    done = false;
    10861076    [webView _completeTextManipulationForItems:@[
    1087         (_WKTextManipulationItem *)createItem(items[0].identifier, { { items[0].tokens[0].identifier, nil } }),
    1088         (_WKTextManipulationItem *)createItem(items[1].identifier, { { items[1].tokens[0].identifier, @"hello, world" } }),
     1077        (_WKTextManipulationItem *)createItem(items[0].identifier, { { items[0].tokens[0].identifier, @"hello, world" } }),
    10891078    ] completion:^(NSArray<NSError *> *errors) {
    10901079        EXPECT_EQ(errors, nil);
    … …  
    11631152
    11641153    auto *items = [delegate items];
    1165     EXPECT_EQ(items.count, 3UL);
     1154    EXPECT_EQ(items.count, 2UL);
    11661155    EXPECT_EQ(items[0].tokens.count, 1UL);
    11671156    EXPECT_STREQ("heeey", items[0].tokens[0].content.UTF8String);
    11681157    EXPECT_EQ(items[1].tokens.count, 1UL);
    1169     EXPECT_STREQ("[]", items[1].tokens[0].content.UTF8String);
    1170     EXPECT_EQ(items[2].tokens.count, 1UL);
    1171     EXPECT_STREQ("woorld", items[2].tokens[0].content.UTF8String);
     1158    EXPECT_STREQ("woorld", items[1].tokens[0].content.UTF8String);
    11721159
    11731160    done = false;
    11741161    [webView _completeTextManipulationForItems:@[
    11751162        (_WKTextManipulationItem *)createItem(items[0].identifier, { { items[0].tokens[0].identifier, @"hello" } }),
    1176         (_WKTextManipulationItem *)createItem(items[1].identifier, { { items[1].tokens[0].identifier, nil } }),
    1177         (_WKTextManipulationItem *)createItem(items[2].identifier, { { items[2].tokens[0].identifier, @"world" } }),
     1163        (_WKTextManipulationItem *)createItem(items[1].identifier, { { items[1].tokens[0].identifier, @"world" } }),
    11781164    ] completion:^(NSArray<NSError *> *errors) {
    11791165        EXPECT_EQ(errors, nil);
    … …  
    16851671}
    16861672
     1673TEST(TextManipulation, CompleteTextManipulationCorrectParagraphRange)
     1674{
     1675    auto delegate = adoptNS([[TextManipulationDelegate alloc] init]);
     1676    auto webView = adoptNS([[TestWKWebView alloc] initWithFrame:NSMakeRect(0, 0, 400, 400)]);
     1677    [webView _setTextManipulationDelegate:delegate.get()];
     1678
     1679    [webView synchronouslyLoadHTMLString:@"<head><style>ul{display:block}li{display:inline-block}.inline {float: left;}.subframe {height: 42px;}.frame {position: absolute;top: -9999px;}</style></head><body><div class='frame'><div class='subframe'></div></div><style></style><div class='inline'><div><li><a href='#'>holle</a></li><li><a href='#'>wdrlo</a></li></div></div><div class='frame'><div class='subframe'></div></div></body>"];
     1680
     1681    RetainPtr<_WKTextManipulationConfiguration> configuration = adoptNS([[_WKTextManipulationConfiguration alloc] init]);
     1682    done = false;
     1683    [webView _startTextManipulationsWithConfiguration:configuration.get() completion:^{
     1684        done = true;
     1685    }];
     1686    TestWebKitAPI::Util::run(&done);
     1687
     1688    auto *items = [delegate items];
     1689    EXPECT_EQ(items.count, 1UL);
     1690    EXPECT_EQ(items[0].tokens.count, 2UL);
     1691    EXPECT_STREQ("holle", items[0].tokens[0].content.UTF8String);
     1692    EXPECT_STREQ("wdrlo", items[0].tokens[1].content.UTF8String);
     1693
     1694    done = false;
     1695    [webView _completeTextManipulationForItems:@[(_WKTextManipulationItem *)createItem(items[0].identifier, {
     1696        { items[0].tokens[0].identifier, @"hello" },
     1697        { items[0].tokens[1].identifier, @"world" },
     1698    })] completion:^(NSArray<NSError *> *errors) {
     1699        EXPECT_EQ(errors, nil);
     1700        done = true;
     1701    }];
     1702    TestWebKitAPI::Util::run(&done);
     1703
     1704    EXPECT_WK_STREQ("<div class=\"frame\"><div class=\"subframe\"></div></div><style></style><div class=\"inline\"><div><li><a href=\"#\">hello</a></li><li><a href=\"#\">world</a></li></div></div><div class=\"frame\"><div class=\"subframe\"></div></div>", [webView stringByEvaluatingJavaScript:@"document.body.innerHTML"]);
     1705}
     1706
    16871707TEST(TextManipulation, InsertingContentIntoAlreadyManipulatedContentDoesNotCreateTextManipulationItem)
    16881708{
Note: See TracChangeset for help on using the changeset viewer.