Closed Bug 1407987 Opened 2 years ago Closed 2 years ago

Text search in scroll box limited to visible region

Categories

(Toolkit :: Find Toolbar, defect, P1)

58 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: finnbryant, Assigned: bradwerth)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

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.
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.
Component: Untriaged → Find Toolbar
Product: Firefox → Toolkit
Thanks for filing this!
Assignee: nobody → mdeboer
Blocks: 1302470
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Priority: -- → P1
Assignee: mdeboer → nobody
Status: ASSIGNED → NEW
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?
Flags: needinfo?(bwerth)
Duplicate of this bug: 1407772
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.
Hmmm, arguments are fine. There's a problem with the algorithm in IsRangeVisible. I'll post a patch.
Attachment 8918037 [details] fixes this in a simplistic way. Running tests to see if it breaks anything else.
Flags: needinfo?(bwerth)
Assignee: nobody → bwerth
Attachment #8918037 - Flags: review?(mdeboer)
Attachment #8918063 - Flags: review?(mdeboer)
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/
Attachment #8918037 - Flags: review?(mdeboer) → review+
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 ;-)
Attachment #8918063 - Flags: review?(mdeboer) → review+
And thanks for jumping on this so quickly, Brad!
Duplicate of this bug: 1408292
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 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.
Status: NEW → ASSIGNED
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 :)
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 ;-)
Blocks: 1409184
Attachment #8919049 - Flags: review?(mdeboer)
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?
Attachment #8919049 - Flags: review?(mdeboer) → review+
Pushed by bwerth@mozilla.com:
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 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.
Duplicate of this bug: 1408661
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)
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)
Flags: needinfo?(bwerth)
Pushed by bwerth@mozilla.com:
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
Duplicate of this bug: 1409763
Flags: needinfo?(bwerth)
My JSON search case (bug 1408292, which was dup'd to this bug) is fixed in the latest Nightly. Thank you!
Depends on: 1435589
You need to log in before you can comment on or make changes to this bug.