Closed Bug 1550207 Opened 5 years ago Closed 5 years ago

[wpt-sync] Sync PR 16395 - Snap correctly when scroll operation goes outside scroll bounds

Categories

(Core :: CSS Parsing and Computation, defect, P4)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla69
Tracking Flags:
Tracking Status
firefox69 --- fixed

People

(Reporter: mozilla.org, Unassigned)

References

(
URL
)

Details

(Whiteboard: [wptsync downstream])

Sync web-platform-tests PR 16395 into mozilla-central (this bug is closed when the sync is complete).

PR: https://github.com/web-platform-tests/wpt/pull/16395
Details from upstream follow.

Majid Valipour <majidvp@chromium.org> wrote:

Snap correctly when scroll operation goes outside scroll bounds

In the current snap selection algorithm there is an inherent assumption that the
selected snap position in the other axis is inside the scroller bounds. Breaking
this invariant in snap selection algorithm causes it to not find any valid
candidate.

In practice this invariant was being violated. More concretely an snap operation
with intended end and direction may have a base and intended snap positions
that are outside the bounds when for example script does
Element.scrollBy(-100,-100) or when user flings diagonally in a horizontal only
scroller.

This fix updates the logic so that instead of making the assumption we actively
enforce it by clamping the out-of-bound values. An alternative approach may have
been to perhaps sanitize the input from all the various entry points into the
snap selection logic but the current approach seems simpler and more robust.

Note that the problem applies to all snapping but manifested itself most
obviously when |scroll-snap-stop:always| was used because in normal snapping
even if fling didn't snap correctly we would re-snap at the end of the fling so
it was less obvious.

Other minor changes:

  • correct spelling mistake s/cros/cross/
  • reuse base::ClampToRange to clamp

TEST:

  1. wpt/css/css-scroll-snap/scroll-snap-stop-always.html: cover snapping with scroll-snap-stop
  2. wpt/css/css-scroll-snap/scrollTo-scrollBy-snaps.html: covers basic snapping
  3. fast/scroll-snap/animate-fling-to-snap-points.html: cover user fling snapping

Bug: 823998

Change-Id: I85ff8b2c87518358805064375e0386606aad7b66
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1572286
Commit-Queue: Majid Valipour \<majidvp@chromium.org>
Reviewed-by: Majid Valipour \<majidvp@chromium.org>
Reviewed-by: David Bokan \<bokan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#652185}

Component: web-platform-tests → CSS Parsing and Computation
Product: Testing → Core
Failed to get results from try push

I've commented in the PR that is that some of test cases might not sound right. https://github.com/web-platform-tests/wpt/pull/16395/commits/1477c4c0f7627d6c20ae78b882e9734683f3c985#r285857875

Pushed by james@hoppipolla.co.uk:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bdd3dc94b195
[wpt PR 16395] - Snap correctly when scroll operation goes outside scroll bounds, a=testonly

https://hg.mozilla.org/mozilla-central/rev/bdd3dc94b195

Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox69: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla69
You need to log in before you can comment on or make changes to this bug.