Closed Bug 1379458 Opened 7 years ago Closed 1 year ago

scroll gets handed off from `position: fixed` element to container

Categories

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

Product:
Core
Component:
Panning and Zooming
Version:
56 Branch
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
110 Branch
Tracking Flags:
Tracking Status
firefox55 --- wontfix
firefox56 --- wontfix
firefox57 --- wontfix
firefox110 --- verified

People

(Reporter: 709922234, Assigned: dlrobertson)

References

(
URL
)

Details

(Keywords: correctness, Whiteboard: [gfx-noted])

Attachments

(7 files)

1.gif
131.03 KB, image/gif
Details
Bug 1379458 - Add test group for overscroll handoff. r=tnikkel
48 bytes, text/x-phabricator-request
Details | Review
Bug 1379458 - The ScrollParentId should match the ASR tree. r=tnikkel
48 bytes, text/x-phabricator-request
Details | Review
Testcase 1
425 bytes, text/html
Details
Testcase 2
348 bytes, text/html
Details
Testcase 3
480 bytes, text/html
Details
Bug 1379458 - Remove the unused SCROLLABLE flag. r=tnikkel
48 bytes, text/x-phabricator-request
Details | Review
Attached image 1.gif — 
User Agent: Mozilla/5.0 (Windows NT 6.1; rv:56.0) Gecko/20100101 Firefox/56.0
Build ID: 20170708030206

Steps to reproduce:

1. open https://codepen.io/mantou132/pen/MoPrYZ
2. scroll list


Actual results:

<main> element scroll


Expected results:

<main> element not scroll
Component: Untriaged → Layout
Product: Firefox → Core
Firefox 54 is expected results
I reproduce the same behaviour in 54 all the way back to 45 and current nightly. So it doesn't seem that anything changed.

I'm guessing this is probably because the scroll ancestor of the fixed pos content is the root scroll frame. In this case we shouldn't be scrolling the root scroll frame because it's overflow hidden.
Priority: -- → P3
(In reply to Timothy Nikkel (:tnikkel) from comment #2)
> I reproduce the same behaviour in 54 all the way back to 45 and current
> nightly. So it doesn't seem that anything changed.

What I'm seeing is that with APZ, scroll is handed off from the <dialog> element to the <main> element, but without APZ, it's not. So APZ has changed the scroll handoff behaviour.

> I'm guessing this is probably because the scroll ancestor of the fixed pos
> content is the root scroll frame. In this case we shouldn't be scrolling the
> root scroll frame because it's overflow hidden.

The thing that the scroll is being handed off to is the <main> element, not the RSF. The <main> element is overflow:auto, so scroll handoff targeting it is expected.

The question, then, is why the scroll _wasn't_ handed off pre-APZ. My guess is it has to do with the <dialog> being position:fixed.
Component: Layout → Panning and Zooming
Keywords: correctness
Whiteboard: [gfx-noted]
Neither Chrome nor Safari do the handoff so this is likely a Firefox (and therefore APZ) bug.
Status: UNCONFIRMED → NEW
status-firefox55: --- → wontfix
status-firefox56: --- → fix-optional
status-firefox57: --- → affected
Ever confirmed: true
OS: Unspecified → All
Hardware: Unspecified → All
Summary: not should automatically scroll `position: fixed` container of the parent element → scroll gets handed off from `position: fixed` element to container
This property can solve the problem

https://bugzilla.mozilla.org/show_bug.cgi?id=951793
(In reply to 709922234 from comment #5)
> This property can solve the problem
> 
> https://bugzilla.mozilla.org/show_bug.cgi?id=951793

It can be used as a workaround, yes. However, it looks like there is a bug in our default behaviour, and that should be fixed too (which is what this bug tracks).
Assignee: nobody → drobertson
Attachment #9280274 - Attachment description: Bug 1379458 - Add test case for overscroll handoff of a fixed element. r=botond → Bug 1379458 - Add test group for overscroll handoff. r=botond

Bug 1693451 (which has a prototype patch by Timothy attached to it) might be relevant here.

See Also: → 1693451
Attached file Testcase 1 —
Attached file Testcase 2 —
Attached file Testcase 3 —

I attached some testcases to help us think through the various considerations here and arrive at a correct fix.

Brief descriptions:

Testcase 1

This is basically a simplified version of the testcase in the original report. The important features are:

  • the fixed element is itself a scroll frame
  • the fixed element has an ancestor (in the DOM) which is scrollable but isn't the <html>

Testcase 2

This is like testcase 1, but the fixed element's scrollable ancestor is the <html>.

Testcase 3

This is like testcase 1, but the fixed element itself isn't a scroll frame; rather, it contains a scroll frame descendant (and we're interested in the handoff behaviour of that descendant).

Now comparing the behaviour on the three testcases:

Chrome Firefox (status quo) Firefox (with current patch)
Testcase 1 no handoff handoff no handoff
Testcase 2 handoff handoff no handoff
Testcase 3 no handoff handoff handoff

If we take Chrome's behaviour as the desired state, then the current patch fixes testcase 1 as intended, but it breaks testcase 2 (prevents handoff to the root when it shouldn't), and does not fix testcase 3 (it only handles the case where the fixed element is itself a scroll frame).

Here's the conceptual model behind the desired behaviour: when an element is fixed, it's taken "out of flow", and, in the frame tree, becomes a descendant of the viewport rather than a descendant of (the frame for) its in-flow (DOM) ancestor.

Scroll handoff is intended to follow the relationships in the frame tree, which is why we should not hand off from a fixed element (or its descendants) to its non-root DOM ancestors, but should still hand off from a fixed element to the viewport (root).

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

Here's the conceptual model behind the desired behaviour: when an element is fixed, it's taken "out of flow", and, in the frame tree, becomes a descendant of the viewport rather than a descendant of (the frame for) its in-flow (DOM) ancestor.

Scroll handoff is intended to follow the relationships in the frame tree, which is why we should not hand off from a fixed element (or its descendants) to its non-root DOM ancestors, but should still hand off from a fixed element to the viewport (root).

Ah! This is very helpful!

Attachment #9282279 - Attachment description: Bug 1379458 - Do not handoff overscroll from fixed elements. r=botond → Bug 1379458 - The ScrollParentId should match the ASR tree. r=botond
Attachment #9280274 - Attachment description: Bug 1379458 - Add test group for overscroll handoff. r=botond → Bug 1379458 - Add test group for overscroll handoff. r=tnikkel
Attachment #9282279 - Attachment description: Bug 1379458 - The ScrollParentId should match the ASR tree. r=botond → Bug 1379458 - The ScrollParentId should match the ASR tree. r=tnikkel

The following patch is waiting for review from an inactive reviewer:

ID Title Author Reviewer Status
D149925 Bug 1379458 - The ScrollParentId should match the ASR tree. r=tnikkel dlrobertson botond: Back Aug 13, 2022

:dlrobertson, could you please find another reviewer?

For more information, please visit auto_nag documentation.

Flags: needinfo?(drobertson)

Bad bot, that doesn't make sense, he's just on PTO.

Flags: needinfo?(drobertson)

Remove the unused SCROLLABLE_FOLLOW_OOF_TO_PLACEHOLDER flag now that ASRs should
match the mScrollParentId.

Depends on D149925

Severity: normal → S3
Pushed by drobertson@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e1347c761038
Add test group for overscroll handoff. r=botond
https://hg.mozilla.org/integration/autoland/rev/54494d5e152c
The ScrollParentId should match the ASR tree. r=botond,tnikkel,mstange
https://hg.mozilla.org/integration/autoland/rev/1d37427ef6f2
Remove the unused SCROLLABLE flag. r=botond

Backed out for causing reftest failures on nsDisplayList.cpp

  • Backout link
  • Push with failures
  • Failure Log
  • Failure line: Assertion failure: false (item should have finite clip with respect to aASR), at /builds/worker/checkouts/gecko/layout/painting/nsDisplayList.cpp:2662
Flags: needinfo?(drobertson)

(In reply to Cristian Tuns from comment #21)

Backed out for causing reftest failures on nsDisplayList.cpp

  • Backout link
  • Push with failures
  • Failure Log
  • Failure line: Assertion failure: false (item should have finite clip with respect to aASR), at /builds/worker/checkouts/gecko/layout/painting/nsDisplayList.cpp:2662

Ran a ton of tests today, and I've yet to see the failure linked in any of the try pushes or local tests. I could be wrong, but I think the failure linked was due to https://hg.mozilla.org/integration/autoland/rev/4dfea46f3cd01c2134c9f1034136e3a065cc81cd. I'm going to re-land the change. Please back out the patch if I've misunderstood something.

Flags: needinfo?(drobertson) → needinfo?(ctuns)
Pushed by drobertson@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/61a605c335b0
Add test group for overscroll handoff. r=botond
https://hg.mozilla.org/integration/autoland/rev/3239f01e81d7
The ScrollParentId should match the ASR tree. r=botond,tnikkel,mstange
https://hg.mozilla.org/integration/autoland/rev/dd6888957eff
Remove the unused SCROLLABLE flag. r=botond

Backed out 1 changesets (Bug 1379458) for causing devtools failures on nsGfxScrollFrame.cpp.
Backout link
Push with failures <--> dt2
Failure Log

Flags: needinfo?(drobertson)

(In reply to Marian-Vasile Laza from comment #24)

Backed out 1 changesets (Bug 1379458) for causing devtools failures on nsGfxScrollFrame.cpp.
Backout link
Push with failures <--> dt2
Failure Log

Thanks! It appears there is a bug in AutoCurrentActiveScrolledRootSetter::SetCurrentScrollParentId, where we set mCanBeScrollParent when we shouldn't. I think this can be fixed by ensuring that mOldScrollParentId is the old mBuilder->mCurrentScrollParentId when the setter is called.

Flags: needinfo?(drobertson)
Flags: needinfo?(ctuns)

I figured out the cause of us hitting the assert, but I've got some more testing to do before I push the change for review.

Flags: needinfo?(drobertson)
Pushed by drobertson@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f94a03948385
Add test group for overscroll handoff. r=botond
https://hg.mozilla.org/integration/autoland/rev/58f50284310b
The ScrollParentId should match the ASR tree. r=botond,tnikkel,mstange
https://hg.mozilla.org/integration/autoland/rev/4681951d59b7
Remove the unused SCROLLABLE flag. r=botond

https://hg.mozilla.org/mozilla-central/rev/f94a03948385
https://hg.mozilla.org/mozilla-central/rev/58f50284310b
https://hg.mozilla.org/mozilla-central/rev/4681951d59b7

Status: NEW → RESOLVED
Closed: 1 year ago
status-firefox110: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 110 Branch
Regressions: 1808833
Regressions: 1809574
QA Whiteboard: [qa-110b-p2]

Reproduced the issue on Firefox 70.0 on macOS 13.1 by following the STR from Comment 0 and the testcases provided by Botond.

The behaviors are now as follows on Firefox 110.0b3:

  1. TC from original post has no handoff
  2. Testcase 1 and 3 has no handoff
  3. Testcase 2 has the handoff

Tests were performed on macOS 13.1, Windows 11 and Ubuntu 22.04.

QA Whiteboard: [qa-110b-p2]
Status: RESOLVED → VERIFIED
status-firefox110: fixed → verified
Duplicate of this bug: 1693451
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: