Bug 198566

Summary: CSS Scroll Snap is not in effect when the user scrolls via the keyboard
Product: WebKit Reporter: Šime Vidas <sime.vidas>
Component: CSSAssignee: Martin Robinson <mrobinson>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarcelo, ews-watchlist, fred.wang, jamesr, luiz, m.goleb+bugzilla, mrobinson, simon.fraser, thorton, tonikitoo, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 218115    
Attachments:
Description Flags
Patch
none
Patch
none
Diff from reviewed version of the change
none
Patch
none
Patch none

Šime Vidas
Reported 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.
Attachments
Patch (38.57 KB, patch)
2020-12-07 01:41 PST, Martin Robinson
no flags
Patch (60.85 KB, patch)
2020-12-10 09:34 PST, Martin Robinson
no flags
Diff from reviewed version of the change (33.11 KB, patch)
2020-12-10 09:35 PST, Martin Robinson
no flags
Patch (60.93 KB, patch)
2020-12-10 11:28 PST, Martin Robinson
no flags
Patch (63.02 KB, patch)
2020-12-11 02:10 PST, Martin Robinson
no flags
Radar WebKit Bug Importer
Comment 1 2019-06-06 10:09:37 PDT
Martin Robinson
Comment 2 2020-12-07 01:41:55 PST
Simon Fraser (smfr)
Comment 3 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).
Martin Robinson
Comment 4 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.
Martin Robinson
Comment 5 2020-12-10 09:34:39 PST
Martin Robinson
Comment 6 2020-12-10 09:35:26 PST
Created attachment 415886 [details] Diff from reviewed version of the change
Martin Robinson
Comment 7 2020-12-10 11:28:50 PST
Martin Robinson
Comment 8 2020-12-11 02:10:23 PST
EWS
Comment 9 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].
Note You need to log in before you can comment on or make changes to this bug.