Bug 135994 – Implement "proximity" scroll snapping

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Wenson Hsieh]

Since it's not exactly clear what -scroll-snap-type: proximity entails, proximity snapping is not yet implemented. We'll need to clarify the behavior of proximity, as well as modify scroll snapping on iOS and Mac to support proximity scroll snapping.

To use proximity, just add the property -webkit-scroll-snap-type: proximity; to the scroll snapping container.

[Radar WebKit Bug Importer]

<rdar://problem/18162418>

[Wenson Hsieh]

On ToT, Firefox's proximity threshold (the minimum distance between the destination scroll offset and the potential snap offset before we choose that snap offset) is hard-coded to be 200px. I think we should make this a ratio of the size of the scroll port width/height, but it's worth playing around with this a bit to see what feels best.

[Wenson Hsieh]

Created attachment 297838 [details]
First pass

[Wenson Hsieh]

Created attachment 297842 [details]
First pass (fix EFL build)

[Dean Jackson]

Comment on attachment 297842 [details]
First pass (fix EFL build)

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

> Source/WebCore/ChangeLog:28
> +        position given a destination offset is now:
> +
> +        -   If the scroll destination lies within a snap offset range, return the scroll destination
> +        -   Otherwise, compute the nearest lower/upper snap offsets and lower/upper snap offset ranges
> +        -   If scrolling ended with no velocity, return the nearest snap offset
> +        -   If scrolling ended with positive velocity, choose the upper snap offset only if there is no snap offset
> +            range in between the scroll destination and the snap offset; else, choose the lower snap offset
> +        -   If scrolling ended with negative velocity, choose the lower snap offset only if there is no snap offset
> +            range in between the scroll destination and the snap offset; else, choose the upper snap offset
> +
> +        The extra rule accounting for scroll offset ranges in between the scroll destination and a potential snap offset
> +        handles the corner case where the user scrolls with momentum very lightly away from a snap offset, such that the
> +        predicted scroll destination is still within proximity of the snap offset. In this case, the regular (mandatory
> +        scroll snapping) behavior would be to snap to the next offset in the direction of momentum scrolling, but
> +        instead, it is more intuitive to return to the original snap position.

I wonder if some of this would be better suited to comments in the code. Otherwise we'll have to hope people come to the changelog from the code.

> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:94
> +    return s.toString().utf8().data();

Why .utf8().data()? You're returning a String, so .toString() is fine.

> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:156
> +        int middleIndex = (lowerIndex + upperIndex) / 2;
> +        if (offset < snapOffsets[middleIndex])
> +            upperIndex = middleIndex;
> +        else if (offset > snapOffsets[middleIndex])
> +            lowerIndex = middleIndex;
> +        else {
> +            upperIndex = middleIndex;
> +            lowerIndex = middleIndex;
> +            break;
> +        }

Maybe you should have the logic in the same order you use within indicesOfNearestSnapOffsetRanges? There you check the exit clause first.

> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:174
> +    static const float ratioOfScrollPortAxisLengthToBeConsideredForProximity = 0.3;

Maybe put a FIXME or comment here to indicate this is an arbitrary choice?

> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:304
> +    // Nonzero velocity indicates a flick gesture. Even if another snap point is closer, we should choose the one in the direction of the flick gesture

Nit: Non-zero :)

> Source/WebCore/page/scrolling/ScrollingMomentumCalculator.cpp:37
> +    const static double inertialScrollPredictionFactor = 16.7;

Explain why this number?

> Source/WebCore/page/scrolling/ScrollingMomentumCalculator.h:33
> +#include <wtf/Optional.h>

Do you need wtf/Optional.h if you're using std::optional? (I don't know... I've never tried)

> LayoutTests/tiled-drawing/scrolling/scroll-snap/scroll-snap-mandatory-then-proximity.html:61
> +                write("This test requires UIScriptController support.");

I think it's only eventSender, not UIScriptController.

[Wenson Hsieh]

Comment on attachment 297842 [details]
First pass (fix EFL build)

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

>> Source/WebCore/ChangeLog:28
>> +        instead, it is more intuitive to return to the original snap position.
> 
> I wonder if some of this would be better suited to comments in the code. Otherwise we'll have to hope people come to the changelog from the code.

Good idea -- I've included this as a comment.

>> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:94
>> +    return s.toString().utf8().data();
> 
> Why .utf8().data()? You're returning a String, so .toString() is fine.

Good catch! Fixed. (I initially had this return a const char*, but then changed to String).

>> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:156
>> +        }
> 
> Maybe you should have the logic in the same order you use within indicesOfNearestSnapOffsetRanges? There you check the exit clause first.

Yes -- that's reasonable. Fixed.

>> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:174
>> +    static const float ratioOfScrollPortAxisLengthToBeConsideredForProximity = 0.3;
> 
> Maybe put a FIXME or comment here to indicate this is an arbitrary choice?

Ok -- I'll include the relevant comments from the ChangeLog here.

>> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:304
>> +    // Nonzero velocity indicates a flick gesture. Even if another snap point is closer, we should choose the one in the direction of the flick gesture
> 
> Nit: Non-zero :)

Fixed.

>> Source/WebCore/page/scrolling/ScrollingMomentumCalculator.cpp:37
>> +    const static double inertialScrollPredictionFactor = 16.7;
> 
> Explain why this number?

On macOS 10.10 and earlier, we don't have a platform scrolling momentum calculator, so we instead approximate the scroll destination by multiplying the initial wheel delta by a constant factor. By running a few "experiments" (i.e. logging scroll destination and initial wheel delta for many scroll gestures) I determined that this is a reasonable way to approximate where scrolling will take us without using _NSScrollingMomentumCalculator. I'll add this information as a comment here.

>> Source/WebCore/page/scrolling/ScrollingMomentumCalculator.h:33
>> +#include <wtf/Optional.h>
> 
> Do you need wtf/Optional.h if you're using std::optional? (I don't know... I've never tried)

It works if I remove it here, but perhaps that's because it might have been included by another header? (e.g. MathCommon.h includes it). Many files in WebCore and both WebKits still include <wtf/Optional.h> -- I'll look into whether we can remove most of these.

>> LayoutTests/tiled-drawing/scrolling/scroll-snap/scroll-snap-mandatory-then-proximity.html:61
>> +                write("This test requires UIScriptController support.");
> 
> I think it's only eventSender, not UIScriptController.

Ah, that's right -- fixed!

[Wenson Hsieh]

Created attachment 298508 [details]
Patch for landing

[Wenson Hsieh]

Comment on attachment 298508 [details]
Patch for landing

Never mind -- looks like I do need to #include <wtf/Optional.h> after all.

[Wenson Hsieh]

Created attachment 298509 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 298509 [details]
Patch for landing

Clearing flags on attachment: 298509

Committed r210560: <http://trac.webkit.org/changeset/210560>