Bug 198566 - CSS Scroll Snap is not in effect when the user scrolls via the keyboard
Summary: CSS Scroll Snap is not in effect when the user scrolls via the keyboard
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Martin Robinson
URL:
Keywords: InRadar
Depends on:
Blocks: 218115
  Show dependency treegraph
 
Reported: 2019-06-05 08:44 PDT by Šime Vidas
Modified: 2020-12-15 06:34 PST (History)
12 users (show)

See Also:


Attachments
Patch (38.57 KB, patch)
2020-12-07 01:41 PST, Martin Robinson
no flags Details | Formatted Diff | Diff
Patch (60.85 KB, patch)
2020-12-10 09:34 PST, Martin Robinson
no flags Details | Formatted Diff | Diff
Diff from reviewed version of the change (33.11 KB, patch)
2020-12-10 09:35 PST, Martin Robinson
no flags Details | Formatted Diff | Diff
Patch (60.93 KB, patch)
2020-12-10 11:28 PST, Martin Robinson
no flags Details | Formatted Diff | Diff
Patch (63.02 KB, patch)
2020-12-11 02:10 PST, Martin Robinson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Šime Vidas 2019-06-05 08:44:44 PDT
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.
Comment 1 Radar WebKit Bug Importer 2019-06-06 10:09:37 PDT
<rdar://problem/51488088>
Comment 2 Martin Robinson 2020-12-07 01:41:55 PST
Created attachment 415539 [details]
Patch
Comment 3 Simon Fraser (smfr) 2020-12-07 08:52:01 PST
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).
Comment 4 Martin Robinson 2020-12-10 09:33:24 PST
(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.
Comment 5 Martin Robinson 2020-12-10 09:34:39 PST
Created attachment 415885 [details]
Patch
Comment 6 Martin Robinson 2020-12-10 09:35:26 PST
Created attachment 415886 [details]
Diff from reviewed version of the change
Comment 7 Martin Robinson 2020-12-10 11:28:50 PST
Created attachment 415906 [details]
Patch
Comment 8 Martin Robinson 2020-12-11 02:10:23 PST
Created attachment 415979 [details]
Patch
Comment 9 EWS 2020-12-15 06:34:53 PST
Committed r270838: <https://trac.webkit.org/changeset/270838>

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