[webkit-reviews] review granted: [Bug 198566] CSS Scroll Snap is not in effect when the user scrolls via the keyboard : [Attachment 415539] 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 198566: CSS Scroll Snap is not in effect when the user scrolls via the
keyboard
https://bugs.webkit.org/show_bug.cgi?id=198566

Attachment 415539: Patch

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




--- Comment #3 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 415539
  --> https://bugs.webkit.org/attachment.cgi?id=415539
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=415539&action=review

> Source/WebCore/page/scrolling/ScrollSnapOffsetsInfo.cpp:141
>      if (velocity < 0) {
> -	   if (lowerSnapOffsetRangeIndex != invalidSnapOffsetIndex &&
lowerSnapPosition < snapOffsetRanges[lowerSnapOffsetRangeIndex].end) {
> +	   if (lowerSnapOffsetRangeIndex == invalidSnapOffsetIndex ||
lowerSnapPosition >= snapOffsetRanges[lowerSnapOffsetRangeIndex].end) {
> +	       activeSnapIndex = lowerIndex;
> +	       return lowerSnapPosition;

Is this logic correct for horizontal scrolling in RTL? Please add a test to
ensure that it is.

> Source/WebCore/platform/ScrollAnimator.h:83
> +    bool scrollWithDirectionalSnapping(ScrollbarOrientation,
ScrollGranularity, float step, float multiplier);

Instead of a new function, can we just add a behavior enum to an existing
function?

> Source/WebCore/platform/cocoa/ScrollController.mm:919
> +    return closestSnapOffset(snapState.snapOffsetsForAxis(axis),
snapState.snapOffsetRangesForAxis(axis), LayoutUnit(destination /
m_client.pageScaleFactor()), velocity, snapIndex, LayoutUnit(originalPosition /
m_client.pageScaleFactor()));

Would be nice to have a test that exercises the "destination /
m_client.pageScaleFactor()" logic (i.e. zooms then snaps).