1768393 - scroll snap incorrectly snaps to a point where it's defined on an invisible element

Status: RESOLVED FIXED

(Core :: Layout: Scrolling and Overflow, defect, P3)

Description

[Hiroyuki Ikezoe (:hiro)]

Attachment: A test case

STR;

1. Open the attachment in this comment
2. Press arrow left key to scroll right

Actual result;
it snaps to 100px left of the blue box you can see after scrolling

Expected result;
it snaps to the left edge of the blue box

The reason why it snaps to 100px left is there's another snap target element but it's out of the scroll port.

Comment 1

[Thomas Wisniewski [:twisniewski]]

Does this cover these interop2023 WPTs?

• https://wpt.fyi/results/css/css-scroll-snap/snap-to-visible-areas-both.html
• https://wpt.fyi/results/css/css-scroll-snap/snap-to-visible-areas-x-axis.html
• https://wpt.fyi/results/css/css-scroll-snap/snap-to-visible-areas-y-axis.html

Or should we file any new bugs for them?

Comment 2

[Hiroyuki Ikezoe (:hiro)]

(In reply to Thomas Wisniewski [:twisniewski] from comment #1)

Does this cover these interop2023 WPTs?

• https://wpt.fyi/results/css/css-scroll-snap/snap-to-visible-areas-both.html
• https://wpt.fyi/results/css/css-scroll-snap/snap-to-visible-areas-x-axis.html
• https://wpt.fyi/results/css/css-scroll-snap/snap-to-visible-areas-y-axis.html

I believe so, yes.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #2)

(In reply to Thomas Wisniewski [:twisniewski] from comment #1)

Does this cover these interop2023 WPTs?

• https://wpt.fyi/results/css/css-scroll-snap/snap-to-visible-areas-both.html
• https://wpt.fyi/results/css/css-scroll-snap/snap-to-visible-areas-x-axis.html
• https://wpt.fyi/results/css/css-scroll-snap/snap-to-visible-areas-y-axis.html

There are two different reasons making these test fail.

The reason of [x|y]-axis.html failrues is pretty simple. We do ignore the scroll container's scroll snap strictness property when we tell whether scrolling to the given snap position makes the snap target element into the view or not. It's exactly the same reason why the test case in comment 0 fails.

The reason of both.html failure is much much more complicated. The test case is something like this;

+---------------------------------------------------+
| +--+             +--+                             |
| |  |             |  |                             |
| +--+             +--+                             |
|                                                   |
|           d                                       |
|                                                   |
|                                                   |
| +--+             X                                |
| |  |                                              |
| +--+                                              |
+---------------------------------------------------+

The outer box is the scroll container's scrollable range, and the inner boxes are snap target elements. The 'd' is the given scroll destination. If we naively choose the snap position on each axis respectively, "X" would be the one. But scrolling to the "X" will not scroll any snap target elements in the scrollport view. We need to choose either of the top right one or the bottom left one depending on the distance from "d".

There's another interesting similar test case in scrollTo-scrollBy-snaps.html, the case is something like;

+---------------------------------------------------+
| +--+             +--+                             |
| |  |             |  |                             |
| +--+             +--+                             |
|                                                   |
|                                                   |
|                                                   |
|               d                                   |
| +--+             +--+                             |
| |  |             |  |                             |
| +--+             +--+                             |
+---------------------------------------------------+

Now there's a snap target element at bottom right which is closest to "d". But our algorithm to choose the snap position has only one position on each axis as the best position. Thus if we iterate the top right and the bottom left ones before the bottom right one, the bottom right one will never be chosen. We need to keep all snap target candidates on the same position and choose the closest one lastly.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Attachment: An exaggerated test case

The latter case in comment 3 is pretty tough.

Attaching file is exaggerating the toughness. There are 12 vertically aligned 50x50 sized boxes from top: 0px to top: 550px at left: 600px. Also there are 12 horizontally aligned 50x50 size boxes from left: 0px to left: 550px at top: 600px. Thus if scrolling to (600, 600), no box is visible. But Firefox snaps to (600, 600). Chrome also snaps to the same position.

Ideally what we need to do to determine the correct snap position is calculating each distance from the scroll destination against each combinations of 12x12 boxes. And we need to choose the nearest distance one where both vertical/horizontal aligned box are visible. I have been figuring out efficient ways to do it, but haven't yet.

Now I'd rather do a not-perfect way, just choosing one of the box instead of trying to iterate 12x12 combinations. But still there are possibilities that the nearest-ish box doesn't specify snap point on the other axis.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1768393 - Add wpts snapping to points of combinations of two different elements. r?botond

snap-to-combination-of-two-elements-2.tentative.html is a bit questionable,
thus it's tentative for now.

We fail the tentative test due to bug 1768393. Chrome also fails in a different
way. Chrome seems to filter out snap target elements initially if they are
outside of the scroll port and then determine the final snap position. Thus snap
positions defined by #right-bottom element in the test are ignored in the case
of the initial scroll position (0, 0).

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1768393 - Split out ScrollSnapInfo. r?botond

Depends on D184332

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1768393 - Rename mozilla::SnapTarget to mozilla::SnapDestination. r?botond

It was confusing with ScrollSnapInfo::SnapTarget.

Depends on D184333

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1768393 - Use the snap position on the axis only if the scroll snap strictness is not `none` to tell whether the given snap position is inside the scrollport or not. r?botond

Depends on D184334

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1768393 - Change `mSecondBestEdge` type to nscoord. r?botond

Depends on D184335

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1768393 - Introduce SnapPoint to wrap a pair of Maybe<nscoord>. r?botond

Depends on D184336

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1768393 - Drop mBestEdge initialization. r?botond

We can refer mEdgeFound whether mBestEdge has been set or not.

Depends on D184337

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1768393 - Support multiple SnapPositions in CandidateTracker. r?botond

It's possible that multiple snap target elements are located at the same
position in one axis but at different position in the other axis. We need to
keep track of them respectively in each CandidateTracker.

Depends on D184338

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1768393 - Add the snap area into ScrollSnapRange. r?botond

We could avoid adding the snap area into ScrollSnapRange if we added two
additional flags representing whether the snap area size is larger than the
snapport into ScrollTarget and drop ScrollSnapRange entirely. But it would
be inefficient in most cases since such larger snap area cases are uncommon, we
will not find any larger snap areas while iterating over
ScrollSnapInfo::mSnapTargets.

Depends on D184339

Comment 14

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1768393 - Make SnapPosition inherit from ScrollSnapInfo::SnapTarget. r?botond

We will need the snap area, (i.e. SnapTarget::mSnapArea) in SnapPosition to tell
whether the snap area is visible or not if we snap to a candidate snap position.

Though SnapPosition::mPosition is redundant, it's equivalent to either
SnapTarget::mSnapPoint.mX or SnapTarget::mSnapPoint.mY, if we omit it, we need
another variable representing the axis of SnapPosition.

Depends on D184340

Comment 15

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1768393 - Filter out snap candidates if snapping to the point makes the snap area outside of the snapport. r?botond

Depends on D184341

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4eb09c5400ea
Add wpts snapping to points of combinations of two different elements. r=botond
https://hg.mozilla.org/integration/autoland/rev/13a5913debb4
Split out ScrollSnapInfo. r=botond
https://hg.mozilla.org/integration/autoland/rev/caf1efcd57ba
Rename mozilla::SnapTarget to mozilla::SnapDestination. r=botond
https://hg.mozilla.org/integration/autoland/rev/b0265c1e58ca
Use the snap position on the axis only if the scroll snap strictness is not `none` to tell whether the given snap position is inside the scrollport or not. r=botond
https://hg.mozilla.org/integration/autoland/rev/6efd1a99d805
Change `mSecondBestEdge` type to nscoord. r=botond
https://hg.mozilla.org/integration/autoland/rev/2f8339183e3a
Introduce SnapPoint to wrap a pair of Maybe<nscoord>. r=botond
https://hg.mozilla.org/integration/autoland/rev/5b878451ca58
Drop mBestEdge initialization. r=botond
https://hg.mozilla.org/integration/autoland/rev/75993d5000bc
Support multiple SnapPositions in CandidateTracker. r=botond
https://hg.mozilla.org/integration/autoland/rev/7a1ffa23ff41
Add the snap area into ScrollSnapRange. r=botond
https://hg.mozilla.org/integration/autoland/rev/f30d814e0bdb
Make SnapPosition inherit from ScrollSnapInfo::SnapTarget. r=botond
https://hg.mozilla.org/integration/autoland/rev/824ceee16145
Filter out snap candidates if snapping to the point makes the snap area outside of the snapport. r=botond

Comment 17

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/41927 for changes under testing/web-platform/tests

Comment 18

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/4eb09c5400ea
https://hg.mozilla.org/mozilla-central/rev/13a5913debb4
https://hg.mozilla.org/mozilla-central/rev/caf1efcd57ba
https://hg.mozilla.org/mozilla-central/rev/b0265c1e58ca
https://hg.mozilla.org/mozilla-central/rev/6efd1a99d805
https://hg.mozilla.org/mozilla-central/rev/2f8339183e3a
https://hg.mozilla.org/mozilla-central/rev/5b878451ca58
https://hg.mozilla.org/mozilla-central/rev/75993d5000bc
https://hg.mozilla.org/mozilla-central/rev/7a1ffa23ff41
https://hg.mozilla.org/mozilla-central/rev/f30d814e0bdb
https://hg.mozilla.org/mozilla-central/rev/824ceee16145

Comment 19

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot