Bug 145330 – Scroll-snap points should be triggered during programmatic scroll

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

[Brent Fulgham]

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."

[Brent Fulgham]

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.

[Radar WebKit Bug Importer]

<rdar://problem/21467780>

[Martin Robinson]

Created attachment 417363 [details]
Patch

[Martin Robinson]

Created attachment 417365 [details]
Patch

[Martin Robinson]

Created attachment 417371 [details]
Patch

[Simon Fraser (smfr)]

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

[Martin Robinson]

Created attachment 417448 [details]
Patch

[Martin Robinson]

(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.

[Simon Fraser (smfr)]

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

[Martin Robinson]

Created attachment 417520 [details]
Patch

[Martin Robinson]

(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.

[EWS]

Committed r271439: <https://trac.webkit.org/changeset/271439>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 417520 [details].

[Darin Adler]

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.