Bug 158395

Summary: Find on page finds too many matches
Product: WebKit Reporter: Antti Koivisto <koivisto>
Component: Layout and RenderingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, beidson, buildbot, darin, mitz, rniwa, simon.fraser
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=82101
Attachments:
Description Flags
patch
mitz: review+, buildbot: commit-queue-
Archive of layout-test-results from ews104 for mac-yosemite-wk2
none
Archive of layout-test-results from ews125 for ios-simulator-wk2
none
Archive of layout-test-results from ews103 for mac-yosemite
none
Archive of layout-test-results from ews116 for mac-yosemite
none
patch
none
patch none

Description Antti Koivisto 2016-06-05 07:10:16 PDT
WebKit may find non-visible text matches when doing find on page from browser UI. 

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.
Comment 1 Antti Koivisto 2016-06-05 07:10:55 PDT
rdar://problem/7440637
Comment 2 Antti Koivisto 2016-06-05 08:14:28 PDT
Created attachment 280547 [details]
patch
Comment 3 Brady Eidson 2016-06-05 08:48:15 PDT
👍👍👍
Comment 4 Build Bot 2016-06-05 09:06:04 PDT
Comment on attachment 280547 [details]
patch

Attachment 280547 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1447562

New failing tests:
imported/blink/fast/shapes/shape-outside-floats/shape-outside-negative-height-crash-width.html
Comment 5 Build Bot 2016-06-05 09:06:07 PDT
Created attachment 280551 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 6 Build Bot 2016-06-05 09:12:09 PDT
Comment on attachment 280547 [details]
patch

Attachment 280547 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1447563

New failing tests:
imported/blink/fast/shapes/shape-outside-floats/shape-outside-negative-height-crash-width.html
Comment 7 Build Bot 2016-06-05 09:12:12 PDT
Created attachment 280552 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.4
Comment 8 Darin Adler 2016-06-05 10:13:06 PDT
Comment on attachment 280547 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=280547&action=review

Wow, I am so excited about this one.

> Source/WebCore/editing/TextIterator.cpp:215
> +    return downcast<RenderBox>(*renderer).contentBoxRect().isEmpty();

It’s a shame that this has to compute the position of the content box, since the code doesn’t need it. The width and height are always just contentWidth() and contentHeight(). If there was a helper that just returned that we could have used it.

> Source/WebCore/editing/TextIterator.cpp:254
> +    auto* owner = document.ownerElement();
> +    while (owner) {

I’d like this better as a for loop:

    for (auto* owner = document.ownerElement(); owner; owner = owner->document().ownerElement()) {

> Source/WebCore/editing/TextIterator.cpp:1206
> +    , m_positionNode(nullptr)

I don’t think this is needed. The class definition already sets m_positionNode to nullptr.

> Source/WebCore/editing/TextIteratorBehavior.h:58
> +    // Makes visiblility test take into account the visibility of the frame.

Typo in the first use of the word "visibility".
Comment 9 Build Bot 2016-06-05 10:14:06 PDT
Comment on attachment 280547 [details]
patch

Attachment 280547 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1447823

New failing tests:
imported/blink/fast/shapes/shape-outside-floats/shape-outside-negative-height-crash-width.html
Comment 10 Build Bot 2016-06-05 10:14:09 PDT
Created attachment 280554 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 11 Build Bot 2016-06-05 10:46:21 PDT
Comment on attachment 280547 [details]
patch

Attachment 280547 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1447912

New failing tests:
imported/blink/fast/shapes/shape-outside-floats/shape-outside-negative-height-crash-width.html
Comment 12 Build Bot 2016-06-05 10:46:24 PDT
Created attachment 280555 [details]
Archive of layout-test-results from ews116 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 13 Antti Koivisto 2016-06-05 12:05:45 PDT
Created attachment 280557 [details]
patch
Comment 14 Antti Koivisto 2016-06-05 12:07:36 PDT
Created attachment 280558 [details]
patch
Comment 15 Antti Koivisto 2016-06-05 12:48:15 PDT
https://trac.webkit.org/r201701