Closed Bug 1560237 Opened 5 years ago Closed 5 years ago

position: sticky does not work with scroll-behavior and overflow

Categories

(Core :: Layout: Scrolling and Overflow, defect, P3)

Product:
Core
Component:
Layout: Scrolling and Overflow
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla69
Tracking Flags:
Tracking Status
firefox69 --- fixed

People

(Reporter: vinyldarkscratch, Assigned: hiro)

References

Details

(Keywords: reproducible)

Attachments

(4 files)

Example
(deleted), text/html
Details
Bug 1560237 - Introduce nsIScrollableFrame::IsSmoothScroll. r?botond
(deleted), text/x-phabricator-request
Details
Bug 1560237 - Rename ScrollFrameHelper::GetFrameForScrollSnap to ScrollFrameHelper::GetFrameForStyle. r?botond
(deleted), text/x-phabricator-request
Details
Bug 1560237 - Don't propagate scroll-behavior from <body>. r?botond
(deleted), text/x-phabricator-request
Details
Attached file Example (deleted) —

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_14_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/74.0.3729.169 Safari/537.36

Steps to reproduce:

When plugging position: sticky; into my own website (https://www.gooborg.com/team), I noticed that none of the elements with sticky positioning would...well, stick. After digging into the issue further, I found that if the following CSS is present, position: sticky; doesn’t function:

html {
scroll-behavior: smooth;
}

body {
overflow-x | overflow-y | overflow: auto | hidden | scroll | -moz-hidden-unscrollable;
}

It does not occur with one statement or the other. Both must be present to replicate the bug.

Actual results:

The elements behaved like relative positioning (aside from not listening to any of the top/left/bottom/right properties) and remained relative to the DOM.

Expected results:

The elements should have stuck to the top of the page. All other browsers function as expected.

Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: reproducible
Product: Firefox → Core
Component: Untriaged → Layout: Scrolling and Overflow

I think this is expected. scroll-behavior (or any other scrolling-related properties) on the root causes scroll propagation from the <body> to be prevented (that propagation is what generally allows you to set overflow on the body and make it work on the root).

If you mix them, you really have two scrolling boxes (the viewport, and the <body>), and thus sticky, which only works on the closest ancestor, doesn't stick to the viewport. You can see this on other browsers if you specify overflow-x: hidden both in the body and the html.

So tl;dr: if you want your scrolling-related properties to work on the viewport you should put them all on the <body>, or all on the <html> but you cannot mix them up (I'd choose the <html> fwiw).

Overflow propagation from the body is a nasty thing over-all, but it's needed for compat. It seems other browsers don't doesn't disable overflow propagation from the <body> when you specify scroll-behavior. That seems like a bug there, but I don't recall where the list of properties that decide overflow propagation is: https://drafts.csswg.org/css-overflow/#overflow-propagation doesn't contain an exhaustive list.

Hiro / Botond, do you know?

Flags: needinfo?(hikezoe)
Flags: needinfo?(botond)

It's hard to tell whether it's expected behavior from the perspective of web developers. Basically what Elimio is saying in comment 1 is correct. One caveat is that scroll-behavior is one of the properties which is not propagated from <body>. But we do propagate it as of now, and IIRC chrome also does. That's said, it's not yet clear to me that how it does affect to position:sticky elements. I will dig into detail once after I come back to home.

Flags: needinfo?(botond)

Ok, a problem related to scroll-behavior is that if html has scroll-behavior: smooth, the propagation stuff from <body> is broken. Stop propagating scroll-behavior will fix this particular issue. That's said, it seems the propagation stuff in CheckOverflow is broken regardless of scroll-behavior so we probably need fix it in another bug.

Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
Flags: needinfo?(hikezoe)
Priority: -- → P3

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

It's hard to tell whether it's expected behavior from the perspective of web developers. Basically what Elimio is saying in comment 1 is correct. One caveat is that scroll-behavior is one of the properties which is not propagated from <body>. But we do propagate it as of now, and IIRC chrome also does. That's said, it's not yet clear to me that how it does affect to position:sticky elements. I will dig into detail once after I come back to home.

Chrome does not actually propagate. In the duplicate bug 1481103, which describes the more distilled firefox issue of simply not propagating, there are links to the csswg issue and then a chrome ticket where they decided on wontfix for changing to this unspecified firefox behavior.

https://bugs.chromium.org/p/chromium/issues/detail?id=801857

But the sticky in nested scroll containers is a separate, larger issue https://github.com/w3c/csswg-drafts/issues/865, even if some wiggle room has been made for body overflow properties to be for the root scroller, when none of those same properties set the html as attempting root scroller propagation?

(In reply to jonjohnjohnson from comment #5)

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

It's hard to tell whether it's expected behavior from the perspective of web developers. Basically what Elimio is saying in comment 1 is correct. One caveat is that scroll-behavior is one of the properties which is not propagated from <body>. But we do propagate it as of now, and IIRC chrome also does. That's said, it's not yet clear to me that how it does affect to position:sticky elements. I will dig into detail once after I come back to home.

Chrome does not actually propagate. In the duplicate bug 1481103, which describes the more distilled firefox issue of simply not propagating, there are links to the csswg issue and then a chrome ticket where they decided on wontfix for changing to this unspecified firefox behavior.

Chrome doesn't propagate it? I actually see that they propagate it. Maybe their machinery is more complicated than I thought.

But the sticky in nested scroll containers is a separate, larger issue https://github.com/w3c/csswg-drafts/issues/865, even if some wiggle room has been made for body overflow properties to be for the root scroller, when none of those same properties set the html as attempting root scroller propagation?

Yeah, it seems to me that solving ISSUE 6 in the spec would make it clear. I am going to leave the issue here, I just will fix the propagation issue on scroll-behavior to fix the particular example in comment 0.

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

Chrome doesn't propagate it? I actually see that they propagate it. Maybe their machinery is more complicated than I thought.

Hmm, well in their wontfix resolved issue, https://bugs.chromium.org/p/chromium/issues/detail?id=801857, the test case shows behavior that doesn't exhibit propagation.

See in current chrome/canary - > https://jsfiddle.net/8fc2g0u0/1/

In that case, to get smooth scrolling behavior from clicking on the "to top" anchor, you'd not only have to set an overflowing height to the body element and set it's overflow to scroll, but you'd also have to set overflow to hidden on the html element. This might exhibit the more complicated approach of blink.

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

I am going to leave the issue here, I just will fix the propagation issue on scroll-behavior to fix the particular example in comment 0.

Actually if you're not solving this whole issue, it might make more sense to undup bug 1481103 and just fix that, leaving this with its sticky nested scroll containers angle? But that could ultimately just make this a dup of bug 1329203 haha.

This is pretty much the same as ScrollStyles::IsSmoothScroll right now,
but in the next commit, we will no longer propagate scroll-behavior on <body> to
the root element so that nsIScrollableFrame::IsSmoothScroll will be changed
to reflect it.

The function will also be used for scroll-behavior property in a later patch
in this commit series, so it needs a more reasonable name.

Depends on D35737

From the CSSOM View spec[1];

The scroll-behavior property of the HTML body element is not propagated to
the viewport.

The reason why this change fixes the test case in this commit is that we don't
have two different scrollable frames for <html> and <body> respectively if we
don't propagate scroll-behavior property from <body> to <html> so that we can
properly find the flow root of sticky position elements.

In other words, in the case where both of <html> and <body> have properties
that are propagated from <body> but they are different we have two scrollable
frames as a candidate of the 'flow root' for the sticky position element in
the test case, one is the scrollable frame for <html> and the other is the
scrollable frame for <body>. That means that
nsLayoutUtils::GetNearestScrollableFrame doesn't return what we want in some
places, for example we have a pretty similar issue in case of
overscroll-behavior which is bug 1561107.

Note that the test position-sticky-root-scroller-with-scroll-behavior.html is
almost copy-and-pasted from
/css/css-position/position-sticky-root-scroller.html [2] in wpt, the reason why
we put the test in /css/cssom-view is that there is a handy function to wait
for async scroll completion.

[1] https://drafts.csswg.org/cssom-view/#propdef-scroll-behavior
[2] https://searchfox.org/mozilla-central/rev/928742d3ea30e0eb4a8622d260041564d81a8468/testing/web-platform/tests/css/css-position/position-sticky-root-scroller.html

Depends on D35738

Pushed by hikezoe@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/065a45b8659d Introduce nsIScrollableFrame::IsSmoothScroll. r=botond https://hg.mozilla.org/integration/autoland/rev/a269192c5d30 Rename ScrollFrameHelper::GetFrameForScrollSnap to ScrollFrameHelper::GetFrameForStyle. r=botond https://hg.mozilla.org/integration/autoland/rev/9b96152c5e6e Don't propagate scroll-behavior from <body>. r=botond
Regressions: 1561802

https://hg.mozilla.org/mozilla-central/rev/065a45b8659d
https://hg.mozilla.org/mozilla-central/rev/a269192c5d30
https://hg.mozilla.org/mozilla-central/rev/9b96152c5e6e

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox69: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla69

== Change summary for alert #21685 (as of Mon, 01 Jul 2019 04:55:06 GMT) ==

Improvements:

35% tscrollx windows10-64-shippable opt e10s stylo 0.90 -> 0.59
16% tscrollx windows10-64-shippable-qr opt e10s stylo 0.99 -> 0.83
16% tscrollx windows10-64-shippable-qr opt e10s stylo 0.99 -> 0.83

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=21685

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/17695 for changes under testing/web-platform/tests
Regressions: 1562105
Regressions: 1568289

== Change summary for alert #21630 (as of Thu, 27 Jun 2019 10:43:11 GMT) ==

Improvements:

37% tscrollx linux64-shippable-qr opt e10s stylo 1.90 -> 1.20
35% tscrollx linux64-shippable opt e10s stylo 0.86 -> 0.56
32% tscrollx windows7-32-shippable opt e10s stylo 1.05 -> 0.71

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=21630

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

Attachment

General

Creator:
Created:
Updated:
Size: