Closed Bug 1436431 Opened 2 years ago Closed 2 years ago

Typeahead find won't scroll to overflow text in a block formatting context if text frame is within the root scroll pane

Categories

(Core :: Find Backend, defect)

58 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: johnmaverick74, Assigned: bradwerth)

References

Details

(Keywords: testcase)

Attachments

(5 files)

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:58.0) Gecko/20100101 Firefox/58.0
Build ID: 20180128191252

Steps to reproduce:

Using a PHP SaaS website where i get around 200 RMA entries listed (not all visible at the same time - the page has a scroll).
I enable "search for text when you start typing"  (this also happens if i use CTRL+F ) and searched for 20 of those entries


Actual results:

Some of the searched strings were not found!
However, if i scroll thru the list i am able to see them listed!!!!

Once they are "in plain sight", if i searched for them again they are found successfully!!!


Expected results:

Upon search, "Find" should be able to locate any of the strings, since THEY ARE PRESENT in the list (even if they are way up or way down the list)
Please provide a link where the issue can be observed.
Has Regression Range: --- → irrelevant
Has STR: --- → no
Component: Untriaged → Find Toolbar
Flags: needinfo?(johnmaverick74)
Product: Firefox → Toolkit
(In reply to Gingerbread Man from comment #1)
> Please provide a link where the issue can be observed.

Since we are using internal software I'll have to try to find a public alternative.

VERY IMPORTANT: I forgot to mention that this happens using a barcode Scanner (i couldn't verify if this happens when typing manually).
(In reply to Gingerbread Man from comment #1)
> Please provide a link where the issue can be observed.

In the meanwhile let me ask one thing that might solve the bug: Is there a place in Firefox where one can set up the cache/space a query can use, or something like it?

I ask this because i always get the impression that Firefox has a "limit" of lines/results a query can hold in memory. It seems that if we scroll up/down it will load those missing/not in memory results that were not visible and after that it's able to find them.
Flags: needinfo?(mdeboer)
Flags: needinfo?(gingerbread_man)
Sorry, I don't know if that's the case.
Flags: needinfo?(gingerbread_man)
2 questions:

Would a video help validating the bug?

If yes, is there a way to upload the video "privately" (only visible to devs)?

(it's from our internal software so, no public view allowed!)
I found a way that i may provide evaluation of the bug (by saving the page of the system i'm working), however i _really_ need that to be private (e.g.: available only for developer that may solve it).

I'll provide the html plus an ods file (for barcode generation) in zip format where the bug can be observed (didn't find an online page).

Please send instructions on how to proceed
Flags: needinfo?(johnmaverick74) → needinfo?(gingerbread_man)
Once a developer expresses interest in working on this in a comment here, you can hover over the name to see the e-mail address.

(At best I could attempt to reproduce the issue on Windows)
Flags: needinfo?(gingerbread_man)
(In reply to Gingerbread Man from comment #7)
> Once a developer expresses interest in working on this in a comment here,
> you can hover over the name to see the e-mail address.
> 
> (At best I could attempt to reproduce the issue on Windows)

I would appreciate that!

How can i send you the files (privately)?

Thanks
(In reply to Maverick from comment #8)
> How can i send you the files (privately)?

Like I said at comment 7, you can just click someone's name at the top of the comment box to send an e-mail. I also e-mailed you directly when you posted comment 8, but haven't heard back.
(In reply to Gingerbread Man from comment #9)
> (In reply to Maverick from comment #8)
> > How can i send you the files (privately)?
> 
> Like I said at comment 7, you can just click someone's name at the top of
> the comment box to send an e-mail. I also e-mailed you directly when you
> posted comment 8, but haven't heard back.

I've replied your e-mail last Friday. Can you please check if your received it?

If not i'll resend it! 

I'm sorry for taking so much time... I've been rushing everywhere and was not able to reply you sooner.

tks
Flags: needinfo?(mdeboer)
(In reply to Maverick from comment #10)
> I've replied your e-mail last Friday. Can you please check if your received
> it?

I haven't received it. I've added you to the safe sender list, though I don't know if that'll help when the message never reaches my account in any fashion.
(In reply to Gingerbread Man from comment #11)
> (In reply to Maverick from comment #10)
> > I've replied your e-mail last Friday. Can you please check if your received
> > it?
> 
> I haven't received it. I've added you to the safe sender list, though I
> don't know if that'll help when the message never reaches my account in any
> fashion.

Resent it just now (has a Zip attached with everything in it). 
If it still does not arrive we have to find another way around.
@Maverick: Can you see if you can reproduce https://bugzilla.mozilla.org/show_bug.cgi?id=1443249? Maybe it's the same problem?
Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:60.0) Gecko/20100101 Firefox/60.0
20180307220100

(In reply to Maverick from comment #12)
> Resent it just now (has a Zip attached with everything in it).

Got it. I couldn't reproduce this with either find-as-you-type or by bringing up the find bar first. I don't have a barcode scanner and couldn't test that. Sorry for not keeping that in mind, but that requirement wasn't mentioned in either the summary (which I'll now edit) or the description.

I suggest you test in a brand new profile [1] with the latest Nightly [2]. If you can still reproduce the issue, you can try heading over to IRC [3] to see if anyone has a barcode scanner and can test this to confirm it.


[1] https://support.mozilla.org/kb/profile-manager-create-and-remove-firefox-profiles
[2] https://nightly.mozilla.org
[3] https://wiki.mozilla.org/IRC
Component: Find Toolbar → Find Backend
Product: Toolkit → Core
See Also: → 1407312
Summary: Search function does not find some listed entries → Search function does not find some listed entries when using barcode scanner
(In reply to Niklas Hambüchen from comment #13)
> @Maverick: Can you see if you can reproduce
> https://bugzilla.mozilla.org/show_bug.cgi?id=1443249? Maybe it's the same
> problem?

Yes, I can definitely reproduce the problem (bug #1443249) here!!!! 
Not Just that but also the other one mentioned in the comments ( bug #1441486 )

So, i believe they're the same!
(In reply to Gingerbread Man from comment #14)
> Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:60.0) Gecko/20100101
> Firefox/60.0
> 20180307220100
> 
> (In reply to Maverick from comment #12)
> > Resent it just now (has a Zip attached with everything in it).
> 
> Got it. I couldn't reproduce this with either find-as-you-type or by
> bringing up the find bar first. I don't have a barcode scanner and couldn't
> test that. Sorry for not keeping that in mind, but that requirement wasn't
> mentioned in either the summary (which I'll now edit) or the description.
> 
> I suggest you test in a brand new profile [1] with the latest Nightly [2].
> If you can still reproduce the issue, you can try heading over to IRC [3] to
> see if anyone has a barcode scanner and can test this to confirm it.
> 
> 
> [1]
> https://support.mozilla.org/kb/profile-manager-create-and-remove-firefox-
> profiles
> [2] https://nightly.mozilla.org
> [3] https://wiki.mozilla.org/IRC

Since i constantly use the barcode scanner i never tried it by hand-typing. I have now taken the time and tried it manually and i can still reproduce it!!! Still, has i mentioned above i believe this is the same problem as the other two bugs...
Will do more tests ASAP following above instructions.

In the meanwhile i'm ok whit closing this as duplicate of one of the other two bugs.
So... Firefox 59 and Firefox Nightly (60) both still have the bug present!
I've tried both with a new profile but none found all the strings!
See Also: → 1443249
(In reply to Maverick from comment #17)
> In the meanwhile i'm ok whit closing this as duplicate of one of the other
> two bugs.

I'm not confident enough to mark any as a duplicate of another:
* This - Linux. Reporter can reproduce it in a brand new profile in Nightly (comment 18).
* Bug 1443249 - Linux. GitHub page given as example. Reporter can reproduce it in safe mode in Nightly, but not in a brand new profile, even when copying prefs.js over from the regular profile.
* Bug 1441486 - Windows 10. GitHub page given as example. This is the only one I can at least partially reproduce.

If there's a single underlying cause for these, I for one can't see why it would manifest itself so differently.
(In reply to Gingerbread Man from comment #19)
> (In reply to Maverick from comment #17)
> > In the meanwhile i'm ok whit closing this as duplicate of one of the other
> > two bugs.
> 
> I'm not confident enough to mark any as a duplicate of another:
> * This - Linux. Reporter can reproduce it in a brand new profile in Nightly
> (comment 18).
> * Bug 1443249 - Linux. GitHub page given as example. Reporter can reproduce
> it in safe mode in Nightly, but not in a brand new profile, even when
> copying prefs.js over from the regular profile.
> * Bug 1441486 - Windows 10. GitHub page given as example. This is the only
> one I can at least partially reproduce.
> 
> If there's a single underlying cause for these, I for one can't see why it
> would manifest itself so differently.

OK. You for sure have a better view over this than me!
This last tests i've made in this (and the other bug) was made on a windows 10 (64bit) machine. 
I also have access to Linux machines if you think it's relevant.
(In reply to Gingerbread Man from comment #19)

> If there's a single underlying cause for these, I for one can't see why it
> would manifest itself so differently.

I've sent you a few mails. 
Did you get any of them?
(In reply to Maverick from comment #21)
> I've sent you a few mails.
> Did you get any of them?

I received your testcase, see comment 14.

I also received your question regarding bug 1443249, comment 4. Sorry I couldn't reply earlier. Yes, I did mean when you say you can or cannot reproduce a bug, enter about:support into the location bar and manually paste the user agent string and build ID at the top of your comment here. Bugs can be platform-dependent, and they can be fixed from one Nightly build to the next, so this information is important.
(In reply to Gingerbread Man from comment #22)
> (In reply to Maverick from comment #21)
> > I've sent you a few mails.
> > Did you get any of them?
> 
> I received your testcase, see comment 14.
> 
> I also received your question regarding bug 1443249, comment 4. Sorry I
> couldn't reply earlier. Yes, I did mean when you say you can or cannot
> reproduce a bug, enter about:support into the location bar and manually
> paste the user agent string and build ID at the top of your comment here.
> Bugs can be platform-dependent, and they can be fixed from one Nightly build
> to the next, so this information is important.

Mozilla/5.0 (X11; Linux x86_64; rv:59.0) Gecko/20100101 Firefox/59.0

This is really weird (that you can't reproduce it)!!! 

What are the possibilities then? What do you suggest?

Could it be something specific to my language?
Something an addon changed?

Still it does not happen only to me as there are also the reports from bug 1441486 and bug 1443249

If you want, i can arrange a remote-desktop session to try to identify the bug or why it's happening...
Brad, if you can, please have a look at this and the bugs in the See Also field, notably bug 1443249. If you can't, please bring it to the attention of someone else.


(In reply to Maverick from comment #23)
> What do you suggest?

(In reply to Gingerbread Man from comment #14)
> If you can still reproduce the issue, you can try heading over to IRC [3] to
> see if anyone has a barcode scanner and can test this to confirm it.
>
> [3] https://wiki.mozilla.org/IRC
Flags: needinfo?(bwerth)
See Also: → 1441486
In my case, i use a barcode scanner a lot, but i can also replicate it my manual typing.

the bug that's actually unique to a barcode scanner that might be connected to this bug is what i've mentioned in bug 1407312

Anyway, thanks for your time @Gingerbread Man
I'll figure this out. I'll need the testcase. Maverick, can you send to me, please, with instructions on how to reproduce?
Assignee: nobody → bwerth
Flags: needinfo?(bwerth) → needinfo?(johnmaverick74)
(In reply to Brad Werth [:bradwerth] from comment #26)
> I'll figure this out. I'll need the testcase. Maverick, can you send to me,
> please, with instructions on how to reproduce?

The mail is returned everytime due to security issues.

I'm sending a .ZIP file
(In reply to Maverick from comment #27)
> The mail is returned everytime due to security issues.
> 
> I'm sending a .ZIP file

Probably easiest to use a secure file-sharing service. We have one! https://send.firefox.com
(In reply to Brad Werth [:bradwerth] from comment #28)
> (In reply to Maverick from comment #27)
> > The mail is returned everytime due to security issues.
> > 
> > I'm sending a .ZIP file
> 
> Probably easiest to use a secure file-sharing service. We have one!
> https://send.firefox.com

It only lasts 1h, however :(...

will send you the e-mail with the link
Flags: needinfo?(johnmaverick74)
Confirmed that this replicates in Nightly. Maverick, for a more-direct reproduction, you can just scan the 4th barcode on your list upon initially loading the page. That code is not found, though it appears on the page below the visible area.

Investigating...
Status: UNCONFIRMED → NEW
Ever confirmed: true
Corrected Steps to Reproduce:
Load page
Scroll to bottom
Search for 4th barcode in the list

This is happening because the text to be found is in a block formatting context (defined by an overflow:auto declaration) and is not currently visible in that overflow area, but its frame rect IS within the root scroll pane rect. In that situation, the code treats the text as visible only if it is actually rendered.

We'll need some kind of change that detects the scrollability of the block formatting context, and responds similarly to how we treat text that is outside the root scroll pane rect (we assume we can scroll to it and treat it as findable).

To be clear, this bug is not dependent on the font used, the characters being searched for, or on the input being done with a barcode scanner. I'm changing the bug title to reflect this.
Summary: Search function does not find some listed entries when using barcode scanner → Typeahead find won't scroll to overflow text in a block formatting context if text frame is within the root scroll pane
Attached file find_in_overflow.html
A much simpler testcase. Search for the word "target". You should be able to find it, but can't.
Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:61.0) Gecko/20100101 Firefox/61.0
20180329100042

(In reply to Brad Werth [:bradwerth] from comment #32)
> Search for the word "target". You should be able to find it, but can't.

I can definitely reproduce that.
Has STR: no → yes
Keywords: testcase
OS: Unspecified → All
Hardware: Unspecified → All
Attachment #8971042 - Flags: review?(bzbarsky)
Attachment #8971043 - Flags: review?(bzbarsky)
Attachment #8971044 - Flags: review?(bzbarsky)
Comment on attachment 8971042 [details]
Bug 1436431 Part 1: Extend PresShell::GetRectVisibility to consider the target frame's scrollable ancestors.

https://reviewboard.mozilla.org/r/239770/#review245586

::: layout/base/PresShell.cpp:3793
(Diff revision 1)
> +    scrollAncestorFrame =
> +      nsLayoutUtils::GetNearestScrollableFrame(f->GetParent(),
> +        nsLayoutUtils::SCROLLABLE_INCLUDE_HIDDEN);
> +  }
> +
> +  // Compare aRect in a rect relative to rootFrame.

I'm not sure what this comment is trying to say...
Attachment #8971042 - Flags: review?(bzbarsky) → review+
Comment on attachment 8971043 [details]
Bug 1436431 Part 2: Change nsTypeAheadFind::IsRangeVisible to additionally check for visibility of range rects; not just the range's primary frame.

https://reviewboard.mozilla.org/r/239772/#review245588

It would be nice if the commit message explained why this change is being made.

::: toolkit/components/typeaheadfind/nsTypeAheadFind.cpp:1283
(Diff revision 1)
> +      // least one of the rects is visible.
> +      bool atLeastOneRangeRectVisible = false;
> +
> +      nsIFrame* containerFrame =
> +        nsLayoutUtils::GetContainingBlockForClientRect(frame);
> +      RefPtr<nsRange> range = static_cast<nsRange*>(aRange);

Why do you need the cast?  Do you even need the RefPtr bit?  Or can you just use aRange below?

::: toolkit/components/typeaheadfind/nsTypeAheadFind.cpp:1284
(Diff revision 1)
> +      bool atLeastOneRangeRectVisible = false;
> +
> +      nsIFrame* containerFrame =
> +        nsLayoutUtils::GetContainingBlockForClientRect(frame);
> +      RefPtr<nsRange> range = static_cast<nsRange*>(aRange);
> +      RefPtr<mozilla::dom::DOMRectList> rects = range->GetClientRects(true, true);

You don't need the namespacing here.

::: toolkit/components/typeaheadfind/nsTypeAheadFind.cpp:1286
(Diff revision 1)
> +      nsIFrame* containerFrame =
> +        nsLayoutUtils::GetContainingBlockForClientRect(frame);
> +      RefPtr<nsRange> range = static_cast<nsRange*>(aRange);
> +      RefPtr<mozilla::dom::DOMRectList> rects = range->GetClientRects(true, true);
> +      for (uint32_t i = 0; i < rects->Length(); ++i) {
> +        RefPtr<mozilla::dom::DOMRect> rect = rects->Item(i);

Don't need namespacing.
Attachment #8971043 - Flags: review?(bzbarsky) → review+
Comment on attachment 8971044 [details]
Bug 1436431 Part 3: Add a test to ensure overflow text in the viewport is findable.

https://reviewboard.mozilla.org/r/239774/#review245590
Attachment #8971044 - Flags: review?(bzbarsky) → review+
Comment on attachment 8971043 [details]
Bug 1436431 Part 2: Change nsTypeAheadFind::IsRangeVisible to additionally check for visibility of range rects; not just the range's primary frame.

https://reviewboard.mozilla.org/r/239772/#review245588

You're right. Here's the text I added to the commit message:

Since a range rect can be considerably smaller than the rect of its
containing frame, this change avoids an unpleasant false-negative for
find-in-page. Without this change, if a frame is reported as visible,
but none of the range rects are visible, the later call to isRangeRendered
will report the text as not findable, even though it could be scrolled
into view. With this change, the text in such a case is reported as
findable, but just out of view, and the find-in-page logic will scroll
it into view as needed.
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a51257921d04
Part 1: Extend PresShell::GetRectVisibility to consider the target frame's scrollable ancestors. r=bz
https://hg.mozilla.org/integration/autoland/rev/6d1b03688ba6
Part 2: Change nsTypeAheadFind::IsRangeVisible to additionally check for visibility of range rects; not just the range's primary frame. r=bz
https://hg.mozilla.org/integration/autoland/rev/477ef0f2e047
Part 3: Add a test to ensure overflow text in the viewport is findable. r=bz
Another try run with a test that has more granular checks, hopefully leading to better failure messages: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a11b4e20c517edba32463e5c46ee333bfa45b9c0
Blocks: 1458393
Attachment #8972475 - Flags: review?(bzbarsky)
Comment on attachment 8972475 [details]
Bug 1436431 Part 4: Disable and update an existing mochitest of find-in-page.

https://reviewboard.mozilla.org/r/241064/#review247342
Attachment #8972475 - Flags: review?(bzbarsky) → review+
hg error in cmd: hg push -r tip ssh://hg.mozilla.org/integration/autoland: pushing to ssh://hg.mozilla.org/integration/autoland
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 4 changesets with 6 changes to 6 files
remote: transaction abort!
remote: rollback completed
remote: pretxnchangegroup.d_webidl hook is invalid: import of "mozhghooks.prevent_webidl_changes" failed
remote: (run with --traceback for stack trace)
abort: push failed on remote
Keywords: checkin-needed
Pushed by nerli@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b0c260eb196c
Part 1: Extend PresShell::GetRectVisibility to consider the target frame's scrollable ancestors. r=bz
https://hg.mozilla.org/integration/autoland/rev/ec1b3f950848
Part 2: Change nsTypeAheadFind::IsRangeVisible to additionally check for visibility of range rects; not just the range's primary frame. r=bz
https://hg.mozilla.org/integration/autoland/rev/dc092a92c289
Part 3: Add a test to ensure overflow text in the viewport is findable. r=bz
https://hg.mozilla.org/integration/autoland/rev/d462d502c26b
Part 4: Disable and update an existing mochitest of find-in-page. r=bz
Keywords: checkin-needed
Duplicate of this bug: 1456066
Duplicate of this bug: 1443249
Duplicate of this bug: 1441486
Duplicate of this bug: 1448087
Flags: in-testsuite+
Regressions: 1559372
You need to log in before you can comment on or make changes to this bug.