Bug 135994 - Implement "proximity" scroll snapping
Summary: Implement "proximity" scroll snapping
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: iPhone / iPad Unspecified
: P2 Enhancement
Assignee: Wenson Hsieh
URL:
Keywords: InRadar
Depends on:
Blocks: 134283 172349
  Show dependency treegraph
 
Reported: 2014-08-15 15:17 PDT by Wenson Hsieh
Modified: 2017-05-19 01:04 PDT (History)
16 users (show)

See Also:


Attachments
First pass (132.59 KB, patch)
2016-12-29 11:51 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff
First pass (fix EFL build) (132.59 KB, patch)
2016-12-29 14:13 PST, Wenson Hsieh
dino: review+
Details | Formatted Diff | Diff
Patch for landing (133.73 KB, patch)
2017-01-10 13:25 PST, Wenson Hsieh
wenson_hsieh: commit-queue+
Details | Formatted Diff | Diff
Patch for landing (133.92 KB, patch)
2017-01-10 13:50 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Wenson Hsieh 2014-08-15 15:17:45 PDT
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.
Comment 1 Radar WebKit Bug Importer 2014-08-28 09:34:12 PDT
<rdar://problem/18162418>
Comment 2 Wenson Hsieh 2016-12-26 11:02:53 PST
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.
Comment 3 Wenson Hsieh 2016-12-29 11:51:17 PST
Created attachment 297838 [details]
First pass
Comment 4 Wenson Hsieh 2016-12-29 14:13:43 PST
Created attachment 297842 [details]
First pass (fix EFL build)
Comment 5 Dean Jackson 2017-01-10 12:00:44 PST
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.
Comment 6 Wenson Hsieh 2017-01-10 13:20:00 PST
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!
Comment 7 Wenson Hsieh 2017-01-10 13:25:46 PST
Created attachment 298508 [details]
Patch for landing
Comment 8 Wenson Hsieh 2017-01-10 13:50:23 PST
Comment on attachment 298508 [details]
Patch for landing

Never mind -- looks like I do need to #include <wtf/Optional.h> after all.
Comment 9 Wenson Hsieh 2017-01-10 13:50:47 PST
Created attachment 298509 [details]
Patch for landing
Comment 10 WebKit Commit Bot 2017-01-10 14:28:00 PST
Comment on attachment 298509 [details]
Patch for landing

Clearing flags on attachment: 298509

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