1809574 - Incorrect scroll handoff from content fixed to subdocument

Status: VERIFIED FIXED

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

Description

[Botond Ballo [:botond]]

Attachment: Testcase

STR

1. Load the attached testcase
2. Position the cursor over the red element
3. Scroll the red element to the bottom
4. Continue scrolling and observe which element the scroll is handed off to

Expected results

The scroll is handed off to the green element (the iframe's root scroll frame), and only then (once the green element has also been scrolled to the bottom) the yellow element (the root document's root scroll frame).

This is the behaviour in Chrome, and the behaviour in Firefox prior to bug 1379458.

Actual results

The scroll is handed off directly to the yellow element (the root document's root scroll frame).

Comment 1

[Botond Ballo [:botond]]

I believe what's happening here is that the ASR tree does not reflect the desired handoff sequence -- specifically, the handoff from content fixed to a document, to the root scroll frame of that document.

The ASR assigned to content fixed to a document, is the ancestor of the ASR of the root scroll frame of that document.

If the document in question is the root document in its process, that ancestor will be null, which causes APZCs in the fixed subtree to get a null mScrollParentId. That gets "fixed up" in APZ by us taking this branch and then this branch, which gets us the root scroll frame as the handoff parent.

But if the document in question is not the root document in its process, the ancestor ASR will be the enclosing document's ASR, the corresponding mScrollParentId will not be null, and we'll just hand off directly to that enclosing scroll frame.

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1379458

:dlrobertson, since you are the author of the regressor, bug 1379458, could you take a look? Also, could you set the severity field?

For more information, please visit auto_nag documentation.

Comment 3

[Botond Ballo [:botond]]

Possible fix approach:

• Modify FindHandoffParent() to report whether its result was obtained by using fixed-pos lookup.
• In the BuildOverscrollHandoffChain() loop, call FindHandoffParent() unconditionally, and if it used fixed-pos lookup, prefer its result over GetScrollHandoffParentId().
• Otherwise, use GetScrollHandoffParentId().

This fix approach has the side effect of also giving us a nicer way to fix bug 1808833: if FindHandoffParent() did not use fixed-pos lookup, and we're not IsRootForLayersId(), we should still be able to warn/assert that we we have a non-null scroll handoff parent ID.

Other fix approaches are possible too, such as:

• Arrange for our scroll handoff parent ID to be what we want. (This would break its correspondence to ASRs, though in a different way than it was before bug 1379458.)
• (Maybe) just always use the FindHandoffParent()? I'm not clear on what the other possible differences are between it and the mScrollParentID, that would need to be investigated a bit. This is also less flexible in that it couples (modulo special branches like the fixed-pos lookup in GetTargetApzcForNode()) handoff to the hit-testing tree structure which reflects visual nesting relationships.

Comment 4

[Dan Robertson (:dlrobertson)]

Converted the testcase into a mochitest and started looking into a it.

Comment 5

[Dan Robertson (:dlrobertson)]

Attachment: Bug 1809574 - Use hit-testing tree to determine fixed position handoff. r=botond,hiro

Use the hit-testing tree to determine the APZC to handoff overscroll to
if the current APZC is in a fixed position subtree.

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1379458

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f5cade40880a
Use hit-testing tree to determine fixed position handoff. r=botond

Comment 8

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/f5cade40880a

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:dlrobertson, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox110 to wontfix.

For more information, please visit auto_nag documentation.

Comment 10

[Dan Robertson (:dlrobertson)]

Comment on attachment 9312174 [details]
Bug 1809574 - Use hit-testing tree to determine fixed position handoff. r=botond,hiro

Beta/Release Uplift Approval Request

• User impact if declined: A user scrolling a scrollable element in a position: fixed subtree within a subdocument would see the scroll handoff to the wrong root document.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This should only impact scroll handoff for scrollable elements within a position: fixed subtree.
• String changes made/needed:
• Is Android affected?: Yes

Comment 11

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9312174 [details]
Bug 1809574 - Use hit-testing tree to determine fixed position handoff. r=botond,hiro

Approved for 110 beta 4, thanks.

Comment 12

[Pascal Chevrel:pascalc (PTO until April 26)]

https://hg.mozilla.org/releases/mozilla-beta/rev/f1bed22e4390

Comment 13

[Giorgia Nichita, Release Desktop QA]

Reproduced this issue on an affected Nightly build from 2023-01-10, on Ubuntu 22.04, using the STR from the description.
Verified as fixed on Firefox 111.0a1 (20230125215201) and 110.0b5 (20230124185837) using Ubuntu 22.04, Win 10 and macOS 10.14.