1519542 - Fission: Support scroll wheel scrolling for OOP frames

Status: RESOLVED FIXED

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

Description

[Jonathan Watt [:jwatt]]

When a user uses their scroll wheel to scroll, we need to decide [which element in] which document to scroll if the pointer is currently over an iframe of arbitrary depth. Currently EventStateManager::ComputeScrollTargetAndMayAdjustWheelEvent walks the parent chain, and assumes that it can (synchronously) walk across document boundaries to determine the target. That will not be the case once we have OOP iframes.

Keeping this working won't be straightforward. Scrolling needs to be very responsive, so sending asynchronous messages up/down the parent chain to decide who is scrollable and who's at their scroll extends is not an option.

I brought this up with Kats and Botond at the Mozlando, and it sounded like they thought it would be possible to have oop-iframes record all their scroll frames in the compositor so that the compositor knows where to dispatch events and what to scroll.

I think there may be a bug on file about making sure APZ always handles scroll wheel events which would be good to fix prior to tackling this one, but I couldn't find one. There was some mention about needing to remove the pref layers.async-pan-zoom.enabled first (still used for regression testing to check if regressions are due to APZ).

Comment 1

[Jonathan Watt [:jwatt]]

Botond, would you be able to investigate the solution we discussed in mozlando in a bit more in depth, and possibly even implement it? I think Nika hopes to have something stood up for bug 1500257 this week or not too long after.

Comment 2

[Botond Ballo [:botond]]

I think the description in comment 0 is conflating two different things.

The plan discussed in Orlando of having OOP iframes record information about all scroll frames in the compositor, would avoid the cross-document traversal done by this hit test in PrepareForSetTargetAPZCNotification.

The traversal in EventStateManager::ComputeScrollTargetAndMayAdjustWheelEvent is independent of that, and it's code I understand less well (it has to do with main-thread scrolling, not async scrolling). It needs more investigation before we can come up with a plan of action for it.

We should track these two issues in separate bugs.

(In reply to Jonathan Watt [:jwatt] from comment #1)

Botond, would you be able to investigate the solution we discussed in mozlando in a bit more in depth, and possibly even implement it?

Sure. Note that Nika has asked me to prototype some more basic compositor-based event targeting (figuring out which process to target without considering complications like scroll handoff), which I'll want to do first, and layer this on top.

Comment 3

[Botond Ballo [:botond]]

Compositor-based event targeting is being tracked in bug 1519412.

Comment 4

[Jonathan Watt [:jwatt]]

Given that only basic hit testing is the goal for Fission M2, I guess this should target M3?

Comment 5

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

(In reply to Botond Ballo [:botond] from comment #2)

The plan discussed in Orlando of having OOP iframes record information about all scroll frames in the compositor, would avoid the cross-document traversal done by this hit test in PrepareForSetTargetAPZCNotification.

Some discussion today revealed that there are cases where we still need a main-thread fallback to APZ hit-testing, and I filed bug 1541589 for that. Once that bug is fixed it will subsume this case in that the code in PrepareForSetTargetAPZCNotification linked above will be modified to no longer do the cross-document traversal.

The traversal in EventStateManager::ComputeScrollTargetAndMayAdjustWheelEvent is independent of that, and it's code I understand less well (it has to do with main-thread scrolling, not async scrolling). It needs more investigation before we can come up with a plan of action for it.

We can use this bug to track this issue, but it seems like we'll probably just force APZ handling of input events, and drop the main thread version of input-driven scrolling, which should get rid of ComputeScrollTargetAndMayAdjustWheelEvent entirely.

Comment 6

[Botond Ballo [:botond]]

Scroll wheel scrolling in OOP frames already works, due to work done elsewhere (such as bug 1528725).

We do have some edge cases to fix related to correct hit testing, which we are tracking in bug 1541589. (Those affect all types of events that require hit testing, not just scroll wheel.)

In addition, we want to remove the main-thread scroll path for wheel scrolling (tracked in bug 1541595), but its presence isn't actively harming in any way. (It's only activated if APZ is disabled by the user flipping the layers.async-pan-zoom.enabled pref, and even then we will at most get incorrect hit test results with Fission enabled.)

So, I think we can close this bug.

We should track bug 1541589 for Fission, though not necessarily for M3, as it concerns edge cases. I don't think there's a need to track bug 1541595, as the main-thread codepath isn't actively harming anyone.