1704503 - [Proton] The overscroll effect ignores site fixed headers (with WebRender)

Status: VERIFIED FIXED

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

Description

[Timea Cernea [:tbabos][inactive]]

Affected Versions:
Nightly 89.0a1
apz.overscroll.enabled true

Tested On:
MacOS 10.15/11

Steps to Reproduce:

1. Go to any page that has sticky headers or side menus, for example sites like:

• https://youtube.com
• https://facebook.com (news feed page)
• https://pinterest.com
• https://instagram.com

2. Reach the top of the page and swipe up multiple times

Expected Results:
The overscroll effect should start above the site header.

Actual Result:
The site header remains in place and the overscroll effect is applied under it.
Recording: https://drive.google.com/file/d/1PC3k2uFE2fMUBglqaHF5eotwBZ5jA4NZ/view?usp=sharing

Not reproducible on Safari nor Chrome.

Severity Suggestion:
Marking this S2 given it affects a large number of sites that have sticky headers and the whole rubberbanding effect is no longer applied correctly for these sites. Feel free to change it if you consider otherwise.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Attachment: A test case

It seems that fixed:position elements fixed to the root scroller are not overscrolled.

In this test case, there are two position:fixed elements, one is green box which is fixed to a child scroller of the root one, the other one is red box fixed to the root one. The green one works, the red one doesn't work properly.

I haven't checked position:sticky cases.

Comment 2

[Timothy Nikkel (:tnikkel)]

I don't think the green one would be considered position fixed by any of our code, it'd be considered absolute.

Comment 3

[Botond Ballo [:botond]]

I think we understand why this happens with WR: the overscroll transform is applied to the async scroll offset, which is not applied to fixed nodes.

I don't think we understand yet why this happens with non-WR.

Comment 4

[Botond Ballo [:botond]]

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

I don't think we understand yet why this happens with non-WR.

Ah, I think I know: this call should be GetCurrentAsyncTransformWithOverscroll.

Comment 5

[Botond Ballo [:botond]]

Per discussion with Martin, this is not a release blocker, but it's a nice-to-have and we'd like to try and fix it before release.

Comment 6

[Botond Ballo [:botond]]

Attachment: Bug 1704503 - Apply the overscroll transform to content fixed to the root (non-WebRender). r=tnikkel

This is accomplished by the overscroll transform being part of the visual
component of the root content APZC's async transform.

We were already doing this right in AsyncPanZoomController (as of bug
1699868), but the call sites in AsyncCompositionManager needed to be
updated.

Comment 7

[Botond Ballo [:botond]]

I wrote up the non-WR fix for position:fixed as it was simple. The WR fix will be more involved (requiring similar things as bug 1701831).

I haven't been able to reproduce any issue with position:sticky. The headers of all the sites mentioned in comment 0 are position:fixed, not position:sticky, and I haven't been able to construct a testcase that shows a problem, either.

Comment 8

[Timea Cernea [:tbabos][inactive]]

Re-checked the sites I mentioned and additional ones to confirm that the position:fixed is indeed where the issue is reproducible. Sorry for the confusion, did the research to understand the difference between sticky and fixed (didn't know there is one). Updating the title to reflect that.

Comment 9

[Phabricator Automation]

Comment on attachment 9216140 [details]
Bug 1704503 - Apply the overscroll transform to content fixed to the root (non-WebRender). r=tnikkel

Revision D112221 was moved to bug 1705826. Setting attachment 9216140 [details] to obsolete.

Comment 10

[Botond Ballo [:botond]]

I moved the non-WebRender fix to bug 1705826, and will use this bug to track the WebRender fix.

Comment 12

[Botond Ballo [:botond]]

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

The WR fix will be more involved (requiring similar things as bug 1701831).

I've thought of a way to fix this for WR that doesn't require bug 1701831.

Comment 13

[Botond Ballo [:botond]]

Attachment: Bug 1704503 - Add a reftest for subframe overscrolling. r=tnikkel

This is being added in this bug (which concerns position:fixed)
because a naive fix for this bug could regress subframe overscroll
rendering.

Comment 14

[Botond Ballo [:botond]]

Attachment: Bug 1704503 - Send the root content APZC's overscroll transform to WebRender as part of the zoom container's transform, not the APZC's scroll offset. r=tnikkel

This makes the handling of the overscroll transform consistent
with non-WebRender, including that it will now apply to content
fixed to the root.

Depends on D112877

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/44a218b908a4
Add a reftest for subframe overscrolling. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/9c6220f19543
Send the root content APZC's overscroll transform to WebRender as part of the zoom container's transform, not the APZC's scroll offset. r=tnikkel

Comment 16

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/44a218b908a4
https://hg.mozilla.org/mozilla-central/rev/9c6220f19543

Comment 17

[Andrei Purice]

Verified fixed on Nightly 90.0a1 (2021-04-21) (20210421095627) on MacOS 11.2.3 and Mac mini M1 11.0.1.

Comment 18

[Botond Ballo [:botond]]

Comment on attachment 9217277 [details]
Bug 1704503 - Send the root content APZC's overscroll transform to WebRender as part of the zoom container's transform, not the APZC's scroll offset. r=tnikkel

Beta/Release Uplift Approval Request

• User impact if declined: Overscroll effect will not apply to fixed content

Required for MR1 / Proton

• 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: Bug 1705826, Bug 1705927
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Patch is small and only affects rendering when the page is overscrolled
• String changes made/needed:

Comment 19

[Pascal Chevrel:pascalc]

Comment on attachment 9217277 [details]
Bug 1704503 - Send the root content APZC's overscroll transform to WebRender as part of the zoom container's transform, not the APZC's scroll offset. r=tnikkel

Approved for 89 beta 3, thanks.

Comment 20

[Pascal Chevrel:pascalc]

Comment on attachment 9217276 [details]
Bug 1704503 - Add a reftest for subframe overscrolling. r=tnikkel

Approved for 89 beta 3, thanks.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/2d0b54bdcfb2
https://hg.mozilla.org/releases/mozilla-beta/rev/fd7b8475c6b2

Comment 22

[Andrei Purice]

Verified fixed on Beta 98.0b3 (20210422190146) on MacOS 11.2.3.

Comment 23

[Hiroyuki Ikezoe (:hiro)]

FWIW I raised a spec issue whether position:fixed elements should be moved along with overscrolling or not.

Comment 24

[Hiroyuki Ikezoe (:hiro)]

I am also adding a link to WebKit's bug that they think the current their behavior, moving position:fixed elements during overscrolling, is a bug, they think it's a regression.

Comment 25

[Hiroyuki Ikezoe (:hiro)]

And for records, adding a link to the bug that we decided to move position:fixed elements in the B2G era.

Comment 26

[jonjohnjohnson]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #23)

FWIW I raised a spec issue whether position:fixed elements should be moved along with overscrolling or not.

Your testcase behaves differently if loaded in an iframe. It shows fixedpos elements not moving along with overscrolling.
https://bugzilla.mozilla.org/attachment.cgi?id=9215321

Though I personally would like firefox (as well as chrome & safari) change to match the safari behavior before their regression (primarily to enable custom overscroll stylings that are revealed from underneath fixedpos elements, like a header revealing a spining loader), I do think that scroll containers should at least be consistent in how the treat fixedpos with overscrolling, whether in a root scroller, random internal scroller or iframe scroller.

92.0a1 (2021-07-31) macOS 10.12.6