Closed Bug 1323200 Opened 3 years ago Closed 3 years ago

Find bar cannot find content that is not visible

Categories

(Toolkit :: Find Toolbar, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla53
Tracking Status
firefox50 --- unaffected
firefox51 --- unaffected
firefox52 --- unaffected
firefox53 + verified

People

(Reporter: whimboo, Assigned: mikedeboer)

References

Details

(Keywords: nightly-community, regression)

Attachments

(3 files)

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:53.0) Gecko/20100101 Firefox/53.0 ID:20161212030206 CSet: 42086c06f756cda7fbc25a2e7c20a5711f7e5f26

This seems to be a recent regression on mozilla-central. It still works for me on mozilla-aurora.

Whenever I search for a changeset id which is outside of the current viewport no search results are found.

Steps:
1. Open https://treeherder.mozilla.org/#/jobs?repo=autoland
2. Pick a changeset from below which is reachable via scrolling
3. Scroll back to the top
4. Search for the changeset or a subset of the commit id

No search results are found.
Flags: needinfo?(mdeboer)
Brad, can you take a look at this?
Flags: needinfo?(mdeboer) → needinfo?(bwerth)
(In reply to Henrik Skupin (:whimboo) from comment #1)
> Regression range:
> 
> https://hg.mozilla.org/mozilla-central/
> pushloghtml?fromchange=8404d26166a35406f46ff237ed132735c98882b2&tochange=c51e
> 7406d7b2e2246a1ece0d8989282ca752039f
> 
> So it looks like it regressed by bug 1302470.

This is exactly what I got with mozregression, search in page is totally busted, bug 1302470 should be either fixed or backed out.
Summary: Find bar doesn't find changeset ids on Treeherder → Find bar cannot find content that is not visible
Searching when the content is visible
Scrolling down the same page, no result found.
[Tracking Requested - why for this release]:
Major regression which break the search feature for content that is not visible.
I believe the problem is at https://dxr.mozilla.org/mozilla-central/source/toolkit/components/typeaheadfind/nsTypeAheadFind.cpp#816. Whatever side effect is required here may have been altered by the patch in bug 1302470. I'll try to figure out what that side effect was and restore it.
Flags: needinfo?(bwerth)
Yeah, I think so too, but I think the method is doing what it's supposed to do though. Something I've noticed whilst implementing a hit-test method was that it indeed discarded points that were outside the viewable region, which means that ranges that can be scrolled to but are not inside the current viewport were deemed invisible.
Strictly true, but _we_ need to include ranges that _would_ be visible once the scroll position changes of the root frame.
(In reply to Mike de Boer [:mikedeboer] from comment #8)
> Yeah, I think so too, but I think the method is doing what it's supposed to
> do though. Something I've noticed whilst implementing a hit-test method was
> that it indeed discarded points that were outside the viewable region, which
> means that ranges that can be scrolled to but are not inside the current
> viewport were deemed invisible.

You're right; there's more to it than I suggested in comment 7. I'm looking into it.
Okay, new theory -- this continue is unnecessarily applied unconditionally: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/typeaheadfind/nsTypeAheadFind.cpp?q=nsTypeAheadFind.cpp&redirect_type=direct#480 . I took it out and my limited test cases seem to perform better (including this one). That's a pre-existing continue that was left in place after bz's patch in bug 804799. I think it may only need to be applied in the else cases on lines 459 and 474.

Boris, I'd appreciate your insights. In the meanwhile, I'll push a patch with this continue only applied in the else cases.
Flags: needinfo?(bzbarsky)
Attachment #8819397 - Flags: review?(mdeboer)
Attachment #8819397 - Flags: review?(bzbarsky)
(In reply to Brad Werth [:bradwerth] from comment #10)
> I think it may only need
> to be applied in the else cases on lines 459 and 474.

Correcting myself: the continue appears to be needed in the if case. I've pushed a patch and included bz as one of the reviewers, so clearing the needinfo I set on him.
Flags: needinfo?(bzbarsky)
So I'm a bit confused.  https://reviewboard.mozilla.org/r/91770/diff/5#index_header totally changes the semantics of the isRangeVisible method.  In particular, the method _used_ to have an aMustBeInViewPort argument, and that argument was summarily removed and not replaced with anything.  As a result, for example, the aFirstVisiblePreferred argument of nsTypeAheadFind::GetSearchContainers is now unused.  Why were these changes OK, exactly?
Flags: needinfo?(mstange)
Flags: needinfo?(mdeboer)
Flags: needinfo?(bugs)
My understanding was that the intention was to move the responsibility of doing those checks into the caller. I did not review the intentions very carefully.
In fact, when reviewing this I completely forgot that we have the old find bar implementation as a user of this API and that we need to keep it happy.
I'm in favor of backing out bug 1302470 until a proper solution has been figured out.
Flags: needinfo?(mstange)
yeah, backing out bug 1302470 sounds like a good approach.

I do also realize now that I was confused about aMustBeInViewPort. It wasn't about must-be-in-view-port, but it was about must-not-be-above-viewport.
Flags: needinfo?(bugs)
> My understanding was that the intention was to move the responsibility of doing those checks into the caller

But now the callee _always_ enforces in-viewport, no?  So it will return false when it used to return true, afaict.
Attachment #8819397 - Flags: review?(mstange)
Attachment #8819397 - Flags: review?(bzbarsky)
Attachment #8819397 - Flags: review?(bugs)
Brad, I don't recall how this code works well enough to review this quickly.  I'm hoping the reviewers from bug 1302470 do.  My understanding of what's going on is very clearly flawed because this testcase:

  <iframe srcdoc="<div style='height: 1000px'></div>text"></iframe>
  <iframe srcdoc="<div style='height: 1000px'></div>text"></iframe>
  <div style="height: 5000px"></div>
  text

has no problem finding any of those "text" instances, even though none of them would be hit by hit-testing...

If I use a scrollable thing that's not the viewport, I do reproduce this bug, but I'm not sure why the viewport situation is different.
General thought, though: I _think_ the idea was that if !IsRangeVisible() (by the _old_ definition!) then it's a range we're not supposed to find because it is in fact invisible, period.  So we should set up a place to keep searching from (mStartPointRange) and then move on to looking for the next thing.  That's why, I think, the continue is unconditional in the !IsRangeVisible() block, after we have done all the checking to figure out how to set up mStartPointRange.

Again, the changes in bug 1302470 look like they should make us enter this block in more cases...
So I tried the patch in this bug.  For this testcase:

  <div style="overflow: scroll; height: 500px">
    text
    <div style="height: 5000px"></div>
    text
  </div>

it ends up hitting this assertion:

        NS_ASSERTION(mFoundEditable, "Independent selection controller on "
                     "non-editable element!");

precisely because it doesn't continue when the rest of the logic expects it to continue, I think....  Again, I don't have this stuff anywhere close to page in, so I might be totally barking up the wrong tree here.
Comment on attachment 8819397 [details]
Bug 1323200 Part 1: Fix TypeAheadFind to allow text outside of view bounds to be returned (correctly) as found text.

Sounds like we really want backout now to get Nightlies into the shape for the weekend.
Attachment #8819397 - Flags: review?(mstange)
Attachment #8819397 - Flags: review?(mdeboer)
Attachment #8819397 - Flags: review?(bugs)
(or some other fix to not trigger that assertion)
Brad or Mike, do you have time to do the backout? (or fix this asap)
Flags: needinfo?(bwerth)
I managed to back it out. Should hit tonight's Nightly builds.
Thanks Wes!
Flags: needinfo?(mdeboer)
Flags: needinfo?(bwerth)
(In reply to Olli Pettay [:smaug] from comment #15)
> yeah, backing out bug 1302470 sounds like a good approach.
> 
> I do also realize now that I was confused about aMustBeInViewPort. It wasn't
> about must-be-in-view-port, but it was about must-not-be-above-viewport.

My confusion is of the exact same kind. All things considered here - even though the interim state of a backout may be a bit sad - the end result will be that we'll get a better grasp on this code (again)!
Brad, I really hope you still have the time to work on this.

I hope we all can (still) agree that the 'old' behavior (which is back now) is flawed as well (bug 1302470).

Wes, thanks for the work.
Not sure we still need to track this for 53 - sounds as if the backout has reverted to the old behavior, and there may still be work continuing on this bug.
Fixed by backout.
Assignee: nobody → mdeboer
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:53.0) Gecko/20100101 Firefox/53.0
Build ID:20170105030229

Verified as fixed using the latest Nightly 53.0a1(Build ID:20170105030229) on Windows 10 x64, Ubuntu 16.04 and Mac OS X 10.11.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.