Closed
Bug 1436431
Opened 7 years ago
Closed 7 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)
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)
Comment 1•7 years ago
|
||
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)
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)
Comment 7•7 years ago
|
||
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
Comment 9•7 years ago
|
||
(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.
Reporter | ||
Comment 10•7 years ago
|
||
(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)
Comment 11•7 years ago
|
||
(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.
Reporter | ||
Comment 12•7 years ago
|
||
(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.
Comment 13•7 years ago
|
||
@Maverick: Can you see if you can reproduce https://bugzilla.mozilla.org/show_bug.cgi?id=1443249? Maybe it's the same problem?
Comment 14•7 years ago
|
||
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
Summary: Search function does not find some listed entries → Search function does not find some listed entries when using barcode scanner
Reporter | ||
Comment 15•7 years ago
|
||
(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!
Reporter | ||
Comment 16•7 years ago
|
||
(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...
Reporter | ||
Comment 17•7 years ago
|
||
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.
Reporter | ||
Comment 18•7 years ago
|
||
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!
Comment 19•7 years ago
|
||
(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.
Reporter | ||
Comment 20•7 years ago
|
||
(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.
Reporter | ||
Comment 21•7 years ago
|
||
(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?
Comment 22•7 years ago
|
||
(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.
Reporter | ||
Comment 23•7 years ago
|
||
(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...
Comment 24•7 years ago
|
||
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)
Reporter | ||
Comment 25•7 years ago
|
||
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
Assignee | ||
Comment 26•7 years ago
|
||
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)
Reporter | ||
Comment 27•7 years ago
|
||
(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
Assignee | ||
Comment 28•7 years ago
|
||
(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
Reporter | ||
Comment 29•7 years ago
|
||
(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)
Assignee | ||
Comment 30•7 years ago
|
||
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
Assignee | ||
Comment 31•7 years ago
|
||
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
Assignee | ||
Comment 32•7 years ago
|
||
A much simpler testcase. Search for the word "target". You should be able to find it, but can't.
Comment 33•7 years ago
|
||
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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8971042 -
Flags: review?(bzbarsky)
Attachment #8971043 -
Flags: review?(bzbarsky)
Attachment #8971044 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 37•7 years ago
|
||
Comment hidden (mozreview-request) |
Assignee | ||
Comment 39•7 years ago
|
||
Comment hidden (mozreview-request) |
Assignee | ||
Comment 41•7 years ago
|
||
Comment 42•7 years ago
|
||
mozreview-review |
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 43•7 years ago
|
||
mozreview-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 44•7 years ago
|
||
mozreview-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+
Assignee | ||
Comment 45•7 years ago
|
||
mozreview-review-reply |
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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 49•7 years ago
|
||
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
Comment 50•7 years ago
|
||
Backed out 3 changesets (bug 1436431) for Linting failures on a CLOSED TREE
Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=477ef0f2e047e270514c9cfeb9a328c0a99a0797&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&selectedJob=175847005
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=175847005&repo=autoland&lineNumber=682
Backout: https://hg.mozilla.org/integration/autoland/rev/4e857676a1a54ca6d8f3acc9883c66f4c88176d9
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 54•7 years ago
|
||
Assignee | ||
Comment 55•7 years ago
|
||
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
Assignee | ||
Comment 56•7 years ago
|
||
Assignee | ||
Comment 57•7 years ago
|
||
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 62•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Attachment #8972475 -
Flags: review?(bzbarsky)
Comment 63•7 years ago
|
||
mozreview-review |
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+
Comment 64•7 years ago
|
||
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
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 65•7 years ago
|
||
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
Comment 66•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b0c260eb196c
https://hg.mozilla.org/mozilla-central/rev/ec1b3f950848
https://hg.mozilla.org/mozilla-central/rev/dc092a92c289
https://hg.mozilla.org/mozilla-central/rev/d462d502c26b
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Updated•6 years ago
|
Flags: in-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•