Source/WebCore:
authorantti@apple.com <antti@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 5 Jun 2016 19:48:26 +0000 (19:48 +0000)
committerantti@apple.com <antti@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 5 Jun 2016 19:48:26 +0000 (19:48 +0000)
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.

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@201701 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/editing/text-iterator/count-matches-in-frames-expected.txt [new file with mode: 0644]
LayoutTests/editing/text-iterator/count-matches-in-frames.html [new file with mode: 0644]
LayoutTests/imported/blink/fast/shapes/shape-outside-floats/shape-outside-negative-height-crash-width-expected.txt
Source/WebCore/ChangeLog
Source/WebCore/editing/TextIterator.cpp
Source/WebCore/editing/TextIteratorBehavior.h
Source/WebCore/rendering/RenderBox.h
Source/WebCore/testing/Internals.cpp
Source/WebCore/testing/Internals.h
Source/WebCore/testing/Internals.idl

index 7ec2234..5a36395 100644 (file)
@@ -1,3 +1,14 @@
+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>
 
         Modern IDB: Add -private.html variants of crypto/subtle IndexedDB tests.
diff --git a/LayoutTests/editing/text-iterator/count-matches-in-frames-expected.txt b/LayoutTests/editing/text-iterator/count-matches-in-frames-expected.txt
new file mode 100644 (file)
index 0000000..2cf52dd
--- /dev/null
@@ -0,0 +1,12 @@
+findme0
+
+findme1
+findme2
+
+findme3
+
+PASS Search from frame in normal tree 
+PASS Search from frame in display:none subtree 
+PASS Search from frame in zero sized subtree 
+PASS Search from frame in zero sized subtree with overflow:hidden 
+
diff --git a/LayoutTests/editing/text-iterator/count-matches-in-frames.html b/LayoutTests/editing/text-iterator/count-matches-in-frames.html
new file mode 100644 (file)
index 0000000..df25413
--- /dev/null
@@ -0,0 +1,48 @@
+<!DOCTYPE html>
+<html>
+<head>
+<title>Text search from frames</title>
+<script src="../../resources/testharness.js"></script>
+<script src="../../resources/testharnessreport.js"></script>
+<link rel='stylesheet' href='../../resources/testharness.css'>
+</head>
+<body>
+
+<script>
+var count = 0;
+function findInFrame(frameHostStyle, shouldFindInFrame)
+{
+    var findString = "findme" + count++;
+    var textDiv = document.createElement("div");
+    textDiv.innerHTML = findString;
+    document.body.appendChild(textDiv)
+
+    var hostDiv = document.createElement("div");
+    hostDiv.setAttribute("style", frameHostStyle);
+    var frame = document.createElement("iframe");
+    hostDiv.appendChild(frame);
+    document.body.appendChild(hostDiv);
+
+    frame.contentDocument.body.innerHTML = findString;
+
+    assert_equals(internals.countFindMatches(findString, 0, ''), shouldFindInFrame ? 2 : 1);
+}
+
+test(function () {
+    findInFrame("", true);
+}, "Search from frame in normal tree");
+
+test(function () {
+    findInFrame("display:none", false);
+}, "Search from frame in display:none subtree");
+
+test(function () {
+    findInFrame("position:relative; width:0px; height:0px", true);
+}, "Search from frame in zero sized subtree");
+
+test(function () {
+    findInFrame("position:relative; width:0px; height:0px; border: 2px solid red; overflow:hidden", false);
+}, "Search from frame in zero sized subtree with overflow:hidden");
+
+</script>
+</html>
index 1d93ab7..9c5a8db 100644 (file)
@@ -1,3 +1,62 @@
+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>
 
         Do not construct temporary copy of String from AtomicString.
index 2dff042..cb74af9 100644 (file)
@@ -33,6 +33,7 @@
 #include "Frame.h"
 #include "HTMLBodyElement.h"
 #include "HTMLElement.h"
+#include "HTMLFrameOwnerElement.h"
 #include "HTMLInputElement.h"
 #include "HTMLLegendElement.h"
 #include "HTMLMeterElement.h"
@@ -185,18 +186,6 @@ unsigned BitStack::size() const
 
 // --------
 
-#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)
 {
@@ -214,9 +203,17 @@ static Node* nextInPreOrderCrossingShadowBoundaries(Node& rangeEndContainer, int
 static inline bool fullyClipsContents(Node& node)
 {
     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;
+    auto& box = downcast<RenderBox>(*renderer);
+    if (!box.hasOverflowClip())
         return false;
-    return downcast<RenderBox>(*renderer).size().isEmpty();
+    return box.contentSize().isEmpty();
 }
 
 static inline bool ignoresContainerClip(Node& node)
@@ -229,8 +226,6 @@ static inline bool ignoresContainerClip(Node& node)
 
 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.
     stack.push(fullyClipsContents(node) || (stack.top() && !ignoresContainerClip(node)));
@@ -239,6 +234,7 @@ static void pushFullyClippedState(BitStack& stack, Node& node)
 static void setUpFullyClippedStack(BitStack& stack, Node& node)
 {
     // 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())
         ancestry.append(parent);
@@ -248,8 +244,20 @@ static void setUpFullyClippedStack(BitStack& stack, Node& node)
     for (size_t i = 0; i < size; ++i)
         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;
 }
 
 // FIXME: editingIgnoresContent and isRendererReplacedElement try to do the same job.
@@ -365,15 +373,17 @@ TextIterator::TextIterator(const Range* range, TextIteratorBehavior behavior)
     m_node = range->firstNode();
     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();
 }
@@ -1223,10 +1233,7 @@ SimplifiedBackwardsTextIterator::SimplifiedBackwardsTextIterator(const Range& ra
     m_endContainer = endNode;
     m_endOffset = endOffset;
     
-#ifndef NDEBUG
-    // Need this just because of the assert.
     m_positionNode = endNode;
-#endif
 
     m_lastTextNode = nullptr;
     m_lastCharacter = '\n';
@@ -2613,7 +2620,7 @@ static size_t findPlainText(const Range& range, const String& target, FindOption
         }
     }
 
-    CharacterIterator findIterator(range, TextIteratorEntersTextControls);
+    CharacterIterator findIterator(range, TextIteratorEntersTextControls | TextIteratorClipsToFrameAncestors);
 
     while (!findIterator.atEnd()) {
         findIterator.advance(buffer.append(findIterator.text()));
@@ -2652,7 +2659,7 @@ Ref<Range> findPlainText(const Range& range, const String& target, FindOptions o
     }
 
     // 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);
 }
 
index 7cc19eb..855e9fc 100644 (file)
@@ -54,6 +54,10 @@ enum TextIteratorBehaviorFlag {
     TextIteratorEmitsImageAltText = 1 << 6,
 
     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,
 };
 
 typedef unsigned short TextIteratorBehavior;
index c16e75c..e869414 100644 (file)
@@ -209,6 +209,7 @@ public:
     
     void updateLayerTransform();
 
+    LayoutSize contentSize() const { return { contentWidth(), contentHeight() }; }
     LayoutUnit contentWidth() const { return clientWidth() - paddingLeft() - paddingRight(); }
     LayoutUnit contentHeight() const { return clientHeight() - paddingTop() - paddingBottom(); }
     LayoutUnit contentLogicalWidth() const { return style().isHorizontalWritingMode() ? contentWidth() : contentHeight(); }
index 983f002..9608ccd 100644 (file)
@@ -1733,6 +1733,15 @@ unsigned Internals::countMatchesForText(const String& text, unsigned findOptions
     return document->frame()->editor().countMatchesForText(text, nullptr, findOptions, 1000, mark, nullptr);
 }
 
+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
 {
     unsigned nodeCount = 0;
index 5613d9a..3ec5ac7 100644 (file)
@@ -224,6 +224,7 @@ public:
     void toggleOverwriteModeEnabled(ExceptionCode&);
 
     unsigned countMatchesForText(const String&, unsigned findOptions, const String& markMatches, ExceptionCode&);
+    unsigned countFindMatches(const String&, unsigned findOptions, ExceptionCode&);
 
     unsigned numberOfScrollableAreas(ExceptionCode&);
 
index 36cb1bd..547a058 100644 (file)
@@ -161,6 +161,7 @@ enum AutoFillButtonType {
     void setAutofilled(HTMLInputElement inputElement, boolean enabled);
     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);