Bug 191816

Summary: [css-scroll-snap] scroll-snap-align not honored on child with non-visible overflow
Product: WebKit Reporter: jonjohnjohnson <hi>
Component: Layout and RenderingAssignee: Frédéric Wang (:fredw) <fred.wang>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, commit-queue, fred.wang, simon.fraser, sparhami, webkit-bug-importer, wenson_hsieh, zalan
Priority: P2 Keywords: InRadar
Version: Safari Technology Preview   
Hardware: Macintosh   
OS: Unspecified   
Attachments:
Description Flags
Testcase
none
Patch
none
Patch for landing
none
Patch for landing none

Description jonjohnjohnson 2018-11-17 14:56:17 PST
cc wenson_hsieh@apple.com

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/
Comment 1 jonjohnjohnson 2018-11-17 15:06:10 PST
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
Comment 2 jonjohnjohnson 2018-11-17 15:15:37 PST
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.
Comment 3 Sepand Parhami 2018-11-27 15:06:08 PST
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/
Comment 4 jonjohnjohnson 2018-11-27 15:15:10 PST
Thanks sparhami@google.com 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.
Comment 5 jonjohnjohnson 2018-11-27 15:17:00 PST
Yes, if I change the css line 27 in https://jsfiddle.net/awq5u789/ to `overflow: auto`, the bug is still present.
Comment 6 jonjohnjohnson 2018-12-03 16:19:12 PST
wenson_hsieh@apple.com 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?
Comment 7 Frédéric Wang (:fredw) 2019-01-15 08:03:36 PST
Created attachment 359159 [details]
Testcase

A reduced testcase (not using flexbox, although I don't think that really matters).
Comment 8 Frédéric Wang (:fredw) 2019-01-15 08:14:46 PST
(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.
Comment 9 jonjohnjohnson 2019-01-15 08:22:08 PST
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.
Comment 10 jonjohnjohnson 2019-01-15 08:25:10 PST
(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.
Comment 11 jonjohnjohnson 2019-01-15 10:38:03 PST
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???
Comment 12 Frédéric Wang (:fredw) 2019-01-30 08:20:59 PST
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.
Comment 13 Frédéric Wang (:fredw) 2019-01-31 03:42:09 PST
Created attachment 360715 [details]
Patch
Comment 14 Wenson Hsieh 2019-01-31 09:50:23 PST
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?
Comment 15 Frédéric Wang (:fredw) 2019-01-31 10:04:23 PST
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()?
Comment 16 Wenson Hsieh 2019-01-31 10:16:54 PST
(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.
Comment 17 Frédéric Wang (:fredw) 2019-01-31 11:56:09 PST
Created attachment 360748 [details]
Patch for landing
Comment 18 Simon Fraser (smfr) 2019-01-31 23:04:02 PST
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)?
Comment 19 Frédéric Wang (:fredw) 2019-02-01 00:15:54 PST
(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.
Comment 20 Frédéric Wang (:fredw) 2019-02-04 07:00:05 PST
Created attachment 361053 [details]
Patch for landing
Comment 21 WebKit Commit Bot 2019-02-04 07:39:53 PST
Comment on attachment 361053 [details]
Patch for landing

Clearing flags on attachment: 361053

Committed r240921: <https://trac.webkit.org/changeset/240921>
Comment 22 WebKit Commit Bot 2019-02-04 07:39:55 PST
All reviewed patches have been landed.  Closing bug.
Comment 23 Radar WebKit Bug Importer 2019-02-04 07:40:29 PST
<rdar://problem/47785869>