Closed Bug 1704503 Opened 4 years ago Closed 4 years ago

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

Categories

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

Desktop
macOS
defect

Tracking

()

VERIFIED FIXED
90 Branch
Tracking Status
firefox87 --- disabled
firefox88 --- disabled
firefox89 --- verified
firefox90 --- verified

People

(Reporter: tbabos, Assigned: botond)

References

(Blocks 3 open bugs)

Details

(Whiteboard: [mac:ux] [proton-platform] [proton-uplift])

Attachments

(3 files, 1 obsolete file)

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:
  1. 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.

Blocks: overscroll
Summary: [Proton] The overscroll effect ignores site sticky headers → [Proton] The overscroll effect ignores site fixed/sticky headers
Attached file A test case (deleted) —

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.

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

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.

(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.

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.

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.

Assignee: nobody → botond
Status: NEW → ASSIGNED

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.

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.

Summary: [Proton] The overscroll effect ignores site fixed/sticky headers → [Proton] The overscroll effect ignores site fixed headers
Whiteboard: [mac:ux] → [mac:ux] [proton-platform]
Depends on: 1705826

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.

Attachment #9216140 - Attachment is obsolete: true

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

Summary: [Proton] The overscroll effect ignores site fixed headers → [Proton] The overscroll effect ignores site fixed headers (with WebRender)
Priority: -- → P3

(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.

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

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

Pushed by bballo@mozilla.com: 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
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 90 Branch

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

Status: RESOLVED → VERIFIED
Whiteboard: [mac:ux] [proton-platform] → [mac:ux] [proton-platform] [proton-uplift]

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:
Attachment #9217277 - Flags: approval-mozilla-beta?
Attachment #9217276 - Flags: approval-mozilla-beta?

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.

Attachment #9217277 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

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

Approved for 89 beta 3, thanks.

Attachment #9217276 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

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

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

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.

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

(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

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: