1482147 - Search in page can't find text in offscreen overflow:hidden containers

Status: RESOLVED FIXED

(Core :: Find Backend, defect, P2)

Description

[Florian Lasinger]

Attachment: minimal example page

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:61.0) Gecko/20100101 Firefox/61.0
Build ID: 20180704192850

Steps to reproduce:

Open Jira Active Sprint view -> Hit Ctrl-F -> search some text contained in a task that is out of view and only visible by scrolling down the page.

A stripped down example page without scripts and external references is attached.


Actual results:

Firefox reports zero occurrences. The search term is not found.

(This only happens if the element to be searched for is below the current viewport. If it is in view or above, it is found correctly.)


Expected results:

The search term should be found. Firefox should report 1 occurrence. Firefox should scroll the occurrence into view.

Comment 1

[Florian Lasinger]

Running Firefox 61.0.1 64 bit on Ubuntu 4.15.0-30.
Search for 'findme' in the attached page.
All the 'other ticket' issues are there so findme can be scrolled out of view either to the top or bottom.

Among others, things will start to work if you
- remove the height from .ghx-work OR
- remove display: block from .ghx-inner

Comment 2

[Florian Lasinger]

Oh boy... I just came back to work only to realize that my example is working for me today... It definitely did not yesterday. And the original unstripped Jira Page is still not working.

I will try to create another example page.

But please someone try the old example. It will still be interesting to see for how many people it works.

Comment 3

[Florian Lasinger]

Attachment: Second example page

I created a second example page. This one works for me currently, even after rebooting the system.

This time all Jira tasks are numbered. Try searching for e.g. task20.

Comment 4

[Cristian Craciun]

I've tested this issue and didn't managed to reproduce it using the first example, but I've managed to reproduce it using the second one. 

The issue occurs on all platforms (Linux, Mac OS and Windows) and all three versions 61 (release), 62 (beta) and 63 (nightly). 

Based on this, I've changed the status of this bug to NEW and set the right component.

Comment 5

[Mike de Boer [:mikedeboer]]

Hi Brad, this looks like one variation you might have missed for the visibility check (yes, the web is a fantastically diverse place!) - can you take a look whether that's the case?

So it's attachment 8999117 [details] and enter 'task20' in the findbar. Thanks!!

Comment 6

[Dinar]

[Tracking Requested - why for this release]:

Comment 7

[Dinar]

see also bug 1475180

Comment 8

[Brad Werth [:bradwerth]]

This has a strange, interesting cause. At https://searchfox.org/mozilla-central/rev/2a6f3dde00801374d3b2a704232de54a132af389/layout/base/PresShell.cpp#3586, we are checking for the directional visibility of a target rect relative to the intersection of the rects of all of its scroll frame ancestors, including the root scroll frame. Either the rect should be partly visible, or should be scrollable into view either up, down, left, or right. Here's what's happening in this case:

1. Text frames are enclosed in an overflow:hidden element, which creates a scroll frame.
2. The text of interest is off the bottom of the visible area. That means the correct result would be to return nsRectVisibility_kBelowViewport.
3. This algorithm intersects the off-screen scroll frame with the root scroll frame and comes up with an empty rect LOCATED AT THE ORIGIN of the off-screen scroll frame. This is because of how we define our Intersect function at https://searchfox.org/mozilla-central/rev/2a6f3dde00801374d3b2a704232de54a132af389/gfx/2d/BaseRect.h#112.
4. The algorithm then deflates the empty visible area rect, which pushes it slightly right and down. It is still empty, but now its origin is inside the bounds of the text frame.
5. The algorithm concludes that since the text frame is neither wholly above, below, to the left or right of the (empty) visible rect, it must be visible.

I'm not sure what the best solution will be, though one option may be to post-process an empty visible rect after Step 3 to favor a different origin.

Comment 9

[Brad Werth [:bradwerth]]

Attachment: Bug 1482147 Part 1: Make scroll port visibility checks better handle non-intersecting rects. r=mstange!

Comment 10

[Brad Werth [:bradwerth]]

Attachment: find_task20.html

attachment 9049392 [details] solves the original issue, but reveals another issue that I'd like to characterize better before closing this bug. With a similar local testcase, the text is always found, but not always scrolled into view. It appears to be related to some transform that is not accounted for when determining if a range is visible.

I've attached my local testcase here. You may be able to reproduce this new issue by applying the attachment 9049392 [details] patch and then searching repeatedly for "task2". The text will consistently be found, but not always scrolled into view.

Comment 11

[Brad Werth [:bradwerth]]

Attachment: find_within_clipped_scrollFrame.html

This attachment is a minimal testcase. Searching for the test "findme" fails to scroll the text into view. Here's the reason:

nsIPresShell::ScrollFrameRectIntoView() walks up from the found nsTextFrame and attempts to scroll it into view. It does this by walking up the frame ancestor chain. For each scrollframe ancestor, it scrolls it if necessary and possible. For EVERY ancestor frame, it transforms the nsTextFrame rect into the ancestor's coordinate space.

In this case:

1. The first scrollFrame ancestor has overflow-y:scroll, so could scroll if it wanted to. However, it decides that the nsTextFrame is within its scrollportrect, so it doesn't attempt to scroll.
2. The body is the next scrollFrame ancestor, and it has overflow:hidden, so it can't scroll to the nsTextFrame.

So a possible fix will be to make each scrollportrect intersect with the top level scrollportrect for purposes of determining the need to scroll into view. Something like that.

Comment 12

[Brad Werth [:bradwerth]]

Attachment: Bug 1482147 Part 2: Refactor GetRectVisibility to separate viewport retrieval. r=mstange!

In addition to the existing callsite, this new function will be useful
for other functions that want to compare against the scroll ancestor
visible area.

Depends on D22626

Comment 13

[Brad Werth [:bradwerth]]

Attachment: Bug 1482147 Part 3: Make nsIPresShell::ScrollFrameRectIntoView consider viewport visibility. r=mstange!

This patch propagates the top level visible area through to all the calls
to ScrollToShowRect() for purposes of intersection with the local
scrollFrame's visible area.

Depends on D23420

Comment 14

[Brad Werth [:bradwerth]]

Attachment: Bug 1482147 Part 4: Add a test of finding text in a clipped scrollFrame. r=mstange!

This test has a scrollable div within a non-scrollable div. The scrollable
div has a size big enough to include the target text, but it's clipped
by the non-scrollable div. The other changes in this patch ensure that
the scrollable div only considers its visible area intersected with
the viewport visible area, ensuring that the text is not only found,
but scrolled into view.

Depends on D23422

Comment 15

[Brad Werth [:bradwerth]]

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

Comment 17

[Markus Stange [:mstange]]

Sorry for the long delay in reviews. I'm going to start reviewing these patches today.

Comment 18

[Brad Werth [:bradwerth]]

(In reply to Markus Stange [:mstange] from comment #17)

Sorry for the long delay in reviews. I'm going to start reviewing these patches today.

There are still test failures I haven't resolved. I would still like your review on what's posted, because if you spot any questionable choices there, it might help me spot the changes needed. If you don't want to be in that position, feel free to resign as reviewer and I'll re-apply you when the patches are solid.

Comment 19

[Emilio Cobos Álvarez (:emilio)]

Any update on this Brad? Any way I can help?

Comment 20

[Brad Werth [:bradwerth]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #19)

Any update on this Brad? Any way I can help?

Ack, it's been so long. I'll rebase the patches and send a new try server push to see the state of the tests.

Comment 21

[Brad Werth [:bradwerth]]

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

Comment 22

[Brad Werth [:bradwerth]]

Attachment: Bug 1482147 Part 5: Make a scroll port test open a larger window.

This is necessary because the test is designed to scroll a frame into
view within its container's viewport. The test currently does this
within a window that is too small to show all the viewports being
tested. Changes in this patch series makes the root viewport also
be considered for whether or not a frame has been scrolled into view.

Depends on D23428

Comment 24

[Brad Werth [:bradwerth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6e5ad620c555835b61efad1360c4cdf3a13cc30b

Comment 25

[Brad Werth [:bradwerth]]

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

Comment 26

[Brad Werth [:bradwerth]]

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

Comment 28

[kiprasm]

Any updates on this issue?

Comment 29

[Markus Stange [:mstange]]

It's currently waiting on my review. I really hope to get to it soon.

Comment 31

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1482147 - Simplify nsTypeAheadFind visibility code to not lie. r=mstange

Multiple issues here. The IsRangeVisible code was wrong, it was returning false
for ranges that were perfectly valid, but outside the viewport, because the
following piece of code:

if (!aMustBeInViewPort) {
// This is an early exit case because we don't care that that range
// is out of viewport, so we return that the range is "visible".
return true;
}

Was incorrectly after some stuff checking viewport visibility. This code is
pretty complex for no good reason, it wants to do something very
simple: Start from the visible selection if possible.

This patch still achieves this, using IsRangeRendered (which does a proper
hit-test to figure out if a range is in the viewport). Should have no behavior
differences except for non-collapsed ranges that are partially inside the
viewport.

Comment 32

[Emilio Cobos Álvarez (:emilio)]

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

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6a15a1a87348
Simplify nsTypeAheadFind visibility code to not lie. r=masayuki

Comment 34

[Natalia Csoregi [:nataliaCs]]

Backed out for geckoview assertion error

backout: https://hg.mozilla.org/integration/autoland/rev/5c4dea9c2760d76b00abdea5ff74df04e2c604d2

push: https://treeherder.mozilla.org/#/jobs?repo=autoland&searchStr=android%2C7.0%2Cx86-64%2Copt%2Ctest-android-em-7.0-x86_64%2Fopt-geckoview-junit-e10s%2C%28gv-junit%29&tochange=6a15a1a873488084dd9fc9be1da317cd4b24cbe9&fromchange=9022d327e0bd4b19ce43684701eececc9459a4ef&selectedJob=297866186

failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=297866186&repo=autoland&lineNumber=3267

[task 2020-04-16T03:01:36.631Z] 03:01:36 INFO - TEST-START | org.mozilla.geckoview.test.FinderTest.find
[task 2020-04-16T03:01:37.584Z] 03:01:37 INFO - org.mozilla.geckoview.test | INSTRUMENTATION_STATUS: numtests=674
[task 2020-04-16T03:01:37.585Z] 03:01:37 INFO - org.mozilla.geckoview.test | INSTRUMENTATION_STATUS: stream=
[task 2020-04-16T03:01:37.586Z] 03:01:37 INFO - org.mozilla.geckoview.test | Error in find(org.mozilla.geckoview.test.FinderTest):
[task 2020-04-16T03:01:37.587Z] 03:01:37 INFO - org.mozilla.geckoview.test | java.lang.AssertionError: Should have wrapped
[task 2020-04-16T03:01:37.589Z] 03:01:37 INFO - org.mozilla.geckoview.test | Expected: <true>
[task 2020-04-16T03:01:37.590Z] 03:01:37 INFO - org.mozilla.geckoview.test | but: was <false>
[task 2020-04-16T03:01:37.592Z] 03:01:37 INFO - org.mozilla.geckoview.test | at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
[task 2020-04-16T03:01:37.594Z] 03:01:37 INFO - org.mozilla.geckoview.test | at org.junit.Assert.assertThat(Assert.java:956)
[task 2020-04-16T03:01:37.597Z] 03:01:37 INFO - org.mozilla.geckoview.test | at org.junit.rules.ErrorCollector$1.call(ErrorCollector.java:65)
[task 2020-04-16T03:01:37.599Z] 03:01:37 INFO - org.mozilla.geckoview.test | at org.junit.rules.ErrorCollector.checkSucceeds(ErrorCollector.java:78)
[task 2020-04-16T03:01:37.603Z] 03:01:37 INFO - org.mozilla.geckoview.test | at org.junit.rules.ErrorCollector.checkThat(ErrorCollector.java:63)
[task 2020-04-16T03:01:37.605Z] 03:01:37 INFO - org.mozilla.geckoview.test | at org.mozilla.geckoview.test.rule.GeckoSessionTestRule.checkThat(GeckoSessionTestRule.java:801)
[task 2020-04-16T03:01:37.605Z] 03:01:37 INFO - org.mozilla.geckoview.test | at org.mozilla.geckoview.test.BaseSessionTest.assertThat(BaseSessionTest.kt:81)
[task 2020-04-16T03:01:37.605Z] 03:01:37 INFO - org.mozilla.geckoview.test | at org.mozilla.geckoview.test.FinderTest.find(FinderTest.kt:28)
[task 2020-04-16T03:01:37.605Z] 03:01:37 INFO - org.mozilla.geckoview.test | at java.lang.reflect.Method.invoke(Native Method)
[task 2020-04-16T03:01:37.605Z] 03:01:37 INFO - org.mozilla.geckoview.test | at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
[task 2020-04-16T03:01:37.606Z] 03:01:37 INFO - org.mozilla.geckoview.test | at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
[task 2020-04-16T03:01:37.606Z] 03:01:37 INFO - org.mozilla.geckoview.test | at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
[task 2020-04-16T03:01:37.606Z] 03:01:37 INFO - org.mozilla.geckoview.test | at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
[task 2020-04-16T03:01:37.606Z] 03:01:37 INFO - org.mozilla.geckoview.test | at org.mozilla.geckoview.test.rule.GeckoSessionTestRule$2.lambda$evaluate$0$GeckoSessionTestRule$2(GeckoSessionTestRule.java:1284)
[task 2020-04-16T03:01:37.606Z] 03:01:37 INFO - org.mozilla.geckoview.test | at org.mozilla.geckoview.test.rule.-$$Lambda$GeckoSessionTestRule$2$sIbRNaZJgAu-QrUVWSGD8JbPSWM.run(lambda)
[task 2020-04-16T03:01:37.606Z] 03:01:37 INFO - org.mozilla.geckoview.test | at android.app.Instrumentation$SyncRunnable.run(Instrumentation.java:1950)
[task 2020-04-16T03:01:37.607Z] 03:01:37 INFO - org.mozilla.geckoview.test | at android.os.Handler.handleCallback(Handler.java:751)
[task 2020-04-16T03:01:37.608Z] 03:01:37 INFO - org.mozilla.geckoview.test | at android.os.Handler.dispatchMessage(Handler.java:95)
[task 2020-04-16T03:01:37.609Z] 03:01:37 INFO - org.mozilla.geckoview.test | at android.os.Looper.loop(Looper.java:154)
[task 2020-04-16T03:01:37.609Z] 03:01:37 INFO - org.mozilla.geckoview.test | at android.app.ActivityThread.main(ActivityThread.java:6077)
[task 2020-04-16T03:01:37.610Z] 03:01:37 INFO - org.mozilla.geckoview.test | at java.lang.reflect.Method.invoke(Native Method)
[task 2020-04-16T03:01:37.610Z] 03:01:37 INFO - org.mozilla.geckoview.test | at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:866)
[task 2020-04-16T03:01:37.611Z] 03:01:37 INFO - org.mozilla.geckoview.test | at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:756)
[task 2020-04-16T03:01:37.612Z] 03:01:37 INFO - org.mozilla.geckoview.test |
[task 2020-04-16T03:01:37.612Z] 03:01:37 INFO - org.mozilla.geckoview.test | INSTRUMENTATION_STATUS: id=AndroidJUnitRunner
[task 2020-04-16T03:01:37.613Z] 03:01:37 INFO - org.mozilla.geckoview.test | INSTRUMENTATION_STATUS: test=find
[task 2020-04-16T03:01:37.613Z] 03:01:37 INFO - org.mozilla.geckoview.test | INSTRUMENTATION_STATUS: class=org.mozilla.geckoview.test.FinderTest
[task 2020-04-16T03:01:37.613Z] 03:01:37 INFO - org.mozilla.geckoview.test | INSTRUMENTATION_STATUS: stack=java.lang.AssertionError: Should have wrapped
[task 2020-04-16T03:01:37.614Z] 03:01:37 INFO - org.mozilla.geckoview.test | Expected: <true>
[task 2020-04-16T03:01:37.614Z] 03:01:37 INFO - org.mozilla.geckoview.test | but: was <false>
[task 2020-04-16T03:01:37.615Z] 03:01:37 INFO - org.mozilla.geckoview.test | at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
[task 2020-04-16T03:01:37.615Z] 03:01:37 INFO - org.mozilla.geckoview.test | at org.junit.Assert.assertThat(Assert.java:956)
[task 2020-04-16T03:01:37.616Z] 03:01:37 INFO - org.mozilla.geckoview.test | at org.junit.rules.ErrorCollector$1.call(ErrorCollector.java:65)
[task 2020-04-16T03:01:37.616Z] 03:01:37 INFO - org.mozilla.geckoview.test | at org.junit.rules.ErrorCollector.checkSucceeds(ErrorCollector.java:78)
[task 2020-04-16T03:01:37.617Z] 03:01:37 INFO - org.mozilla.geckoview.test | at org.junit.rules.ErrorCollector.checkThat(ErrorCollector.java:63)
[task 2020-04-16T03:01:37.617Z] 03:01:37 INFO - org.mozilla.geckoview.test | at org.mozilla.geckoview.test.rule.GeckoSessionTestRule.checkThat(GeckoSessionTestRule.java:801)
[task 2020-04-16T03:01:37.618Z] 03:01:37 INFO - org.mozilla.geckoview.test | at org.mozilla.geckoview.test.BaseSessionTest.assertThat(BaseSessionTest.kt:81)
[task 2020-04-16T03:01:37.618Z] 03:01:37 INFO - org.mozilla.geckoview.test | at org.mozilla.geckoview.test.FinderTest.find(FinderTest.kt:28)
[task 2020-04-16T03:01:37.619Z] 03:01:37 INFO - org.mozilla.geckoview.test | at java.lang.reflect.Method.invoke(Native Method)
[task 2020-04-16T03:01:37.619Z] 03:01:37 INFO - org.mozilla.geckoview.test | at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
[task 2020-04-16T03:01:37.620Z] 03:01:37 INFO - org.mozilla.geckoview.test | at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
[task 2020-04-16T03:01:37.620Z] 03:01:37 INFO - org.mozilla.geckoview.test | at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
[task 2020-04-16T03:01:37.621Z] 03:01:37 INFO - org.mozilla.geckoview.test | at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
[task 2020-04-16T03:01:37.621Z] 03:01:37 INFO - org.mozilla.geckoview.test | at org.mozilla.geckoview.test.rule.GeckoSessionTestRule$2.lambda$evaluate$0$GeckoSessionTestRule$2(GeckoSessionTestRule.java:1284)
[task 2020-04-16T03:01:37.622Z] 03:01:37 INFO - org.mozilla.geckoview.test | at org.mozilla.geckoview.test.rule.-$$Lambda$GeckoSessionTestRule$2$sIbRNaZJgAu-QrUVWSGD8JbPSWM.run(lambda)
[task 2020-04-16T03:01:37.623Z] 03:01:37 INFO - org.mozilla.geckoview.test | at android.app.Instrumentation$SyncRunnable.run(Instrumentation.java:1950)
[task 2020-04-16T03:01:37.623Z] 03:01:37 INFO - org.mozilla.geckoview.test | at android.os.Handler.handleCallback(Handler.java:751)
[task 2020-04-16T03:01:37.623Z] 03:01:37 INFO - org.mozilla.geckoview.test | at android.os.Handler.dispatchMessage(Handler.java:95)
[task 2020-04-16T03:01:37.624Z] 03:01:37 INFO - org.mozilla.geckoview.test | at android.os.Looper.loop(Looper.java:154)
[task 2020-04-16T03:01:37.624Z] 03:01:37 INFO - org.mozilla.geckoview.test | at android.app.ActivityThread.main(ActivityThread.java:6077)
[task 2020-04-16T03:01:37.625Z] 03:01:37 INFO - org.mozilla.geckoview.test | at java.lang.reflect.Method.invoke(Native Method)
[task 2020-04-16T03:01:37.626Z] 03:01:37 INFO - org.mozilla.geckoview.test | at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:866)
[task 2020-04-16T03:01:37.626Z] 03:01:37 INFO - org.mozilla.geckoview.test | at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:756)
[task 2020-04-16T03:01:37.627Z] 03:01:37 INFO - org.mozilla.geckoview.test |
[task 2020-04-16T03:01:37.627Z] 03:01:37 INFO - org.mozilla.geckoview.test | INSTRUMENTATION_STATUS: current=110
[task 2020-04-16T03:01:37.628Z] 03:01:37 INFO - org.mozilla.geckoview.test | INSTRUMENTATION_STATUS_CODE: -2
[task 2020-04-16T03:01:37.628Z] 03:01:37 WARNING - TEST-UNEXPECTED-FAIL | org.mozilla.geckoview.test.FinderTest.find | java.lang.AssertionError: Should have wrapped
[task 2020-04-16T03:01:37.628Z] 03:01:37 INFO - TEST-INFO took 973ms

Comment 35

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1482147 - Fix the expectation of a GeckoView test which was relying on this bug. r=#geckoview-reviewers

It makes no sense that the first thing in the session would be considered as
wrapping, it was happening only because we were incorrectly failing to find a
"visible" range deep in Gecko (so we started back from the beginning of the
document, wrapping).

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ff7529683179
Simplify nsTypeAheadFind visibility code to not lie. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/cf2beed740ef
Fix the expectation of a GeckoView test which was relying on this bug. r=agi

Comment 37

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/ff7529683179
https://hg.mozilla.org/mozilla-central/rev/cf2beed740ef