1407987 - Text search in scroll box limited to visible region

Status: RESOLVED FIXED

(Toolkit :: Find Toolbar, defect, P1)

Description

[Finn Bryant]

Attachment: Simple webpage with long scrollbox to search through

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:58.0) Gecko/20100101 Firefox/58.0
Build ID: 20171011220113

Steps to reproduce:

Attempt to search for text "findthis" within attached example.
Click the arrows on the findbar to cycle through the different uses.

Also attempted on a clean profile to confirm real issue. 
Reproduced on Nightly 58.0a1 (2017-10-11) (64-bit) MacOS build. 
*Doesn't* reproduce on current v56 release.


Actual results:

Could only step through results within the visible region of the scroll box. The total number of results shown on the find bar was correct however.


Expected results:

Should have cycled through all results.

Comment 1

[Finn Bryant]

I said above that the total number of results was correct, but I've now seen that if the phrase doesn't exist in either the currently visible secion *or* the first screen's worth of output, it will show "Phrase not found" instead.

Comment 2

[Mike de Boer [:mikedeboer]]

Thanks for filing this!

Comment 3

[Mike de Boer [:mikedeboer]]

Brad, this is a bit of fallout from bug 1302470... so the internal methods in nsTypeAheadFind that use IsRangeVisible() need to be able to find occurrences that are outside of the currently visible viewport.

Do you have an idea how to fix this, perhaps or do you want me to take a look instead?

Comment 5

[Brad Werth [:bradwerth]]

The fourth argument of IsRangeVisible is aMustBeInViewport. If this value is set to false, then you'll get the old (desired) behavior. The callsite I can find at http://searchfox.org/mozilla-central/rev/ed1d5223adcdc07e9a2589ee20f4e5ee91b4df10/toolkit/components/typeaheadfind/nsTypeAheadFind.cpp#465 is passing a variable called aIsFirstVisiblePreferred as the fourth argument. Semantically, this isn't really the same thing as aMustBeInViewport. I'll do some debugging to see if I can understand the calling pattern and propose a fix.

Comment 6

[Brad Werth [:bradwerth]]

Hmmm, arguments are fine. There's a problem with the algorithm in IsRangeVisible. I'll post a patch.

Comment 7

[Brad Werth [:bradwerth]]

Attachment: Bug 1407987 Part 1: Change isRangeVisible to only use isRangeRendered for text within the viewport.

Review commit: https://reviewboard.mozilla.org/r/188934/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/188934/

Comment 8

[Brad Werth [:bradwerth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=02e276fc73408799bc29f9a5d2e6c005a6e84704

Comment 9

[Brad Werth [:bradwerth]]

Attachment 8918037 [details] fixes this in a simplistic way. Running tests to see if it breaks anything else.

Comment 10

[Brad Werth [:bradwerth]]

Attachment: Bug 1407987 Part 2: Add a test of finding offscreen text.

Review commit: https://reviewboard.mozilla.org/r/188960/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/188960/

Comment 11

[Brad Werth [:bradwerth]]

Comment on attachment 8918037 [details]
Bug 1407987 Part 1: Change isRangeVisible to only use isRangeRendered for text within the viewport.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/188934/diff/1-2/

Comment 12

[Brad Werth [:bradwerth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=774eda758ec8f7e88fd5648f10ff28ac80f60eca

Comment 13

[Mike de Boer [:mikedeboer]]

Comment on attachment 8918037 [details]
Bug 1407987 Part 1: Change isRangeVisible to only use isRangeRendered for text within the viewport.

https://reviewboard.mozilla.org/r/188934/#review194378

::: toolkit/components/typeaheadfind/nsTypeAheadFind.cpp:1272
(Diff revision 2)
>        aPresShell->GetRectVisibility(frame,
>                                      nsRect(nsPoint(0,0), frame->GetSize()),
>                                      minDistance);
>  
> -    if (rectVisibility != nsRectVisibility_kAboveViewport) {
> +    if (rectVisibility == nsRectVisibility_kVisible) {
>        // This is an early exit case, where we return true iff the range

Oooh, that's subtle. While you're here, can you correct the comment below? s/iff/if/

Comment 14

[Mike de Boer [:mikedeboer]]

Comment on attachment 8918063 [details]
Bug 1407987 Part 2: Add a test of finding offscreen text.

https://reviewboard.mozilla.org/r/188960/#review194380

Please take a look at my suggestion below. It's important to change your try push to include 'mochitest-bc' and 'mochitest-bc-e10s', because those will actually _run_ this test ;-)

::: toolkit/modules/tests/browser/browser_Finder_offscreen_text.js:1
(Diff revision 1)
> +/* This Source Code Form is subject to the terms of the Mozilla Public

nit: Please change the license to CC - (see  https://www.mozilla.org/en-US/MPL/headers/)

::: toolkit/modules/tests/browser/browser_Finder_offscreen_text.js:22
(Diff revision 1)
> +    if(t < targetCount && linesToInsertTargetText[t] == i) {
> +      URI += TARGET_TEXT;
> +      t++;
> +    }
> +  }
> +  URI += "</body>";

This'll work, it's also possible to make the test look similar to http://searchfox.org/mozilla-central/source/browser/base/content/test/about/browser_aboutHome_wrapsCorrectly.js, so that it's ensured to go offscreen on any machine.

Please don't copy the license header from there, it's incorrect ;-)

Comment 15

[Mike de Boer [:mikedeboer]]

And thanks for jumping on this so quickly, Brad!

Comment 17

[Brad Werth [:bradwerth]]

Comment on attachment 8918037 [details]
Bug 1407987 Part 1: Change isRangeVisible to only use isRangeRendered for text within the viewport.

https://reviewboard.mozilla.org/r/188934/#review194486

::: toolkit/components/typeaheadfind/nsTypeAheadFind.cpp:1272
(Diff revision 2)
>        aPresShell->GetRectVisibility(frame,
>                                      nsRect(nsPoint(0,0), frame->GetSize()),
>                                      minDistance);
>  
> -    if (rectVisibility != nsRectVisibility_kAboveViewport) {
> +    if (rectVisibility == nsRectVisibility_kVisible) {
>        // This is an early exit case, where we return true iff the range

Sure. I have seen "iff" sometimes in comments to mean "if and only if", and that was my intended use. I've expanded it here.

Comment 18

[Brad Werth [:bradwerth]]

Comment on attachment 8918063 [details]
Bug 1407987 Part 2: Add a test of finding offscreen text.

https://reviewboard.mozilla.org/r/188960/#review194512

::: toolkit/modules/tests/browser/browser_Finder_offscreen_text.js:22
(Diff revision 1)
> +    if(t < targetCount && linesToInsertTargetText[t] == i) {
> +      URI += TARGET_TEXT;
> +      t++;
> +    }
> +  }
> +  URI += "</body>";

Since it appears to work, I'll leave it as it is. The test framework has some fixed size for testing, and the gap between targets 2 and 3 (100 lines) is large enough to span whatever window height is used or simulated.

Comment 19

[Brad Werth [:bradwerth]]

Comment on attachment 8918037 [details]
Bug 1407987 Part 1: Change isRangeVisible to only use isRangeRendered for text within the viewport.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/188934/diff/2-3/

Comment 20

[Brad Werth [:bradwerth]]

Comment on attachment 8918063 [details]
Bug 1407987 Part 2: Add a test of finding offscreen text.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/188960/diff/1-2/

Comment 21

[Brad Werth [:bradwerth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1ad16ae291a08f4e67a300e2da67e783598cd7d9

Comment 22

[Josh Gaunt [:jgaunt]]

This looks like the same thing that has been harming my UX on Nightly reliably for both my personal and moz laptops.

That is, I can only CTRL-F for text that is visible on the screen; if I scroll a unique string as much as 1 line off screen it will no longer be found/highlighted.

*Of note, though; it doesn't seem to affect all sites equally.*

I have had trouble searching for specific transactions at my bank's website (CTRL-F for 10.18 fails until I scroll to where the transaction took place, defeating the purpose of the find).

Also, I have had this trouble searching for strings in the Raw Data view of the Raw JSON data from about:telemetry.

An exception I noticed, though; this site for both laptops CTRL-F works as expected: http://www.meterplugs.com/perception

Maybe you all already fixed this and the changes haven't landed yet... The preceding discussion gets a bit esoteric for me :)

Comment 23

[Mike de Boer [:mikedeboer]]

Well, more data points are good for Brad to verify that his fix is doing the right thing in those circumstances too. So thanks for mentioning ;-)

Comment 24

[Brad Werth [:bradwerth]]

Attachment: Bug 1407987 Part 3: Disable browser_visibleFindSelection.js for failing to automate properly.

Review commit: https://reviewboard.mozilla.org/r/189962/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/189962/

Comment 25

[Brad Werth [:bradwerth]]

Comment on attachment 8918037 [details]
Bug 1407987 Part 1: Change isRangeVisible to only use isRangeRendered for text within the viewport.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/188934/diff/3-4/

Comment 26

[Brad Werth [:bradwerth]]

Comment on attachment 8918063 [details]
Bug 1407987 Part 2: Add a test of finding offscreen text.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/188960/diff/2-3/

Comment 27

[Brad Werth [:bradwerth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2eb830a1de5d8922f41c2afe3c743f6e47c2592f

Comment 28

[Mike de Boer [:mikedeboer]]

Comment on attachment 8919049 [details]
Bug 1407987 Part 3: Disable browser_visibleFindSelection.js for failing to automate properly.

https://reviewboard.mozilla.org/r/189962/#review195350

I'll take a look at why the test stopped working in bug 1409184. Did you test that this patch also fixes the cases reported and closed as duplicates here?

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ed9097c98efa
Part 1: Change isRangeVisible to only use isRangeRendered for text within the viewport. r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/fb0a06ceed77
Part 2: Add a test of finding offscreen text. r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/3464e112b7d4
Part 3: Disable browser_visibleFindSelection.js for failing to automate properly. r=mikedeboer

Comment 30

[Brad Werth [:bradwerth]]

Comment on attachment 8919049 [details]
Bug 1407987 Part 3: Disable browser_visibleFindSelection.js for failing to automate properly.

https://reviewboard.mozilla.org/r/189962/#review195350

I did not test the duplicate bugs. As described, they all should be resolved by this since find text is either above or to the side of the viewport. Prior to this landing, only visible text and text below the viewport would be found.

Comment 32

[Brad Werth [:bradwerth]]

Comment on attachment 8918037 [details]
Bug 1407987 Part 1: Change isRangeVisible to only use isRangeRendered for text within the viewport.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/188934/diff/4-5/

Comment 33

[Brad Werth [:bradwerth]]

Comment on attachment 8918063 [details]
Bug 1407987 Part 2: Add a test of finding offscreen text.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/188960/diff/3-4/

Comment 34

[Brad Werth [:bradwerth]]

Comment on attachment 8919049 [details]
Bug 1407987 Part 3: Disable browser_visibleFindSelection.js for failing to automate properly.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/189962/diff/1-2/

Comment 35

[Brad Werth [:bradwerth]]

Comment on attachment 8918063 [details]
Bug 1407987 Part 2: Add a test of finding offscreen text.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/188960/diff/4-5/

Comment 36

[Brad Werth [:bradwerth]]

Comment on attachment 8919049 [details]
Bug 1407987 Part 3: Disable browser_visibleFindSelection.js for failing to automate properly.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/189962/diff/2-3/

Comment 37

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 9addc94c3252 -d ef42d6855193: rebasing 427036:9addc94c3252 "Bug 1407987 Part 1: Change isRangeVisible to only use isRangeRendered for text within the viewport. r=mikedeboer"
note: rebase of 427036:9addc94c3252 created no changes to commit
rebasing 427037:3efd933d6f43 "Bug 1407987 Part 2: Add a test of finding offscreen text. r=mikedeboer"
merging toolkit/modules/tests/browser/browser_Finder_offscreen_text.js
warning: conflicts while merging toolkit/modules/tests/browser/browser_Finder_offscreen_text.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 38

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for eslint failure at browser/extensions/onboarding/bootstrap.js:159: Expected space(s) after "catch":

https://hg.mozilla.org/integration/autoland/rev/26268962cf5e333759a624b2fc7f4c2d765b99f8
https://hg.mozilla.org/integration/autoland/rev/16b6599eb2e51d23344ea73e38fe1c00b5d4f9e1
https://hg.mozilla.org/integration/autoland/rev/8d369c23c5b5f35ff863a60052ed41492ba4d02a

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=137601899&repo=autoland

TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/toolkit/modules/tests/browser/browser_Finder_offscreen_text.js:15:3 | Expected space(s) after "for". (keyword-spacing)
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/toolkit/modules/tests/browser/browser_Finder_offscreen_text.js:17:5 | Expected space(s) after "if". (keyword-spacing)
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/toolkit/modules/tests/browser/browser_Finder_offscreen_text.js:41:7 | Expected space(s) after "for". (keyword-spacing)
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/toolkit/modules/tests/browser/browser_Finder_offscreen_text.js:43:9 | Expected space(s) after "if". (keyword-spacing)

Comment 39

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bec74ea19545
Part 1: Change isRangeVisible to only use isRangeRendered for text within the viewport. r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/bca102a85314
Part 2: Add a test of finding offscreen text. r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/5e7b8f9471a4
Part 3: Disable browser_visibleFindSelection.js for failing to automate properly. r=mikedeboer

Comment 40

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/bec74ea19545
https://hg.mozilla.org/mozilla-central/rev/bca102a85314
https://hg.mozilla.org/mozilla-central/rev/5e7b8f9471a4

Comment 42

[Nicholas Nethercote [inactive]]

My JSON search case (bug 1408292, which was dup'd to this bug) is fixed in the latest Nightly. Thank you!