2019912 - position:sticky scrollIntoView in a scroll event listener stops the ongoing scroll if the element is already in the view

Status: RESOLVED FIXED

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

Description

[Kagami Rosylight 🏳️‍🌈🏳️‍⚧️ [:saschanaz] (they/them)]

Attachment: 2012743.html

1. Open the attachment
2. Try scrolling

Chrome: Scroll feels fast and smooth
Firefox: Scroll is slow

Comment 1

[Hiroyuki Ikezoe (:hiro)]

I believe this is position:sticky specific. We do basically bailout from scrollIntoView relevant functions if the destination is same as before;

https://searchfox.org/firefox-main/rev/3c918058f580ec2cd39d80c9f77ccd06982c0029/layout/base/PresShell.cpp#3567-3571

// If we don't need to scroll, then don't try since it might cancel
// a current smooth scroll operation.
if (scrollPt == originalScrollPt) {
  return Nothing();
}

But in position:sticky case, the calculation of the destination is not so smart I guess.

Setting P3:S3 since the user impact score is quite low, 40.

Comment 2

[Kagami Rosylight 🏳️‍🌈🏳️‍⚧️ [:saschanaz] (they/them)]

And then it became 200...

Comment 3

[David Shin[:dshin]]

Attachment: Modified Testcase

So it is sticky-specific in the sense that the code path isn't really taken e.g. for the fixed element, but the root cause is with the computation of horizontal scroll.
We end up trying to center the scroll target horizontally, and this means that we end up with a negative scroll offset. This causes the bailout code in Bug 2019912 Comment 1 to be skipped.

I've modified the testcase's scroll target, #target, to use a display: block element (i.e. <div>) - With width: fit-content, centering the target would involve scrolling left, which computes as a negative scroll offset. This prevents the bailout, leading to the buggy behaviour because originalScrollPt is based on GetVisualViewportOffset(), which wouldn't include this negative scroll offset. OTOH, setting width: 100vw causes the target horizontal scroll offset 0, avoiding the buggy behaviour.

I would think that the the scroll offsets should be clamped to prospective min/max values, but we in fact force the range to include the computed offset, which is curious. It points to bug 681192, which I will have to investigate a bit.

Comment 4

[David Shin[:dshin]]

Attachment: Bug 2019912: Clamp scroll offset to the visual scroll range. r=#layout

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/505d06ba2fd0
https://hg.mozilla.org/integration/autoland/rev/97f56cb76710
Clamp scroll offset to the visual scroll range. r=layout-reviewers,emilio

Comment 6

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/58718 for changes under testing/web-platform/tests

Comment 7

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/97f56cb76710

Comment 8

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 9

[Frank Griffith Jr]

Thank again David!