Closed Bug 1766192 Opened 7 months ago Closed 7 months ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
102 Branch
Tracking Status
firefox102 --- fixed

People

(Reporter: hiro, Assigned: hiro)

References

(Regressed 1 open bug, )

Details

Attachments

(6 files)

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.

Severity: -- → S3
Assignee: nobody → hikezoe.birchill

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

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

Depends on D144531

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

See Also: → 1768393
Keywords: leave-open
Pushed by hikezoe.birchill@mozilla.com:
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 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>
Attachment #9275658 - Attachment description: A test case Botond raied in a review comment → A test case Botond raised in a review comment
Regressions: 1768701
Keywords: leave-open
Pushed by hikezoe.birchill@mozilla.com:
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
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/34050 for changes under testing/web-platform/tests
Status: ASSIGNED → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → 102 Branch
Upstream PR merged by moz-wptsync-bot
You need to log in before you can comment on or make changes to this bug.