Bug 227062 – [css-scroll-snap] Simplify snap point selection helpers

• Component: Scrolling
• Status: RESOLVED FIXED

[Martin Robinson]

We can combine the two snap point selection helpers indicesOfNearestSnapOffsets and searchForPotentialSnapPoints, reducing the amount of times that we walk the snap point list and making it more obvious what the return values of the arguments are. In addition, we can have this method return the geometric next and previous snap offsets if they exist, rather than two offsets that might both be greater or lesser than the target. This is useful because parts of the specification refer to these two values when implementing certain features. This makes the code a lot easier to read and very slightly more efficient.

[Martin Robinson]

(In reply to Martin Robinson from comment #0)
> We can combine the two snap point selection helpers
> indicesOfNearestSnapOffsets and searchForPotentialSnapPoints
                                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Here I meant to write findFirstSnapStopOffsetBetweenOriginAndDestination.

[Martin Robinson]

Created attachment 431531 [details]
Patch

[Martin Robinson]

Created attachment 431535 [details]
Patch

[Frédéric Wang (:fredw)]

Comment on attachment 431535 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=431535&action=review

> Source/WebCore/page/scrolling/ScrollSnapOffsetsInfo.cpp:109
> +

It seems this neither use viewportLength nor isNearEnoughToOffsetForProximity, so it can keep it at the place of the original "if (originalOffsetForDirectionalSnapping)".

[Radar WebKit Bug Importer]

<rdar://problem/79657705>

[Martin Robinson]

Created attachment 432041 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 432041 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=432041&action=review

> Source/WebCore/ChangeLog:16
> +        most snap containers have a small number of snap points. Thus, it's unlikely that this

"most snap containers have a small number of snap points". That's true until it isn't! What about a big table where every cell is a snap point?

> Source/WebCore/page/scrolling/ScrollSnapOffsetsInfo.cpp:59
> +    // offset than the previously selected snap stop. We always want to stop at the closest snap stop to the original offset.

Clearer as "We always want to stop at the snap stop closest to the original offset"

> Source/WebCore/page/scrolling/ScrollSnapOffsetsInfo.cpp:134
> +        if (*originalOffsetForDirectionalSnapping < scrollDestinationOffset && previous && (*previous).first <= *originalOffsetForDirectionalSnapping)
> +            previous.reset();
> +        if (*originalOffsetForDirectionalSnapping > scrollDestinationOffset && next && (*next).first >= *originalOffsetForDirectionalSnapping)
> +            next.reset();

Maybe put *originalOffsetForDirectionalSnapping in a local variable.

[Martin Robinson]

Created attachment 432244 [details]
Patch

[Frédéric Wang (:fredw)]

Comment on attachment 432244 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=432244&action=review

> Source/WebCore/ChangeLog:18
> +        of snap points and performance does not seem to be changed. In addition, a future change should mean

of of

[Martin Robinson]

Created attachment 432245 [details]
Patch

[Martin Robinson]

(In reply to Simon Fraser (smfr) from comment #7)
> Comment on attachment 432041 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=432041&action=review
> 
> > Source/WebCore/ChangeLog:16
> > +        most snap containers have a small number of snap points. Thus, it's unlikely that this
> 
> "most snap containers have a small number of snap points". That's true until
> it isn't! What about a big table where every cell is a snap point?

This is a good point. I did some testing with large 500 cell tables and didn't notice any performance degradation. I think the main reason is that for cases where you are scrolling around like this, we were doing a linear search through the scroll stops any way. In addition, I think there's a lot of performance enhancement opportunities to be had by calling this function less (such as only after an animation completes).

> > Source/WebCore/page/scrolling/ScrollSnapOffsetsInfo.cpp:59
> > +    // offset than the previously selected snap stop. We always want to stop at the closest snap stop to the original offset.
> 
> Clearer as "We always want to stop at the snap stop closest to the original
> offset"

Fixed!

> > Source/WebCore/page/scrolling/ScrollSnapOffsetsInfo.cpp:134
> > +        if (*originalOffsetForDirectionalSnapping < scrollDestinationOffset && previous && (*previous).first <= *originalOffsetForDirectionalSnapping)
> > +            previous.reset();
> > +        if (*originalOffsetForDirectionalSnapping > scrollDestinationOffset && next && (*next).first >= *originalOffsetForDirectionalSnapping)
> > +            next.reset();
> 
> Maybe put *originalOffsetForDirectionalSnapping in a local variable.

Sure. This new version does this.

[Martin Robinson]

(In reply to Frédéric Wang (:fredw) from comment #9)
> Comment on attachment 432244 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=432244&action=review
> 
> > Source/WebCore/ChangeLog:18
> > +        of snap points and performance does not seem to be changed. In addition, a future change should mean
> 
> of of

I've fixed this.

[EWS]

Committed r279272 (239151@main): <https://commits.webkit.org/239151@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 432245 [details].