Summary: | [css-scroll-snap] scroll-snap-align not honored on child with non-visible overflow | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | jonjohnjohnson <hi> | ||||||||||
Component: | Layout and Rendering | Assignee: | 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: | Mac | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
jonjohnjohnson
2018-11-17 14:56:17 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 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. 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/ 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. Yes, if I change the css line 27 in https://jsfiddle.net/awq5u789/ to `overflow: auto`, the bug is still present. 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? Created attachment 359159 [details]
Testcase
A reduced testcase (not using flexbox, although I don't think that really matters).
(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. 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. (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. 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??? 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. Created attachment 360715 [details]
Patch
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 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()? (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. Created attachment 360748 [details]
Patch for landing
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)? (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. Created attachment 361053 [details]
Patch for landing
Comment on attachment 361053 [details] Patch for landing Clearing flags on attachment: 361053 Committed r240921: <https://trac.webkit.org/changeset/240921> All reviewed patches have been landed. Closing bug. |