Summary: | [css-scroll-snap] Don't snap to offscreen snap areas in unidirectional scrolls | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
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: | 228023, 228141 | ||||||||||
Bug Blocks: | 145099 | ||||||||||
Attachments: |
|
Description
Martin Robinson
2021-07-14 07:08:16 PDT
Created attachment 435192 [details]
Patch
Comment on attachment 435192 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=435192&action=review > Source/WebCore/page/scrolling/ScrollSnapOffsetsInfo.cpp:124 > +static void adjustPreviousAndNextForOnscreenSnapAreas(const InfoType& info, ScrollEventAxis axis, const SizeType& viewportSize, PointType destinationOffsetPoint, PotentialSnapPointSearchResult<UnitType>& searchResult) nit: OnScreen? > Source/WebCore/page/scrolling/ScrollSnapOffsetsInfo.cpp:135 > + } To avoid the type mismatch, I would rewrite this: unsigned oldIndex = (*searchResult.previous).second; searchResult.previous.reset(); for (unsigned i = 0; i <= oldIndex; i++) { const auto& snapOffset = snapOffsets[oldIndex - i]; if (hasCompatibleSnapArea(info, snapOffset, axis, viewportSize, destinationOffsetPoint)) { searchResult.previous = std::make_pair(snapOffset.offset, oldIndex - i); break; } } > Source/WebCore/page/scrolling/ScrollSnapOffsetsInfo.cpp:144 > + } and this unsigned oldIndex = (*searchResult.next).second; searchResult.next.reset(); for (unsigned i = oldIndex, i < snapOffsets.size(); i++) { const auto& snapOffset = snapOffsets[i]; if (hasCompatibleSnapArea(info, snapOffset, axis, viewportSize, destinationOffsetPoint)) { searchResult.next = std::make_pair(snapOffset.offset, i); break; } } > Source/WebCore/page/scrolling/ScrollSnapOffsetsInfo.cpp:162 > + adjustPreviousAndNextForOnscreenSnapAreas<InfoType, LayoutType, PointType, SizeType>(info, axis, viewportSize, scrollDestinationOffsetPoint, searchResult); OnScreen > LayoutTests/imported/w3c/ChangeLog:10 > + scrolling test no longer snaps because we don't have support for choosing between two candidates "no longer": it was already not passing before this change right? (In reply to Frédéric Wang (:fredw) from comment #3) > Comment on attachment 435192 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=435192&action=review > > > Source/WebCore/page/scrolling/ScrollSnapOffsetsInfo.cpp:124 > > +static void adjustPreviousAndNextForOnscreenSnapAreas(const InfoType& info, ScrollEventAxis axis, const SizeType& viewportSize, PointType destinationOffsetPoint, PotentialSnapPointSearchResult<UnitType>& searchResult) > > nit: OnScreen? Okay. It does seem like "OnScreen" is more common in WebKit source than "Onscreen." I'll change these. > > > Source/WebCore/page/scrolling/ScrollSnapOffsetsInfo.cpp:135 > > + } > > To avoid the type mismatch, I would rewrite this: > > unsigned oldIndex = (*searchResult.previous).second; > searchResult.previous.reset(); > for (unsigned i = 0; i <= oldIndex; i++) { > const auto& snapOffset = snapOffsets[oldIndex - i]; > if (hasCompatibleSnapArea(info, snapOffset, axis, viewportSize, > destinationOffsetPoint)) { > searchResult.previous = std::make_pair(snapOffset.offset, oldIndex - > i); > break; > } > } Hrm. I agree that the type mismatch isn't ideal, but the issue is that I want to avoid calling hasCompatibleSnapArea as much as possible since it's possible expensive. I can add a comment to this affect though. > > Source/WebCore/page/scrolling/ScrollSnapOffsetsInfo.cpp:144 > > + } > > and this > > unsigned oldIndex = (*searchResult.next).second; > searchResult.next.reset(); > for (unsigned i = oldIndex, i < snapOffsets.size(); i++) { > const auto& snapOffset = snapOffsets[i]; > if (hasCompatibleSnapArea(info, snapOffset, axis, viewportSize, > destinationOffsetPoint)) { > searchResult.next = std::make_pair(snapOffset.offset, i); > break; > } > } With this change, I think that snap areas that are further away would be selected over closer ones. I can adjust the code to make the oldIndex/index change you suggest though. > > > Source/WebCore/page/scrolling/ScrollSnapOffsetsInfo.cpp:162 > > + adjustPreviousAndNextForOnscreenSnapAreas<InfoType, LayoutType, PointType, SizeType>(info, axis, viewportSize, scrollDestinationOffsetPoint, searchResult); > > OnScreen > > > LayoutTests/imported/w3c/ChangeLog:10 > > + scrolling test no longer snaps because we don't have support for choosing between two candidates > > "no longer": it was already not passing before this change right? It was failing, but due to the fact that it snapped to the wrong location. Now it simply does not snap. Created attachment 435765 [details]
Patch
Created attachment 435766 [details]
Patch
After chatting with Frédéric, I better understand his suggestion and the latest version of the patch contains these. Committed r281189 (240633@main): <https://commits.webkit.org/240633@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 435766 [details]. |