Bug 191816 – [css-scroll-snap] scroll-snap-align not honored on child with non-visible overflow

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[jonjohnjohnson]

cc [e-mail]

Testcase -> https://jsfiddle.net/jfmL13n6/

In the testcase, there is a grid of 4 scrollers. Each scroller scrolls horizontally, and *should have html/css that is spec compliant for snapping between two inner snapped positions. You can witness each scroll horizontally when using at least a trackpad, but only scrollers on the bottom half of the screen exhibit snapping. The bottom scrollers use an extra element between the snapport and the inner scrollers to declare the scroll-snap-align property. This is a remedy for the current bug, but *should not be needed. In the upper scrollers, the scroll-snap-align property is declared on the inner scrolling elements and no snapping is exhibited.

There is also a tangential issue with scroll-snap breaking scroll chaining from inner scrolling element that scroll in the same axis. This can be seen in the right side scrollers specifically, where all scrollers are horizontal.

Steps to reproduce:
1. Go to test case -> https://jsfiddle.net/jfmL13n6/
2. Scroll element in the lower left horizontal and notice snapping behaves as expected.
3. Scroll element in the upper left horizontal and notice no snapping.
4. Notice inner scrollers of upper and lower left can be scrolled vertically. If `overflow-y:scroll` was removed from the inner scrollers, the upper left would begin to snap correctly.

Tangiential issue steps:
5. Scroll the upper right scroller horizontally, through the inner scrollers so that the scroll boundaries are reached and "scroll chaining" behavior allows for movement of what should be the outer snapport.
6. Scroll the lower right scroller horizontally, it doesn't allow for "scroll chaining" because the inner scroller is in the same axis as the outer snapport.

Tip. Scroll with your pointer above the red borders to ensure you are scrolling the outer snapports. Scroll on the striped areas when ensuring correct scroll chaining.

Expectations:
1. Scrollers on the top half of the screen should snap just like the scrollers on the bottom half.
2. Scroll chaining shouldn't be broken between same axis scrolling of snap and non-snap scrollers.

Relevant texts
https://drafts.csswg.org/css-scroll-snap-1/
https://drafts.csswg.org/css-overscroll-behavior-1/

[jonjohnjohnson]

The bug was first discovered in this much less reduced test case, where '.view' both scrolls vertically and is attempting to create a snap position for it's immediate parents horizontal snapport by using a computed `scroll-snap-align: end`. https://jsbin.com/rutoceb/quiet

[jonjohnjohnson]

Also, it appears the original scroll-snap issue also happens in iOS safari. Though the tangential scroll chaining issue from the original test cases lower right scroller is not present on iOS, only showing when scrolling my trackpad in safari on macOS.

[Sepand Parhami]

Even if the child does not scroll, scroll-snap-align is not honored as long you have any overflow value other than visible.

Here is a page showing `overflow: hidden` prevents snapping.

https://jsfiddle.net/awq5u789/

[jonjohnjohnson]

Thanks [e-mail] for the much more isolated case.

Have a feeling this is tied to the age old behavior of `overflow: hidden` being considered a "scroll container" even if its "scrollport" and "scrollbox" have same dimensions, not overflowing, and don't provide programatic or user scrollability.

[jonjohnjohnson]

Yes, if I change the css line 27 in https://jsfiddle.net/awq5u789/ to `overflow: auto`, the bug is still present.

[jonjohnjohnson]

[e-mail] I recall you were working on rounding out scroll-snap, don't mean to bug, but willing to try n get your attention if this issue hasn't been pointed out anywhere else and needs to be changed from "new" to reproduced/observed in the tracker, so it may get assigned?

[Frédéric Wang (:fredw)]

Created attachment 359159 [details]
Testcase

A reduced testcase (not using flexbox, although I don't think that really matters).

[Frédéric Wang (:fredw)]

(In reply to jonjohnjohnson from comment #0)
> Tangiential issue steps:
> 5. Scroll the upper right scroller horizontally, through the inner scrollers
> so that the scroll boundaries are reached and "scroll chaining" behavior
> allows for movement of what should be the outer snapport.
> 6. Scroll the lower right scroller horizontally, it doesn't allow for
> "scroll chaining" because the inner scroller is in the same axis as the
> outer snapport.

I was not able to reproduce these issues with iPad / iOS 12.1 beta 4 (maybe I didn't understand them). Anyway, they should probably be moved into a separate bug entry.

I changed the bug title to reflect comment 3.

[jonjohnjohnson]

Regarding the title, "non-visible overflow" works to reference this in webkit, but across vendors things have been not as simple with the resolution for the `clip` https://drafts.csswg.org/css-overflow-3/#valdef-overflow-clip value not enabling the element to be a scroll container, like the visible value.

[jonjohnjohnson]

(In reply to Frédéric Wang (:fredw) from comment #8)
> I was not able to reproduce these issues with iPad / iOS 12.1 beta 4 (maybe
> I didn't understand them). Anyway, they should probably be moved into a
> separate bug entry.

I'll file it separately, but the bug isn't in iOS safari, only a desktop safari bug. Try those exact steps in latest STP to see the prevention of scroll chaining from the inner scroller because the outer scroller is a snapport.

[jonjohnjohnson]

I think the tangential bug spotted here is also the root of this other issue? 

https://bugs.webkit.org/show_bug.cgi?id=192344

Can you confirm for me if it's the case???

[Frédéric Wang (:fredw)]

OK, I finally got a chance to look into this this afternoon.

I see that the scroll snap offsets are not created for children with overflow != visible. Checking what's happening in updateScrollSnapOffsetForScrollableArea, the following seems dubious:

for (auto* child : scrollContainer->view().boxesWithScrollSnapPositions()) {
    if (child->findEnclosingScrollableContainer() != scrollContainer)
       continue;

For a child that is overflow scrollable, child->findEnclosingScrollableContainer() returns the child itself and hence the child is skipped in the for loop. I suspect findEnclosingScrollableContainer() is supposed to check only strict ancestors and not return the child itself. A quick one-line change makes the testcase behaves as expected. I will try to check more and upload a patch for review later.

[Frédéric Wang (:fredw)]

Created attachment 360715 [details]
Patch

[Wenson Hsieh]

Comment on attachment 360715 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=360715&action=review

> Source/WebCore/rendering/RenderLayerModelObject.cpp:222
> +        const RenderBox* scrollSnapBox = enclosingScrollableContainerForSnapping();

Nit - auto*?

> Source/WebCore/rendering/RenderObject.cpp:443
> +    auto* renderBox = &enclosingBox();

Nit - auto& renderBox = enclosingBox();, and use . instead of ->?

> Source/WebCore/rendering/RenderObject.cpp:447
> +            return renderBox->parentBox()->findEnclosingScrollableContainer();

Is it guaranteed that renderBox.parentBox() exists?

[Frédéric Wang (:fredw)]

Comment on attachment 360715 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=360715&action=review

>> Source/WebCore/rendering/RenderObject.cpp:447
>> +            return renderBox->parentBox()->findEnclosingScrollableContainer();
> 
> Is it guaranteed that renderBox.parentBox() exists?

I expected that it is true otherwise we won't be looking for the scrollable container for snapping (renderBox == this in this if clause). Maybe I can add an ASSERT to be sure? Or should I test parentBox()?

[Wenson Hsieh]

(In reply to Frédéric Wang (:fredw) from comment #15)
> Comment on attachment 360715 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=360715&action=review
> 
> >> Source/WebCore/rendering/RenderObject.cpp:447
> >> +            return renderBox->parentBox()->findEnclosingScrollableContainer();
> > 
> > Is it guaranteed that renderBox.parentBox() exists?
> 
> I expected that it is true otherwise we won't be looking for the scrollable
> container for snapping (renderBox == this in this if clause). Maybe I can
> add an ASSERT to be sure? Or should I test parentBox()?

Ah, I see. In this case, an assertion would be nice.

[Frédéric Wang (:fredw)]

Created attachment 360748 [details]
Patch for landing

[Simon Fraser (smfr)]

Comment on attachment 360715 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=360715&action=review

>>>> Source/WebCore/rendering/RenderObject.cpp:447
>>>> +            return renderBox->parentBox()->findEnclosingScrollableContainer();
>>> 
>>> Is it guaranteed that renderBox.parentBox() exists?
>> 
>> I expected that it is true otherwise we won't be looking for the scrollable container for snapping (renderBox == this in this if clause). Maybe I can add an ASSERT to be sure? Or should I test parentBox()?
> 
> Ah, I see. In this case, an assertion would be nice.

There's non point ASSERTing; this is going to crash anyway.

It's a bit odd that this is on RenderObject. Do we expect this to apply to all renderers (RenderText, SVG things)?

[Frédéric Wang (:fredw)]

(In reply to Simon Fraser (smfr) from comment #18)
> There's non point ASSERTing; this is going to crash anyway.
> 

OK, I think I should return nullprt if there is no parent box then.

> It's a bit odd that this is on RenderObject. Do we expect this to apply to
> all renderers (RenderText, SVG things)?

I guess Wenson can better reply to this but currently RenderView::boxesWithScrollSnapPositions() and similar register/unregister functions work with RenderBoxes.

[Frédéric Wang (:fredw)]

Created attachment 361053 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 361053 [details]
Patch for landing

Clearing flags on attachment: 361053

Committed r240921: <https://trac.webkit.org/changeset/240921>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/47785869>