Summary: | CSS Scroll Snap is not in effect when the user scrolls via the keyboard | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Šime Vidas <sime.vidas> | ||||||||||||
Component: | CSS | Assignee: | 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
Šime Vidas
2019-06-05 08:44:44 PDT
Created attachment 415539 [details]
Patch
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). (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. Created attachment 415885 [details]
Patch
Created attachment 415886 [details]
Diff from reviewed version of the change
Created attachment 415906 [details]
Patch
Created attachment 415979 [details]
Patch
Committed r270838: <https://trac.webkit.org/changeset/270838> All reviewed patches have been landed. Closing bug and clearing flags on attachment 415979 [details]. |