Summary: | [css-scroll-snap] Compute proximity information while snapping | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Martin Robinson <mrobinson> | ||||||||||
Component: | Scrolling | Assignee: | Martin Robinson <mrobinson> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | cmarcelo, ews-watchlist, fred.wang, jamesr, luiz, simon.fraser, tonikitoo, webkit-bug-importer | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 223021, 224380 | ||||||||||||
Attachments: |
|
Description
Martin Robinson
2021-04-08 05:57:48 PDT
Created attachment 425508 [details]
Patch
Created attachment 425591 [details]
Patch
Comment on attachment 425591 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=425591&action=review > Source/WebKit/UIProcess/RemoteLayerTree/ios/ScrollingTreeScrollingNodeDelegateIOS.mm:109 > + WebCore::FloatSize viewportSize(static_cast<float>(CGRectGetWidth([scrollView bounds])), static_cast<float>(CGRectGetHeight([scrollView bounds]))); I think you can construct a FloatSize from a CGSize, so this could be WebCore::FloatSize viewportSize = [scrollView bounds].size; Created attachment 427859 [details]
Patch
Committed r277083 (237386@main): <https://commits.webkit.org/237386@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 427859 [details]. Reopening to attach new patch. Created attachment 428836 [details]
Patch
Comment on attachment 428836 [details]
Patch
Marking patch obsolete. This was uploaded to the incorrect bug.
Comment on attachment 427859 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=427859&action=review > Source/WebCore/page/scrolling/ScrollSnapOffsetsInfo.cpp:157 > - if (velocity < 0) { > - if (lowerSnapOffsetRangeIndex == invalidSnapOffsetIndex || lowerSnapPosition >= snapOffsetRanges[lowerSnapOffsetRangeIndex].end) > - return std::make_pair(lowerSnapPosition, lowerIndex); > - if (!originalOffsetForDirectionalSnapping.hasValue() || *originalOffsetForDirectionalSnapping > upperSnapPosition) > - return std::make_pair(upperSnapPosition, upperIndex); > - } else { > - if (upperSnapOffsetRangeIndex == invalidSnapOffsetIndex || snapOffsetRanges[upperSnapOffsetRangeIndex].start >= upperSnapPosition) > + if (velocity < 0) { > + if (upperIndex != invalidSnapOffsetIndex && (!originalOffsetForDirectionalSnapping || *originalOffsetForDirectionalSnapping > upperSnapPosition)) > return std::make_pair(upperSnapPosition, upperIndex); > - if (!originalOffsetForDirectionalSnapping.hasValue() || *originalOffsetForDirectionalSnapping < lowerSnapPosition) > - return std::make_pair(lowerSnapPosition, lowerIndex); > + return std::make_pair(lowerSnapPosition, lowerIndex); This flipped the upper/lower ordering with velocity < 0. Was that intentional? (In reply to Simon Fraser (smfr) from comment #10) > Comment on attachment 427859 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=427859&action=review > > > Source/WebCore/page/scrolling/ScrollSnapOffsetsInfo.cpp:157 > > - if (velocity < 0) { > > - if (lowerSnapOffsetRangeIndex == invalidSnapOffsetIndex || lowerSnapPosition >= snapOffsetRanges[lowerSnapOffsetRangeIndex].end) > > - return std::make_pair(lowerSnapPosition, lowerIndex); > > - if (!originalOffsetForDirectionalSnapping.hasValue() || *originalOffsetForDirectionalSnapping > upperSnapPosition) > > - return std::make_pair(upperSnapPosition, upperIndex); > > - } else { > > - if (upperSnapOffsetRangeIndex == invalidSnapOffsetIndex || snapOffsetRanges[upperSnapOffsetRangeIndex].start >= upperSnapPosition) > > + if (velocity < 0) { > > + if (upperIndex != invalidSnapOffsetIndex && (!originalOffsetForDirectionalSnapping || *originalOffsetForDirectionalSnapping > upperSnapPosition)) > > return std::make_pair(upperSnapPosition, upperIndex); > > - if (!originalOffsetForDirectionalSnapping.hasValue() || *originalOffsetForDirectionalSnapping < lowerSnapPosition) > > - return std::make_pair(lowerSnapPosition, lowerIndex); > > + return std::make_pair(lowerSnapPosition, lowerIndex); > > This flipped the upper/lower ordering with velocity < 0. Was that > intentional? Actually it didn't, but this logic change makes it always return lowerSnapPosition even if upperSnapPosition is desired. (In reply to Simon Fraser (smfr) from comment #11) > (In reply to Simon Fraser (smfr) from comment #10) > > Comment on attachment 427859 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=427859&action=review > > > > > Source/WebCore/page/scrolling/ScrollSnapOffsetsInfo.cpp:157 > > > - if (velocity < 0) { > > > - if (lowerSnapOffsetRangeIndex == invalidSnapOffsetIndex || lowerSnapPosition >= snapOffsetRanges[lowerSnapOffsetRangeIndex].end) > > > - return std::make_pair(lowerSnapPosition, lowerIndex); > > > - if (!originalOffsetForDirectionalSnapping.hasValue() || *originalOffsetForDirectionalSnapping > upperSnapPosition) > > > - return std::make_pair(upperSnapPosition, upperIndex); > > > - } else { > > > - if (upperSnapOffsetRangeIndex == invalidSnapOffsetIndex || snapOffsetRanges[upperSnapOffsetRangeIndex].start >= upperSnapPosition) > > > + if (velocity < 0) { > > > + if (upperIndex != invalidSnapOffsetIndex && (!originalOffsetForDirectionalSnapping || *originalOffsetForDirectionalSnapping > upperSnapPosition)) > > > return std::make_pair(upperSnapPosition, upperIndex); > > > - if (!originalOffsetForDirectionalSnapping.hasValue() || *originalOffsetForDirectionalSnapping < lowerSnapPosition) > > > - return std::make_pair(lowerSnapPosition, lowerIndex); > > > + return std::make_pair(lowerSnapPosition, lowerIndex); > > > > This flipped the upper/lower ordering with velocity < 0. Was that > > intentional? > > Actually it didn't, but this logic change makes it always return > lowerSnapPosition even if upperSnapPosition is desired. Hrm. When scrolling with the keyboard, I am seeing all of these code paths being exercised. It's true though that this code is *very* confusing and the upper and lower snap position aren't guaranteed to be before or after the scroll destination. Sometimes they are even the same value. I have tried to address this confusing design in: https://bugs.webkit.org/show_bug.cgi?id=227062 |