2015367 - Fix unrelated-gesture-scroll-during-snap.html failure

Status: RESOLVED FIXED

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

Description

[Hiroyuki Ikezoe (:hiro)]

There are 5 issues;

1. scroll snap doesn't work for WebDriver's scroll action due to this SCROLLDELTA_PIXEL check (bug 2008571)
2. IntendedEndPosition needs to be set for WebDriver's scroll action here since the action has the exact end position, in other words scroll actions are more like pan gesture events rather than wheel
3. this mSnapFlags & IntendedDirection should be mSnapFlags == IntendedDirection since IntendedDirection|IntendedEndPosition might snap back to the original position, I thought this is a part of bug 2006243, but it might be independent
4. the test initially sends a small scroll delta that it can not be escaped from the original snap point and checks whether a scroll event is fired, but in Gecko we never scroll the content in such unescape case, the test needs to be revised to not depend on the browser specific behavior
5. In the test the expected scroll position is based on Element.offset, because of %-based margin-bottom, the offset value can be fractional which will be different from the scroll position, we need to avoid the fractional difference, a way I found is adding scrollbar-width: none in the scroll container

Comment 1

[BugBot [:suhaib / :marco]]

The severity field is not set for this bug.
:botond, could you have a look please?

For more information, please visit BugBot documentation.

Comment 2

[Botond Ballo [:botond]]

Attachment: WIP: [WIP] Bug 2015367 - Dispatch WebDriver scroll actions as PanGestureInput

Comment 3

[Botond Ballo [:botond]]

Attachment: WIP: [WIP] Bug 2015367 - Use scrollbar-width:none to avoid scroll position rounding issues

Comment 4

[Botond Ballo [:botond]]

Attachment: WIP: Bug 2015367 - Test expectation changes

Comment 5

[Botond Ballo [:botond]]

Dispatching WebDriver scroll actions as PanGestureInput seems to be sufficient to address issues (2), (3), and (4) from comment 0, though I'm not yet sure what impact it will have on other tests (I believe it's a behaviour change we would need to make globally, rather than just for particular tests).

Using scrollbar-width:none indeed fixes issue (5), though I'm not sure if that change would be appropriate for upstreaming.

Comment 6

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #5)

Dispatching WebDriver scroll actions as PanGestureInput seems to be sufficient to address issues (2), (3), and (4) from comment 0, though I'm not yet sure what impact it will have on other tests (I believe it's a behaviour change we would need to make globally, rather than just for particular tests).

This change regresses a number of web platform tests. One of them is prefer-common-to-both-axes.html.

Alex and I were debugging this test earlier today, but ran into an issue using rr on Alex's machine. I've subsequently taken a recording on my machine and uploaded it to Pernosco for Alex to look at further, here.

I added an entry to the Notebook view to mark the place where APZ processes a PanGestureInput event which the test expects to trigger a scrollend event but the scrollend event never arrives.

Comment 7

[Botond Ballo [:botond]]

Noting this for future reference: another test the changes regress is scrollend-event-not-fired-on-no-scroll.html.

That test performs a scroll action in a direction in which there is no room to scroll, and with pan gesture inputs, we overscroll in this situation, and fire a scrollend event at the end of the overscroll animation, but the test expects that no scrollend event is fired.

Chrome does not fire a scrollend event after an overscroll animation, and I believe it is correct to do so, since the spec describes scrollend events only being enqueued in situations where a scroll event also occurs, and overscrolling does not produce scroll events.

So we will need to change our implementation to avoid sending scrollend events after an overscroll animation.

Comment 8

[Hiroyuki Ikezoe (:hiro)]

I've posted patches both in bug 2038478 and bug 2038480. With those patches I can't see new test failures in testing/web-platform/tests/css/css-scroll-snap/ .

Note that though those patches are ready to be reviewed, but as of now it's marked as WIP since there's no automated test. I believe those bugs can be testable. But as of now both are tricky.

For bug 2038478 we need native events which scrolls the given delta precisely.

For bug 2038480 the test can't run on Linux properly since on Linux native pan gesture events have mSimulateMomentum flag, with the flag, the bug will not be triggered.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

I've written a mochitest for bug 2038478.

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Wrote a mochitest for bug 2038480.

Comment 11

[Hiroyuki Ikezoe (:hiro)]

The last remaining failure in scroll-snap directory is this prefer-common-to-both-axes.html. Whether it's reproducible locally or not was not a big problem. Claude Code immediately found out the failure reason.

The reason is our axis locking.

The test dispatches diagonal pan gestures, well, let me copy and paste the Claude's analysis.

In prefer-common-to-both-axes.html, the deltas going from (0,0) to the various targets are diagonals like (220,110), (110,220), (110,330), (330,110) — all of which sit within 30° of an axis (≈27°, 27°, 18°, 18°). The first such pan locks the minor axis, so the scroller only advances along one axis. After PANGESTURE_END, ScrollSnapToDestination() snaps from that partial position and lands on a different snap target than the test expects, so the assert_equals(scroller.scrollLeft, …) / scrollTop check in scrollToAlignedElements (resources/common.js:45‑48) fails.

The 45° cases (e.g. target box(5) at (220,220)) and box(8) at (220,330) / box(6) at (330,220) (≈33°) shouldn't lock, so those subtests should still pass — it's the off‑diagonal subtests that regress.

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Filed bug 2040252 for the axis lock issue.

Comment 13

[Phabricator Automation]

Comment on attachment 9569606 [details]
WIP: [WIP] Bug 2015367 - Dispatch WebDriver scroll actions as PanGestureInput

Revision D294084 was moved to bug 2041034. Setting attachment 9569606 [details] to obsolete.

Comment 14

[Hiroyuki Ikezoe (:hiro)]

Attachment: WIP: Bug 2015367 - A simple test case to check whether WebDriver scroll action is treated as relative.

Comment 15

[Hiroyuki Ikezoe (:hiro)]

Note that the wpt (D302218) is a test case to check whether WebDriver scroll action is treated as relative scroll operation or not. The test passes if it's relative.

On Firefox it passes, on Chrome it fails.

The reason why it fails on Chrome I suppose is that, they dispatch pan gesture events on WebDriver scroll action but as of now they haven't yet changed their pan gestures as relative (https://issues.chromium.org/issues/451677709).

Comment 16

[Hiroyuki Ikezoe (:hiro)]

It's blocked on the spec issue Alex opened.

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+2]]

The upstream wpt patch that Alex posted via bug 2042841 comment 3 seem to fix the test for all browsers. So I assume we do not need any of these patches as attached to this bug?

Comment 18

[Botond Ballo [:botond]]

(In reply to Henrik Skupin [:whimboo][⌚️UTC+2] from comment #17)

The upstream wpt patch that Alex posted via bug 2042841 comment 3 seem to fix the test for all browsers. So I assume we do not need any of these patches as attached to this bug?

Yes; I will abandon the Phabricator revisions posted to this bug after the test change PR merges and I've confirmed that the test's passing is reflected in our Interop score.

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Great. It will be downstream synced via bug 2043443.

But it means that we also don't need bug 2041034, right? A change like that most likely would require spec work on our side as well.

Comment 20

[Botond Ballo [:botond]]

(In reply to Henrik Skupin [:whimboo][⌚️UTC+2] from comment #19)

But it means that we also don't need bug 2041034, right?

That's right. (Bug 2041034 was spun out from this bug for the purpose of land one of the patches in the original series, possibly behind a pref.)

A change like that most likely would require spec work on our side as well.

At the time of filing, I was thinking we could make this implementation change without any spec changes. I think we still potentially could if we needed to, but now that it's looking like we won't need this for Interop 2026, I think it will make more sense to start with a spec discussion and maybe give users of the WebDriver scroll action API the option to choose between pan-gesture vs. mouse-wheel behaviour.

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Botond Ballo [:botond] from comment #20)

At the time of filing, I was thinking we could make this implementation change without any spec changes. I think we still potentially could if we needed to, but now that it's looking like we won't need this for Interop 2026, I think it will make more sense to start with a spec discussion and maybe give users of the WebDriver scroll action API the option to choose between pan-gesture vs. mouse-wheel behaviour.

Sounds good. I assume that we can now close this bug?

Comment 22

[Hiroyuki Ikezoe (:hiro)]

Yes.