Closed Bug 1411050 Opened 4 years ago Closed 4 years ago

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

Categories

(Toolkit :: Find Toolbar, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: gps, Assigned: bradwerth)

References

Details

Attachments

(3 files)

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.
Component: Search → Find Toolbar
Product: Firefox → Toolkit
Version: unspecified → Trunk
Summary: Search broken on https://projects.fivethirtyeight.com/congress-trump-score/house/ → Find in page broken on https://projects.fivethirtyeight.com/congress-trump-score/house/
I can't reproduce this on Fx 57.0b8.
Status: NEW → UNCONFIRMED
Ever confirmed: false
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Yeah, I was still building my local build to confirm this.
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.
Blocks: 1302470
Severity: normal → major
Flags: needinfo?(bwerth)
Priority: -- → P1
s/is/as/
I'll take a look and figure it out.
Assignee: nobody → bwerth
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.
Flags: needinfo?(bwerth) → needinfo?(mdeboer)
Nevermind, I can reproduce now. Not sure why it was working for me before...
Flags: needinfo?(mdeboer)
You can successfully find any characters in the text "look here" but you can't find any characters of the text "can't find".
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.
Summary: Find in page broken on https://projects.fivethirtyeight.com/congress-trump-score/house/ → Find in page broken for elements with "pointer-events:none" style
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.
Attachment #8921671 - Flags: review?(matt.woodrow)
Attachment #8921672 - Flags: review?(mdeboer)
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
Attachment #8921671 - Flags: review?(matt.woodrow) → review+
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.
Attachment #8921672 - Flags: review?(mdeboer) → review+
Pushed by bwerth@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/a6907856e177
https://hg.mozilla.org/mozilla-central/rev/3997f82fb952
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.