1566599 - Respect ForceEmptyHitRegion flag in WR hit-test

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect, P3)

Description

[Kartikaya Gupta (email:[e-mail])]

There's the notion of the ForceEmptyHitRegion flag, which is used when an entire out-of-process subdocument should be ignored for hit-testing purposes. The way this works is the nsDisplayRemote puts the flag on the RefLayer here (for the non-WR codepath) or on the WR scroll data here. This then gets slurped into the HitTestingTreeNode here.

During hit-testing, the non-WR codepath checks this flag here. But in the WR codepath, the equivalent check is missing. Note that this flag is not part of the CompositorHitTestInfo items (because the flag is known in the parent process from where the CHTI items are generated) and so the check must be done in APZ code after the WR hit-test happens. Bug 1563622 adds a check for the very similar ForceDispatchToContent flag that can be used as a model.

The catch is that the semantics of the ForceEmptyHitRegion flag means that the item needs to be skipped over during hit-testing. When we do a WR hit-test, we get back a list of hit results in front-to-back ordering and iterate that list to pull out the frontmost item. Instead, we need to somehow modify this to keep iterating until we reach the frontmost item which does not have a ForceEmptyHitRegion item on the corresponding HitTestingTreeNode instance. Doing this will entail either passing in additional information (like a map of guids -> flags) to wr_api_hit_test, or returning the whole list of items from wr_api_hit_test, or some other way of combining the information from the WR hit-test with the flags stored in C++ code.

Similar machinery will be desirable for bug 1542020 (to determine exactly which hit-tests are ambiguous - see bug 1542020 comment 0) so it might make sense to write this with that in mind as well.

Comment 1

[Botond Ballo [:botond]]

IIUC, this should only affect:

1. With Fission enabled, pages where an OOP iframe has pointer-events: none.
2. (Hypothetically) with Fission disabled, cases where the browser UI sets pointer-events:none on the entire content area.

#1 suggests that we should track this for Fission. (Probably M5 unless we run into the issue on popular sites.)

#2 presumably doesn't happen, or we would've run into it already.

It's technically a regression in that non-WR handled these scenarios and WR doesn't, but as it doesn't actually occur in shipping configurations, it's not a high priority one. I'm going to mark it as fix-optional for now.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1566599 - Have the WR hit-test return all results to the caller. r?botond

No functional changes here, just plumbing to allow the caller to
access all the results instead of just the one picked out by
bindings.rs.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1566599 - Add support for ForceEmptyHitRegion on the WR hit-test. r?botond

Depends on D69202

Comment 4

[Kartikaya Gupta (email:[e-mail])]

The WIPs attached here should be complete, but I want to write a test before requesting review.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

I wrote a test and got it passing, but it uncovered a couple of other issues with these WIPs that I'm sorting through at the moment. Latest try push is at https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=8003eed4d2bf54ba1d92c66cdb88abe267a40013

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1566599 - Ensure EventRegionOverride flags don't end up on the HTTNs for reflayers. r?botond

We need to propagate the flags from the reflayer into the descendant subtree,
but remove the flag from the HTTN corresponding to the reflayer itself. See
comments in the patch for why.

Depends on D69203

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1566599 - Ensure we still check the flags on the node for the NULL_SCROLL_ID case. r?botond

For the case where we got a hit-result with a NULL_SCROLL_ID, we wouldn't
get a node, and would fall back to the root APZC for the layers id. But we
should actually still find the HTTN so that we can check the override flags.

Depends on D70395

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1566599 - Add a test for the ForceEmptyHitRegion flag. r?botond

Depends on D70396

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b09ad6ab0220
Have the WR hit-test return all results to the caller. r=botond
https://hg.mozilla.org/integration/autoland/rev/97cb47649452
Add support for ForceEmptyHitRegion on the WR hit-test. r=botond
https://hg.mozilla.org/integration/autoland/rev/bef9f8968a42
Ensure EventRegionOverride flags don't end up on the HTTNs for reflayers. r=botond
https://hg.mozilla.org/integration/autoland/rev/c814b414cd82
Ensure we still check the flags on the node for the NULL_SCROLL_ID case. r=botond
https://hg.mozilla.org/integration/autoland/rev/02639d0327aa
Add a test for the ForceEmptyHitRegion flag. r=botond

Comment 10

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/b09ad6ab0220
https://hg.mozilla.org/mozilla-central/rev/97cb47649452
https://hg.mozilla.org/mozilla-central/rev/bef9f8968a42
https://hg.mozilla.org/mozilla-central/rev/c814b414cd82
https://hg.mozilla.org/mozilla-central/rev/02639d0327aa