Changeset 201701 in webkit


Ignore:
Timestamp:
Jun 5, 2016, 12:48:26 PM (9 years ago)
Author:
Antti Koivisto
Message:

Source/WebCore:
Find on page finds too many matches
​https://bugs.webkit.org/show_bug.cgi?id=158395
rdar://problem/7440637

Reviewed by Dan Bernstein and Darin Adler.

There is a long standing bug where in some cases WebKit may find non-visible text matches when doing find on page.
For example searching patch review view in bugs.webkit.org returns twice as many matches as there actually are
on the page. This happens because the text content is replicated in an invisible subframe.

Fix by making TextIterator ignore content in non-visible subframes in findPlainText.

Test: editing/text-iterator/count-matches-in-frames.html

  • editing/TextIterator.cpp:

(WebCore::nextInPreOrderCrossingShadowBoundaries):

Remove support for an uninteresting assertion.

(WebCore::fullyClipsContents):

Elements without renderer clip their content (except for display:contents).
Test the content rect instead of the size rect for emptiness.

(WebCore::ignoresContainerClip):
(WebCore::pushFullyClippedState):
(WebCore::setUpFullyClippedStack):
(WebCore::isClippedByFrameAncestor):

Test if the frame owner element is clipped in any of the parent frames.

(WebCore::TextIterator::TextIterator):

If the frame is clipped by its ancestors the iterator is initialized to end state.
Clipped frame never renders anything so there is no need to maintain clip stack and traverse.

(WebCore::findPlainText):

Use TextIteratorClipsToFrameAncestors behavior. There might be other places where
this behavior should be used (or perhaps it should be used always?) but limit this to
text search for now.

(WebCore::depthCrossingShadowBoundaries): Deleted.

  • editing/TextIterator.h:
  • editing/TextIteratorBehavior.h:

Add TextIteratorClipsToFrameAncestors behavior.

  • testing/Internals.cpp:

(WebCore::Internals::countMatchesForText):
(WebCore::Internals::countFindMatches):
(WebCore::Internals::numberOfLiveNodes):

  • testing/Internals.h:
  • testing/Internals.idl:

Testing support

LayoutTests:
TextIterator should ignore non-visible frames in findPlainText
​https://bugs.webkit.org/show_bug.cgi?id=158395

Reviewed by Dan Bernstein and Darin Adler.

  • editing/text-iterator/count-matches-in-frames-expected.txt: Added.
  • editing/text-iterator/count-matches-in-frames.html: Added.
  • imported/blink/fast/shapes/shape-outside-floats/shape-outside-negative-height-crash-width.html: Non-rendered whitespace change.
Location:
trunk
Files:
2 added
9 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r201685 r201701  
     12016-06-05  Antti Koivisto  <antti@apple.com>
     2
     3        TextIterator should ignore non-visible frames in findPlainText
     4        https://bugs.webkit.org/show_bug.cgi?id=158395
     5
     6        Reviewed by Dan Bernstein and Darin Adler.
     7
     8        * editing/text-iterator/count-matches-in-frames-expected.txt: Added.
     9        * editing/text-iterator/count-matches-in-frames.html: Added.
     10        * imported/blink/fast/shapes/shape-outside-floats/shape-outside-negative-height-crash-width.html: Non-rendered whitespace change.
     11
    1122016-06-04  Brady Eidson  <beidson@apple.com>
    213

  • trunk/LayoutTests/imported/blink/fast/shapes/shape-outside-floats/shape-outside-negative-height-crash-width-expected.txt

    r190629 r201701  
    11Test passes if it does not crash.
    22
    3 

  • trunk/Source/WebCore/ChangeLog

    r201700 r201701  
     12016-06-05  Antti Koivisto  <antti@apple.com>
     2
     3        Find on page finds too many matches
     4        https://bugs.webkit.org/show_bug.cgi?id=158395
     5        rdar://problem/7440637
     6
     7        Reviewed by Dan Bernstein and Darin Adler.
     8
     9        There is a long standing bug where in some cases WebKit may find non-visible text matches when doing find on page.
     10        For example searching patch review view in bugs.webkit.org returns twice as many matches as there actually are
     11        on the page. This happens because the text content is replicated in an invisible subframe.
     12
     13        Fix by making TextIterator ignore content in non-visible subframes in findPlainText.
     14
     15        Test: editing/text-iterator/count-matches-in-frames.html
     16
     17        * editing/TextIterator.cpp:
     18        (WebCore::nextInPreOrderCrossingShadowBoundaries):
     19
     20            Remove support for an uninteresting assertion.
     21
     22        (WebCore::fullyClipsContents):
     23
     24            Elements without renderer clip their content (except for display:contents).
     25            Test the content rect instead of the size rect for emptiness.
     26
     27        (WebCore::ignoresContainerClip):
     28        (WebCore::pushFullyClippedState):
     29        (WebCore::setUpFullyClippedStack):
     30        (WebCore::isClippedByFrameAncestor):
     31
     32            Test if the frame owner element is clipped in any of the parent frames.
     33
     34        (WebCore::TextIterator::TextIterator):
     35
     36            If the frame is clipped by its ancestors the iterator is initialized to end state.
     37            Clipped frame never renders anything so there is no need to maintain clip stack and traverse.
     38
     39        (WebCore::findPlainText):
     40
     41            Use TextIteratorClipsToFrameAncestors behavior. There might be other places where
     42            this behavior should be used (or perhaps it should be used always?) but limit this to
     43            text search for now.
     44
     45        (WebCore::depthCrossingShadowBoundaries): Deleted.
     46        * editing/TextIterator.h:
     47        * editing/TextIteratorBehavior.h:
     48
     49            Add TextIteratorClipsToFrameAncestors behavior.
     50
     51        * testing/Internals.cpp:
     52        (WebCore::Internals::countMatchesForText):
     53        (WebCore::Internals::countFindMatches):
     54        (WebCore::Internals::numberOfLiveNodes):
     55        * testing/Internals.h:
     56        * testing/Internals.idl:
     57
     58            Testing support
     59
    1602016-06-05  Konstantin Tokarev  <annulen@yandex.ru>
    261

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

    r201441 r201701  
    3434#include "HTMLBodyElement.h"
    3535#include "HTMLElement.h"
     36#include "HTMLFrameOwnerElement.h"
    3637#include "HTMLInputElement.h"
    3738#include "HTMLLegendElement.h"
    … …  
    186187// --------
    187188
    188 #if !ASSERT_DISABLED
    189 
    190 static unsigned depthCrossingShadowBoundaries(Node& node)
    191 {
    192     unsigned depth = 0;
    193     for (Node* parent = node.parentOrShadowHostNode(); parent; parent = parent->parentOrShadowHostNode())
    194         ++depth;
    195     return depth;
    196 }
    197 
    198 #endif
    199 
    200189// This function is like Range::pastLastNode, except for the fact that it can climb up out of shadow trees.
    201190static Node* nextInPreOrderCrossingShadowBoundaries(Node& rangeEndContainer, int rangeEndOffset)
    … …  
    215204{
    216205    auto* renderer = node.renderer();
    217     if (!is<RenderBox>(renderer) || !renderer->hasOverflowClip())
     206    if (!renderer) {
     207        if (!is<Element>(node))
     208            return false;
     209        return !downcast<Element>(node).hasDisplayContents();
     210    }
     211    if (!is<RenderBox>(*renderer))
    218212        return false;
    219     return downcast<RenderBox>(*renderer).size().isEmpty();
     213    auto& box = downcast<RenderBox>(*renderer);
     214    if (!box.hasOverflowClip())
     215        return false;
     216    return box.contentSize().isEmpty();
    220217}
    221218
    … …  
    230227static void pushFullyClippedState(BitStack& stack, Node& node)
    231228{
    232     ASSERT(stack.size() == depthCrossingShadowBoundaries(node));
    233 
    234229    // Push true if this node full clips its contents, or if a parent already has fully
    235230    // clipped and this is not a node that ignores its container's clip.
    … …  
    240235{
    241236    // Put the nodes in a vector so we can iterate in reverse order.
     237    // FIXME: This (and TextIterator in general) should use ComposedTreeIterator.
    242238    Vector<Node*, 100> ancestry;
    243239    for (Node* parent = node.parentOrShadowHostNode(); parent; parent = parent->parentOrShadowHostNode())
    … …  
    249245        pushFullyClippedState(stack, *ancestry[size - i - 1]);
    250246    pushFullyClippedState(stack, node);
    251 
    252     ASSERT(stack.size() == 1 + depthCrossingShadowBoundaries(node));
     247}
     248
     249static bool isClippedByFrameAncestor(const Document& document, TextIteratorBehavior behavior)
     250{
     251    if (!(behavior & TextIteratorClipsToFrameAncestors))
     252        return false;
     253
     254    for (auto* owner = document.ownerElement(); owner; owner = owner->document().ownerElement()) {
     255        BitStack ownerClipStack;
     256        setUpFullyClippedStack(ownerClipStack, *owner);
     257        if (ownerClipStack.top())
     258            return true;
     259    }
     260    return false;
    253261}
    254262
    … …  
    366374    if (!m_node)
    367375        return;
     376
     377    if (isClippedByFrameAncestor(m_node->document(), m_behavior))
     378        return;
     379
    368380    setUpFullyClippedStack(m_fullyClippedStack, *m_node);
     381
    369382    m_offset = m_node == m_startContainer ? m_startOffset : 0;
    370383
    371384    m_pastEndNode = nextInPreOrderCrossingShadowBoundaries(*m_endContainer, m_endOffset);
    372385
    373 #ifndef NDEBUG
    374     // Need this just because of the assert in advance().
    375386    m_positionNode = m_node;
    376 #endif
    377387
    378388    advance();
    … …  
    12241234    m_endOffset = endOffset;
    12251235   
    1226 #ifndef NDEBUG
    1227     // Need this just because of the assert.
    12281236    m_positionNode = endNode;
    1229 #endif
    12301237
    12311238    m_lastTextNode = nullptr;
    … …  
    26142621    }
    26152622
    2616     CharacterIterator findIterator(range, TextIteratorEntersTextControls);
     2623    CharacterIterator findIterator(range, TextIteratorEntersTextControls | TextIteratorClipsToFrameAncestors);
    26172624
    26182625    while (!findIterator.atEnd()) {
    … …  
    26532660
    26542661    // Then, find the document position of the start and the end of the text.
    2655     CharacterIterator computeRangeIterator(range, TextIteratorEntersTextControls);
     2662    CharacterIterator computeRangeIterator(range, TextIteratorEntersTextControls | TextIteratorClipsToFrameAncestors);
    26562663    return characterSubrange(range.ownerDocument(), computeRangeIterator, matchStart, matchLength);
    26572664}

  • trunk/Source/WebCore/editing/TextIteratorBehavior.h

    r200744 r201701  
    5555
    5656    TextIteratorBehavesAsIfNodesFollowing = 1 << 7,
     57
     58    // Makes visiblity test take into account the visibility of the frame.
     59    // FIXME: This should probably be always on unless TextIteratorIgnoresStyleVisibility is set.
     60    TextIteratorClipsToFrameAncestors = 1 << 8,
    5761};
    5862

  • trunk/Source/WebCore/rendering/RenderBox.h

    r201635 r201701  
    210210    void updateLayerTransform();
    211211
     212    LayoutSize contentSize() const { return { contentWidth(), contentHeight() }; }
    212213    LayoutUnit contentWidth() const { return clientWidth() - paddingLeft() - paddingRight(); }
    213214    LayoutUnit contentHeight() const { return clientHeight() - paddingTop() - paddingBottom(); }

  • trunk/Source/WebCore/testing/Internals.cpp

    r201422 r201701  
    17341734}
    17351735
     1736unsigned Internals::countFindMatches(const String& text, unsigned findOptions, ExceptionCode&)
     1737{
     1738    Document* document = contextDocument();
     1739    if (!document || !document->page())
     1740        return 0;
     1741
     1742    return document->page()->countFindMatches(text, findOptions, 1000);
     1743}
     1744
    17361745unsigned Internals::numberOfLiveNodes() const
    17371746{

  • trunk/Source/WebCore/testing/Internals.h

    r201422 r201701  
    225225
    226226    unsigned countMatchesForText(const String&, unsigned findOptions, const String& markMatches, ExceptionCode&);
     227    unsigned countFindMatches(const String&, unsigned findOptions, ExceptionCode&);
    227228
    228229    unsigned numberOfScrollableAreas(ExceptionCode&);

  • trunk/Source/WebCore/testing/Internals.idl

    r201422 r201701  
    162162    void setShowAutoFillButton(HTMLInputElement inputElement, AutoFillButtonType autoFillButtonType);
    163163    [RaisesException] unsigned long countMatchesForText(DOMString text, unsigned long findOptions, DOMString markMatches);
     164    [RaisesException] unsigned long countFindMatches(DOMString text, unsigned long findOptions);
    164165
    165166    [RaisesException] DOMString autofillFieldName(Element formControlElement);
Note: See TracChangeset for help on using the changeset viewer.