Bug 198566 – CSS Scroll Snap is not in effect when the user scrolls via the keyboard

• Component: CSS
• Status: RESOLVED FIXED

[Šime Vidas]

1. Open this page https://output.jsbin.com/yugijov/quiet
2. Scroll the page by pressing the spacebar or arrow down keys

Actual results:

Safari scrolls the page without taking the scroll snap positions into account.

Expected results:

When pressing the spacebar or arrow down keys, the page should scroll to the next scroll snap position.

[Radar WebKit Bug Importer]

<rdar://problem/51488088>

[Martin Robinson]

Created attachment 415539 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 415539 [details]
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).

[Martin Robinson]

(In reply to Simon Fraser (smfr) from comment #3)
> Comment on attachment 415539 [details]
> Patch
> 

Thanks for the review!

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

This part was correct for horizontal scrolling in RTL, but the caller was incorrect. I've fixed that and added a test.

> 
> > 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?

Okay. I've done this.

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


I added some tests...and discovered a bug here. I've fixed that and included the tests in the change.

[Martin Robinson]

Created attachment 415885 [details]
Patch

[Martin Robinson]

Created attachment 415886 [details]
Diff from reviewed version of the change

[Martin Robinson]

Created attachment 415906 [details]
Patch

[Martin Robinson]

Created attachment 415979 [details]
Patch

[EWS]

Committed r270838: <https://trac.webkit.org/changeset/270838>

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