Changeset 201701 in webkit

Timestamp:

Jun 5, 2016 12:48:26 PM (8 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:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/editing/text-iterator/count-matches-in-frames-expected.txt (added)
• LayoutTests/editing/text-iterator/count-matches-in-frames.html (added)
• LayoutTests/imported/blink/fast/shapes/shape-outside-floats/shape-outside-negative-height-crash-width-expected.txt (modified) (1 diff)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/editing/TextIterator.cpp (modified) (10 diffs)
• Source/WebCore/editing/TextIteratorBehavior.h (modified) (1 diff)
• Source/WebCore/rendering/RenderBox.h (modified) (1 diff)
• Source/WebCore/testing/Internals.cpp (modified) (1 diff)
• Source/WebCore/testing/Internals.h (modified) (1 diff)
• Source/WebCore/testing/Internals.idl (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2016-06-05  Antti Koivisto  <antti@apple.com>
+
+        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.
+
 2016-06-04  Brady Eidson  <beidson@apple.com>
 

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

@@ -1 +1 @@
 Test passes if it does not crash.
 
-

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2016-06-05  Antti Koivisto  <antti@apple.com>
+
+        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
+
 2016-06-05  Konstantin Tokarev  <annulen@yandex.ru>
 

trunk/Source/WebCore/editing/TextIterator.cpp

@@ -34 +34 @@
 #include "HTMLBodyElement.h"
 #include "HTMLElement.h"
+#include "HTMLFrameOwnerElement.h"
 #include "HTMLInputElement.h"
 #include "HTMLLegendElement.h"
@@ -186 +187 @@
 // --------
 
-#if !ASSERT_DISABLED
-
-static unsigned depthCrossingShadowBoundaries(Node& node)
-{
-    unsigned depth = 0;
-    for (Node* parent = node.parentOrShadowHostNode(); parent; parent = parent->parentOrShadowHostNode())
-        ++depth;
-    return depth;
-}
-
-#endif
-
 // This function is like Range::pastLastNode, except for the fact that it can climb up out of shadow trees.
 static Node* nextInPreOrderCrossingShadowBoundaries(Node& rangeEndContainer, int rangeEndOffset)
@@ -215 +204 @@
 {
     auto* renderer = node.renderer();
-    if (!is<RenderBox>(renderer) || !renderer->hasOverflowClip())
+    if (!renderer) {
+        if (!is<Element>(node))
+            return false;
+        return !downcast<Element>(node).hasDisplayContents();
+    }
+    if (!is<RenderBox>(*renderer))
         return false;
-    return downcast<RenderBox>(*renderer).size().isEmpty();
+    auto& box = downcast<RenderBox>(*renderer);
+    if (!box.hasOverflowClip())
+        return false;
+    return box.contentSize().isEmpty();
 }
 
@@ -230 +227 @@
 static void pushFullyClippedState(BitStack& stack, Node& node)
 {
-    ASSERT(stack.size() == depthCrossingShadowBoundaries(node));
-
     // Push true if this node full clips its contents, or if a parent already has fully
     // clipped and this is not a node that ignores its container's clip.
@@ -240 +235 @@
 {
     // Put the nodes in a vector so we can iterate in reverse order.
+    // FIXME: This (and TextIterator in general) should use ComposedTreeIterator.
     Vector<Node*, 100> ancestry;
     for (Node* parent = node.parentOrShadowHostNode(); parent; parent = parent->parentOrShadowHostNode())
@@ -249 +245 @@
         pushFullyClippedState(stack, *ancestry[size - i - 1]);
     pushFullyClippedState(stack, node);
-
-    ASSERT(stack.size() == 1 + depthCrossingShadowBoundaries(node));
+}
+
+static bool isClippedByFrameAncestor(const Document& document, TextIteratorBehavior behavior)
+{
+    if (!(behavior & TextIteratorClipsToFrameAncestors))
+        return false;
+
+    for (auto* owner = document.ownerElement(); owner; owner = owner->document().ownerElement()) {
+        BitStack ownerClipStack;
+        setUpFullyClippedStack(ownerClipStack, *owner);
+        if (ownerClipStack.top())
+            return true;
+    }
+    return false;
 }
 
@@ -366 +374 @@
     if (!m_node)
         return;
+
+    if (isClippedByFrameAncestor(m_node->document(), m_behavior))
+        return;
+
     setUpFullyClippedStack(m_fullyClippedStack, *m_node);
+
     m_offset = m_node == m_startContainer ? m_startOffset : 0;
 
     m_pastEndNode = nextInPreOrderCrossingShadowBoundaries(*m_endContainer, m_endOffset);
 
-#ifndef NDEBUG
-    // Need this just because of the assert in advance().
     m_positionNode = m_node;
-#endif
 
     advance();
@@ -1224 +1234 @@
     m_endOffset = endOffset;
     
-#ifndef NDEBUG
-    // Need this just because of the assert.
     m_positionNode = endNode;
-#endif
 
     m_lastTextNode = nullptr;
@@ -2614 +2621 @@
     }
 
-    CharacterIterator findIterator(range, TextIteratorEntersTextControls);
+    CharacterIterator findIterator(range, TextIteratorEntersTextControls | TextIteratorClipsToFrameAncestors);
 
     while (!findIterator.atEnd()) {
@@ -2653 +2660 @@
 
     // Then, find the document position of the start and the end of the text.
-    CharacterIterator computeRangeIterator(range, TextIteratorEntersTextControls);
+    CharacterIterator computeRangeIterator(range, TextIteratorEntersTextControls | TextIteratorClipsToFrameAncestors);
     return characterSubrange(range.ownerDocument(), computeRangeIterator, matchStart, matchLength);
 }

trunk/Source/WebCore/editing/TextIteratorBehavior.h

@@ -55 +55 @@
 
     TextIteratorBehavesAsIfNodesFollowing = 1 << 7,
+
+    // Makes visiblity test take into account the visibility of the frame.
+    // FIXME: This should probably be always on unless TextIteratorIgnoresStyleVisibility is set.
+    TextIteratorClipsToFrameAncestors = 1 << 8,
 };
 

trunk/Source/WebCore/rendering/RenderBox.h

@@ -210 +210 @@
     void updateLayerTransform();
 
+    LayoutSize contentSize() const { return { contentWidth(), contentHeight() }; }
     LayoutUnit contentWidth() const { return clientWidth() - paddingLeft() - paddingRight(); }
     LayoutUnit contentHeight() const { return clientHeight() - paddingTop() - paddingBottom(); }

trunk/Source/WebCore/testing/Internals.cpp

@@ -1734 +1734 @@
 }
 
+unsigned Internals::countFindMatches(const String& text, unsigned findOptions, ExceptionCode&)
+{
+    Document* document = contextDocument();
+    if (!document || !document->page())
+        return 0;
+
+    return document->page()->countFindMatches(text, findOptions, 1000);
+}
+
 unsigned Internals::numberOfLiveNodes() const
 {

trunk/Source/WebCore/testing/Internals.h

@@ -225 +225 @@
 
     unsigned countMatchesForText(const String&, unsigned findOptions, const String& markMatches, ExceptionCode&);
+    unsigned countFindMatches(const String&, unsigned findOptions, ExceptionCode&);
 
     unsigned numberOfScrollableAreas(ExceptionCode&);

trunk/Source/WebCore/testing/Internals.idl

@@ -162 +162 @@
     void setShowAutoFillButton(HTMLInputElement inputElement, AutoFillButtonType autoFillButtonType);
     [RaisesException] unsigned long countMatchesForText(DOMString text, unsigned long findOptions, DOMString markMatches);
+    [RaisesException] unsigned long countFindMatches(DOMString text, unsigned long findOptions);
 
     [RaisesException] DOMString autofillFieldName(Element formControlElement);