I've recently made a set of bug fixes that help the engine keep track of the current snap point, even after programmatic scrolls. For proper behavior, when a programmatic scroll occurs that will take us close to a snap point, we should drive an animation to that snap point. If the programmatic scroll was not sufficient to take us past, say, the midpoint between two snap points, we should roll back to the original snap point once the scroll operation completes. However, this would break manually animated scrolling operations. One possible answer to this problem is to say "Don't try to combine scroll-snap points and programmatic scrolling."
This is currently under discussion in the css working group. The current thought is that we should not invoke scroll snap points for programmatic scrolls, but we should add a 'snap' element to the ScrollOptions dictionary.
<rdar://problem/21467780>
Created attachment 417363 [details] Patch
Created attachment 417365 [details] Patch
Created attachment 417371 [details] Patch
Comment on attachment 417371 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=417371&action=review > Source/WebCore/dom/Element.cpp:991 > + FloatPoint originalPosition = FloatPoint( > adjustForAbsoluteZoom(renderer->scrollLeft(), *renderer), > adjustForAbsoluteZoom(renderer->scrollTop(), *renderer) > ); The better way to write this is auto originalPosition = FloatPoint { ... } > Source/WebCore/dom/Element.cpp:992 > + ScrollToOptions scrollToOptions = normalizeNonFiniteCoordinatesOrFallBackTo(options, originalPosition.x(), originalPosition.y()); auto > Source/WebCore/dom/Element.cpp:998 > + SetScrollPositionOptions setScrollPositionOptions = SetScrollPositionOptions::createProgrammatic(); auto > Source/WebCore/dom/Element.cpp:1003 > + if (snapPointSelectionMethod == ScrollSnapPointSelectionMethod::Directional) > + setScrollPositionOptions.enableDirectionalScrollSnapping(originalPosition); > + if (useSmoothScrolling(scrollToOptions.behavior.valueOr(ScrollBehavior::Auto), this)) > + setScrollPositionOptions.setAnimated(); Is this code that could live in a helper on ScrollableArea? > Source/WebCore/dom/Element.cpp:1333 > + auto options = SetScrollPositionOptions::createProgrammatic(); > + if (useSmoothScrolling(ScrollBehavior::Auto, this)) > + options.setAnimated(); Ditto. Like ScrollableArea::optionsForProgrammaticScroll(). > Source/WebCore/page/DOMWindow.cpp:1708 > + SetScrollPositionOptions setScrollPositionOptions = SetScrollPositionOptions::createProgrammatic(); auto. > Source/WebCore/page/FrameView.cpp:2331 > + ScrollOffset snappedOffset = ceiledIntPoint( > + scrollAnimator().adjustScrollOffsetForSnappingIfNeeded(scrollOffsetFromPosition(scrollPosition), delta, originalPosition)); Weird wrapping > Source/WebCore/platform/ScrollTypes.h:257 > +enum class ScrollSnapPointSelectionMethod : bool { I prefer uint8_t rather than bool. > Source/WebCore/platform/ScrollTypes.h:270 > +struct SetScrollPositionOptions { Having "Set" in the name makes this read like a function at the call sites. Maybe ScrollPositionChangeOptions? > Source/WebCore/platform/cocoa/ScrollController.mm:918 > + Optional<LayoutUnit> originalPositionInLayoutUnits = WTF::nullopt; No need to initialize. > Source/WebCore/rendering/RenderLayer.cpp:2849 > + SetScrollPositionOptions setScrollPositionOptions = SetScrollPositionOptions::createProgrammatic(); > + if (shouldUseAnimatedScroll(box->element(), options.behavior)) > + setScrollPositionOptions.setAnimated(); More sharable code. > Source/WebCore/rendering/RenderLayer.cpp:2884 > + SetScrollPositionOptions setScrollPositionOptions = SetScrollPositionOptions::createProgrammatic(); > + if (shouldUseAnimatedScroll(element, options.behavior)) > + setScrollPositionOptions.setAnimated(); Ditto > Source/WebCore/rendering/RenderLayer.cpp:2935 > + SetScrollPositionOptions setScrollPositionOptions = SetScrollPositionOptions::createProgrammatic(); > + if (shouldUseAnimatedScroll(element, options.behavior)) > + setScrollPositionOptions.setAnimated(); Ditto
Created attachment 417448 [details] Patch
(In reply to Simon Fraser (smfr) from comment #6) > Comment on attachment 417371 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=417371&action=review Thanks for the review! > > > Source/WebCore/dom/Element.cpp:991 > > + FloatPoint originalPosition = FloatPoint( > > adjustForAbsoluteZoom(renderer->scrollLeft(), *renderer), > > adjustForAbsoluteZoom(renderer->scrollTop(), *renderer) > > ); > > The better way to write this is auto originalPosition = FloatPoint { ... } I gave this a shot, but it seems that things inside curly initialisers and casts are subject to errors due to narrowing int into float. I've switched this to: FloatPoint originalPosition(...) > > Source/WebCore/dom/Element.cpp:992 > > + ScrollToOptions scrollToOptions = normalizeNonFiniteCoordinatesOrFallBackTo(options, originalPosition.x(), originalPosition.y()); > > auto Fixed. > > > Source/WebCore/dom/Element.cpp:998 > > + SetScrollPositionOptions setScrollPositionOptions = SetScrollPositionOptions::createProgrammatic(); > > auto Fixed. > > > Source/WebCore/dom/Element.cpp:1003 > > + if (snapPointSelectionMethod == ScrollSnapPointSelectionMethod::Directional) > > + setScrollPositionOptions.enableDirectionalScrollSnapping(originalPosition); > > + if (useSmoothScrolling(scrollToOptions.behavior.valueOr(ScrollBehavior::Auto), this)) > > + setScrollPositionOptions.setAnimated(); > > Is this code that could live in a helper on ScrollableArea? It's a little tricky to do a simple helper in ScrollableArea, but I reworked this a quite a bit and now I think there isn't any repeated code, apart from the call to useSmoothScrolling (which takes a different argument). > > > Source/WebCore/dom/Element.cpp:1333 > > + auto options = SetScrollPositionOptions::createProgrammatic(); > > + if (useSmoothScrolling(ScrollBehavior::Auto, this)) > > + options.setAnimated(); > > Ditto. Like ScrollableArea::optionsForProgrammaticScroll(). > > > Source/WebCore/page/DOMWindow.cpp:1708 > > + SetScrollPositionOptions setScrollPositionOptions = SetScrollPositionOptions::createProgrammatic(); > > auto. Fixed. > > > Source/WebCore/page/FrameView.cpp:2331 > > + ScrollOffset snappedOffset = ceiledIntPoint( > > + scrollAnimator().adjustScrollOffsetForSnappingIfNeeded(scrollOffsetFromPosition(scrollPosition), delta, originalPosition)); > > Weird wrapping Fixed. > > > Source/WebCore/platform/ScrollTypes.h:257 > > +enum class ScrollSnapPointSelectionMethod : bool { > > I prefer uint8_t rather than bool. Fixed. > > > Source/WebCore/platform/ScrollTypes.h:270 > > +struct SetScrollPositionOptions { > > Having "Set" in the name makes this read like a function at the call sites. > Maybe ScrollPositionChangeOptions? Sure. I'm not at all attached to the original name. I've renamed it to ScrollPositionChangeOptions. > > > Source/WebCore/platform/cocoa/ScrollController.mm:918 > > + Optional<LayoutUnit> originalPositionInLayoutUnits = WTF::nullopt; > > No need to initialize. Fixed. > > > Source/WebCore/rendering/RenderLayer.cpp:2849 > > + SetScrollPositionOptions setScrollPositionOptions = SetScrollPositionOptions::createProgrammatic(); > > + if (shouldUseAnimatedScroll(box->element(), options.behavior)) > > + setScrollPositionOptions.setAnimated(); > > More sharable code. Fixed.
Comment on attachment 417448 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=417448&action=review > Source/WebCore/platform/ScrollAnimator.cpp:375 > + Optional<float> originalX, originalY; > + FloatSize velocity; > + if (method == ScrollSnapPointSelectionMethod::Directional) { > + FloatSize scrollOrigin = toFloatSize(m_scrollableArea.scrollOrigin()); > + originalX = currentPosition().x(); > + originalY = currentPosition().y(); > + auto newPosition = ScrollableArea::scrollPositionFromOffset(offset, scrollOrigin); > + velocity = { newPosition.x() - currentPosition().x(), newPosition.y() - currentPosition().y() }; > + } > + > + FloatPoint newOffset = offset; > + newOffset.setX(m_scrollController.adjustScrollDestination(ScrollEventAxis::Horizontal, newOffset.x(), velocity.width(), originalX)); > + newOffset.setY(m_scrollController.adjustScrollDestination(ScrollEventAxis::Vertical, newOffset.y(), velocity.height(), originalY)); There's an unsetting mix of scrollOffset and scrollPosition here. originalX/originalY are positions (please name them thus), and are passed into m_scrollController.adjustScrollDestination(), but I can't tell if that actually treats them as positions or offsets. Please test in a horizontal RTL scroller. > Source/WebCore/platform/cocoa/ScrollController.mm:921 > + LayoutUnit offset = closestSnapOffset(snapState.snapOffsetsForAxis(axis), snapState.snapOffsetRangesForAxis(axis), LayoutUnit(destination / m_client.pageScaleFactor()), velocity, snapIndex, originalPositionInLayoutUnits); Does closestSnapOffset() treat originalPositionInLayoutUnits as a position or offset? > Source/WebCore/rendering/RenderLayer.cpp:2645 > + IntPoint snappedOffset = ceiledIntPoint(scrollAnimator().adjustScrollOffsetForSnappingIfNeeded(clampedScrollOffset, options.snapPointSelectionMethod)); IntPoint -> ScrollOffset
Created attachment 417520 [details] Patch
(In reply to Simon Fraser (smfr) from comment #9) > Comment on attachment 417448 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=417448&action=review > > > Source/WebCore/platform/ScrollAnimator.cpp:375 > > + Optional<float> originalX, originalY; > > + FloatSize velocity; > > + if (method == ScrollSnapPointSelectionMethod::Directional) { > > + FloatSize scrollOrigin = toFloatSize(m_scrollableArea.scrollOrigin()); > > + originalX = currentPosition().x(); > > + originalY = currentPosition().y(); > > + auto newPosition = ScrollableArea::scrollPositionFromOffset(offset, scrollOrigin); > > + velocity = { newPosition.x() - currentPosition().x(), newPosition.y() - currentPosition().y() }; > > + } > > + > > + FloatPoint newOffset = offset; > > + newOffset.setX(m_scrollController.adjustScrollDestination(ScrollEventAxis::Horizontal, newOffset.x(), velocity.width(), originalX)); > > + newOffset.setY(m_scrollController.adjustScrollDestination(ScrollEventAxis::Vertical, newOffset.y(), velocity.height(), originalY)); > > There's an unsetting mix of scrollOffset and scrollPosition here. > originalX/originalY are positions (please name them thus), and are passed > into m_scrollController.adjustScrollDestination(), but I can't tell if that > actually treats them as positions or offsets. Please test in a horizontal > RTL scroller. Agreed. After looking at this again, I think we should always be passing offsets here. I've done that and have modified all of the arguments and variable names down the call chain to make it clear that these are scroll offets. Additionally, I've written a test that verifies the correct scroll snap behavior for RTL scrollers when scrolling from script. That test is part of the patch now. > > Source/WebCore/rendering/RenderLayer.cpp:2645 > > + IntPoint snappedOffset = ceiledIntPoint(scrollAnimator().adjustScrollOffsetForSnappingIfNeeded(clampedScrollOffset, options.snapPointSelectionMethod)); > > IntPoint -> ScrollOffset Fixed.
Committed r271439: <https://trac.webkit.org/changeset/271439> All reviewed patches have been landed. Closing bug and clearing flags on attachment 417520 [details].
Comment on attachment 417520 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=417520&action=review > Source/WebCore/page/ScrollBehavior.cpp:41 > + // FIXME: Should we use document()->scrollingElement()? > + // See https://bugs.webkit.org/show_bug.cgi?id=205059 > + if (associatedElement == associatedElement->document().scrollingElement()) > + associatedElement = associatedElement->document().documentElement(); This is dereferencing associatedElement *before* it is null checked below.