2020558 - Investigate why overscroll-snap.html has started failing again on wpt.fyi since bug 2008571

Status: RESOLVED FIXED

(Core :: Panning and Zooming, task, P2)

Description

[Hiroyuki Ikezoe (:hiro)]

Here is a comparison view between the last success run and the first failure run;

https://wpt.fyi/results/css/css-scroll-snap?run_id=5156882370265088&run_id=6205813376155648

As you can see overscroll-snap.html failed on the left side column of Firefox, the test succeeded on the right column.
On the other hand, snap-at-user-scroll-end.html succeeded on the left column, and failed on the right column.

snap-at-user-scroll-end.html has succeeded since bug 2008571, so I am pretty sure that bug 2008571 changed something on wpt.fyi.

This bug would be tough since overscroll-snap.html hasn't failed on our CIs.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Geez, this also depends on pixel alignment.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

The code in question is ScrollSnapRange::IsValid;

// Returns true if |aPoint| is a valid snap position in this range.
bool IsValid(nscoord aPoint, nscoord aSnapportSize) const {
  MOZ_ASSERT(End() - Start() > aSnapportSize);
  return Start() <= aPoint && aPoint <= End() - aSnapportSize;
}

In this overscroll-snap.html case, End() is 1942.5pxin CSS unit, it's the child element height. aSnapportSize is 400px, it's the container's height. So far so good.

The aPoint is 1543px, it's clamped in MaybeAdjustDeltaForScrollSnapping;

CSSPoint destination = Metrics().CalculateScrollRange().ClampPoint(
    aStartPosition + ToCSSPixels(aDelta));

Metrics().CalculateScrollRange() is unfortunately aligned to 1543px, ideally it should be 1542.5px (= 1942.5 - 400).

With the aligned 1543px value, IsValid() returns false since it's outside of the snapport, thus it's not considered a valid snap position.

Note about the test itself. It was landed along with a chromium bug fix in https://chromium-review.googlesource.com/c/chromium/src/+/4774422, as I understand it, the chromium bug is pretty much same what we see in this bug, in other words, the test properly hits our bug too! The way how chrome fixed the bug is to add a tolerance in the comparison. We may need to the same approach here. Though I am not a big fan of the tolerance.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 2020558 - Allow 0.5px in CSS unit tolerance at the end edge of scrollable range in ScrollSnapRange::IsValid. r?botond

The test, overscroll-snap.html, is a clone of
testing/web-platform/tests/css/css-scroll-snap/overscroll-snap.html, but
it runs with layout.disable-pixel-alignment=false.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/97c4506278f1
https://hg.mozilla.org/integration/autoland/rev/2385578957ab
Allow 0.5px in CSS unit tolerance at the end edge of scrollable range in ScrollSnapRange::IsValid. r=botond

Comment 5

[pstanciu]

https://hg.mozilla.org/mozilla-central/rev/2385578957ab