1411050 - Find in page broken for elements with "pointer-events:none" style

Status: RESOLVED FIXED

(Toolkit :: Find Toolbar, defect, P1)

Description

[Gregory Szorc [:gps]]

Running Firefox Nightly 20171023100252 64-bit on Windows and MacOS text search fails to work on https://projects.fivethirtyeight.com/congress-trump-score/house/.

STR

1. Load https://projects.fivethirtyeight.com/congress-trump-score/house/
2. CTRL+f
3. Type "Pel" and see that part of "Nancy Pelosi" is highlighted
4. Type "o" so the search term is "Pelo." Note how there are no longer any search results. This is failure #1.
5. Type BACKSPACE so the search term is "Pel." Note how search no longer finds "Pelosi" despite finding it before. This is failure #2.

Looking at the HTML, I don't see anything obvious that would prevent search from working. The fact that BACKSPACE to an original search term fails to find a search result that was previously found for this term seems to indicate this is a bug in Firefox's search feature and not something wonky at the HTML layer.

I am unsure what the regression range is because I haven't tested.

Comment 1

[Mike de Boer [:mikedeboer]]

I can't reproduce this on Fx 57.0b8.

Comment 2

[Francesco Lodolo [:flod]]

I can easily reproduce on Nightly 58.0a1 (2017-10-23) (64 bit), you are marking as unconfirmed based on Beta, but comment 0 clearly states that it used to work.

Comment 3

[Mike de Boer [:mikedeboer]]

Yeah, I was still building my local build to confirm this.

Comment 4

[Mike de Boer [:mikedeboer]]

Hi Brad, this is another case where the visibility check lies to us... When you check out the table cell, there's two nodes in there that contain the text 'Pelo', but one of the two is hidden and the other isn't. Somehow both are marked is invisible, since they share the same bounds (my uneducated guess).
It doesn't seem wildly complicated, but it is important to get right, I think.

Comment 5

[Mike de Boer [:mikedeboer]]

s/is/as/

Comment 6

[Brad Werth [:bradwerth]]

I'll take a look and figure it out.

Comment 7

[Brad Werth [:bradwerth]]

Hmm... this is working for me on my macOS Nightly build. Mike, can you confirm this is still happening for you?

The HTML usage noted in comment 4 is a bit weird, but our find code should handle it. What we have is two divs with similar text, one visible and one with "display:none". They aren't overlaid (no abs-pos that I could see), and the hidden one won't generate a displayitem that would get involved in the visibility check. There might be some weirdness where the hidden div if out of viewport would be reported as a match, but then scrolling to it would show no result. But that's not the error reported here.

Comment 8

[Brad Werth [:bradwerth]]

Nevermind, I can reproduce now. Not sure why it was working for me before...

Comment 9

[Brad Werth [:bradwerth]]

Attachment: find-with-pointer-events-none.html

You can successfully find any characters in the text "look here" but you can't find any characters of the text "can't find".

Comment 10

[Brad Werth [:bradwerth]]

Greatly reduced testcase attached. It has nothing to do with tables or hidden text, just a failure to handle text with the "pointer-events:none" style.

Comment 11

[Brad Werth [:bradwerth]]

I determined that this problem is happening because our HitTest code makes the assumption that all hit tests are for the purposes of handling pointer events. There's a code path at http://searchfox.org/mozilla-central/rev/d30462037ffea383e74c42542c820cf65b2b144e/layout/painting/nsDisplayList.cpp#2767 that filters out frames that aren't receiving pointer events. I'll make a flag in the nsDisplayListBuilder that indicates whether hit tests are for pointer events or not.

Comment 12

[Brad Werth [:bradwerth]]

Attachment: Bug 1411050 Part 1: Change nsDisplayListBuilder to mark hitests either for visibility or for pointer events.

Review commit: https://reviewboard.mozilla.org/r/192674/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/192674/

Comment 13

[Brad Werth [:bradwerth]]

Attachment: Bug 1411050 Part 2: Add a test to confirm pointer-events:none text is findable.

Review commit: https://reviewboard.mozilla.org/r/192676/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/192676/

Comment 14

[Brad Werth [:bradwerth]]

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

Comment 15

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8921671 [details]
Bug 1411050 Part 1: Change nsDisplayListBuilder to mark hitests either for visibility or for pointer events.

https://reviewboard.mozilla.org/r/192674/#review197870

Comment 16

[Mike de Boer [:mikedeboer]]

Comment on attachment 8921672 [details]
Bug 1411050 Part 2: Add a test to confirm pointer-events:none text is findable.

https://reviewboard.mozilla.org/r/192676/#review198052

r=me. Thanks for finding & fixing this so quickly!

::: toolkit/modules/tests/browser/browser_Finder_pointer_events_none.js:5
(Diff revision 1)
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +add_task(async function test_offscreen_text() {
> +

nit: superfluous newline.

::: toolkit/modules/tests/browser/browser_Finder_pointer_events_none.js:6
(Diff revision 1)
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +add_task(async function test_offscreen_text() {
> +
> +  const URI =

nit: I think the string literal can stay on this line as well.

Comment 17

[Brad Werth [:bradwerth]]

Comment on attachment 8921671 [details]
Bug 1411050 Part 1: Change nsDisplayListBuilder to mark hitests either for visibility or for pointer events.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/192674/diff/1-2/

Comment 18

[Brad Werth [:bradwerth]]

Comment on attachment 8921672 [details]
Bug 1411050 Part 2: Add a test to confirm pointer-events:none text is findable.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/192676/diff/1-2/

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a6907856e177
Part 1: Change nsDisplayListBuilder to mark hitests either for visibility or for pointer events. r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/3997f82fb952
Part 2: Add a test to confirm pointer-events:none text is findable. r=mikedeboer

Comment 20

[Attila Craciun [:arny]]

https://hg.mozilla.org/mozilla-central/rev/a6907856e177
https://hg.mozilla.org/mozilla-central/rev/3997f82fb952