1766192 - Do `snap-scope` with the snapped target point rather than the scroll's destination

Status: RESOLVED FIXED

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

Description

[Hiroyuki Ikezoe (:hiro)]

I did wrongly implement the snap-scope feature by using the snapport on the destination.

From the spec text;

a scroll position cannot be considered a valid snap position if snapping to it would leave the contributing snap area entirely outside the snapport

I believe snapping to it clearly means "snapping to the candidate snap point".

This incorrect implementation has made some wpt tests failure, scrollTo-scrollBY-snaps.html for example. And the incorrect implementation has been made some wpt pass in overflowing-snap-areas.html.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1766192 - Drop CalcSnapPoints::AddEdgeInterval. r?botond

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1766192 - Choose the second best edge on the opposite side of the best edge. r?botond

The second best edge is used for snap-overflow feature [1]. The
snap-overflow is applied when the following conditions are met.

1. A scroll snap target element is larger than the snapport (e.g. scrollport)
   in an axis
2. The distance between the best edge and the second best edge (on the opposite
   side of the best edge) is later than the snapport size in the axis

There was a problem in our implementation. For example, in below diagram, D is
the original destination of the given scroll operation, 1 is the best edge, 2 is
the second best in our original implementation and 3 is of of the other snap
points. In this case if there's an element larger than the distance between 1
and 3 and if the snapport size is larger than the distance between 1 and 2 but
smaller than the distance between 1 and 3, we should apply snap-overflow.

2 1 D 3
|-|---|-----|

There are a couple of test cases in wpt for this condition, for example there's
one of them in oveflowing-snap-area.html [2]. That test case have been passed
incorrectly due to our incorrect snap-scope implementation which will be fixed
a subsequent change in this commit series.

[1] https://drafts.csswg.org/css-scroll-snap-1/#snap-overflow
[2] https://searchfox.org/mozilla-central/rev/13d69189a8abfc5064fe44944550b9b6eb4544f5/testing/web-platform/tests/css/css-scroll-snap/overflowing-snap-areas.html#131-137

Depends on D144530

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1766192 - Introduce SnapTarget struct along with snap-area for each element. r?botond

In the next change we will use the snap-area to implement snap-score
properly.

Depends on D144531

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1766192 - Do `snap-scope` with the snapped point rather than the destination point. r?botond

Depends on D144532

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Attachment: A test case Botond raised in a review comment

For references I am attaching a test case that Botond raised in a review comment.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1766192 - Add a wpt that scrolling to a position on a large element but the element doesn't cover the snapport. r?botond

Depends on D144533

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2034738aeed0
Drop CalcSnapPoints::AddEdgeInterval. r=botond
https://hg.mozilla.org/integration/autoland/rev/162d5a829b60
Choose the second best edge on the opposite side of the best edge. r=botond
https://hg.mozilla.org/integration/autoland/rev/4b5d27903092
Introduce SnapTarget struct along with snap-area for each element. r=botond
https://hg.mozilla.org/integration/autoland/rev/3b3ffdb196d9
Do `snap-scope` with the snapped point rather than the destination point. r=botond

Comment 9

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/2034738aeed0
https://hg.mozilla.org/mozilla-central/rev/162d5a829b60
https://hg.mozilla.org/mozilla-central/rev/4b5d27903092
https://hg.mozilla.org/mozilla-central/rev/3b3ffdb196d9

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 9275658 [details]
A test case Botond raised in a review comment

><!DOCTYPE html>
><style>
>div {
>  position: absolute;
>  width: 100%;
>}
>#scroller {
>  left: 10px;
>  height: 200px;
>  width: 100px;
>  overflow: scroll;
>  scroll-snap-type: y mandatory;
>}
>.snap {
>  scroll-snap-align: start;
>  background-color: green;
>}
>.target {
>  background-color: red;
>  border-radius: 100%;
>  height: 88px;
>  top: 536px;
>}
></style>
><div id="scroller">
>  <div style="height: 2000px;"></div>
>  <div class="snap" style="top:    0px; height:   40px;">1</div>
>  <div class="snap" style="top:   40px; height:   40px;">2</div>
>  <div class="snap" style="top:   80px; height: 1000px;">3</div>
>  <div class="snap" style="top: 1080px; height:   40px;">4</div>
>  <div class="snap" style="top: 1120px; height:   40px;">5</div>
>  <div class="target"></div>
></div>

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1ef2f1d36db0
Add a wpt that scrolling to a position on a large element but the element doesn't cover the snapport. r=botond

Comment 12

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

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

Comment 13

[bszekely]

https://hg.mozilla.org/mozilla-central/rev/1ef2f1d36db0

Comment 14

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

Upstream PR merged by moz-wptsync-bot