[webkit-reviews] review granted: [Bug 145330] Scroll-snap points should be triggered during programmatic scroll : [Attachment 417448] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Martin Robinson
<mrobinson at webkit.org>'s request for review:
Bug 145330: Scroll-snap points should be triggered during programmatic scroll
https://bugs.webkit.org/show_bug.cgi?id=145330

Attachment 417448: Patch

https://bugs.webkit.org/attachment.cgi?id=417448&action=review




--- Comment #9 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 417448
  --> https://bugs.webkit.org/attachment.cgi?id=417448
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::Hori
zontal, newOffset.x(), velocity.width(), originalX));
> +   
newOffset.setY(m_scrollController.adjustScrollDestination(ScrollEventAxis::Vert
ical, 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(clampedSc
rollOffset, options.snapPointSelectionMethod));

IntPoint -> ScrollOffset