Skip to content
Permalink
Browse files
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.



Canonical link: https://commits.webkit.org/176487@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@201701 268f45cc-cd09-0410-ab3c-d52691b4dbfc
  • Loading branch information
anttijk committed Jun 5, 2016
1 parent 0dff35e commit a94983b7bda196bd1eb3e933720b3a84007a4c22
Showing 11 changed files with 178 additions and 26 deletions.
@@ -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.
@@ -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

@@ -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>
@@ -1,3 +1,2 @@
Test passes if it does not crash.


@@ -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.
@@ -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);
}

@@ -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;
@@ -209,6 +209,7 @@ class RenderBox : public RenderBoxModelObject {

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(); }
@@ -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;
@@ -224,6 +224,7 @@ class Internals final : public RefCounted<Internals>, private ContextDestruction
void toggleOverwriteModeEnabled(ExceptionCode&);

unsigned countMatchesForText(const String&, unsigned findOptions, const String& markMatches, ExceptionCode&);
unsigned countFindMatches(const String&, unsigned findOptions, ExceptionCode&);

unsigned numberOfScrollableAreas(ExceptionCode&);

@@ -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);

0 comments on commit a94983b

Please sign in to comment.