Implement snapping behavior for iOS

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Wenson Hsieh]

Implement snapping for both mainframe and overflow scrolling on iOS. This will involve moving snap position data via XPC, and then retargeting the scroll offset in overflow/mainframe scrolling to that of the appropriate snap point.

[Wenson Hsieh]

Created attachment 236563 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 236563 [details]
Patch

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

Looks good other than the encoding/decoding.

> Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.h:62
> +    Vector<LayoutUnit> horizontalSnapOffsets() const { return m_horizontalSnapOffsets; }
> +    Vector<LayoutUnit> verticalSnapOffsets() const { return m_verticalSnapOffsets; }

Please return a const reference.

> Source/WebCore/rendering/RenderLayer.cpp:3041
> +    

Whitespace.

> Source/WebCore/rendering/RenderLayer.cpp:3316
> +#if ENABLE(CSS_SCROLL_SNAP)
> +    // FIXME: Ensure that offsets are also updated in case of programmatic style changes.
> +    updateSnapOffsets();
> +#endif

Shouldn't this be in an earlier patch?

> Source/WebKit2/Shared/Scrolling/RemoteScrollingCoordinatorTransaction.cpp:135
> +        encoder << static_cast<uint64_t>(node.horizontalSnapOffsets().size());
> +        for (LayoutUnit position : node.horizontalSnapOffsets())
> +            encoder << position.toDouble();

Can't you just encode the Vector directly?

> Source/WebKit2/Shared/Scrolling/RemoteScrollingCoordinatorTransaction.cpp:222
> +    if (node.hasChangedProperty(ScrollingStateScrollingNode::HorizontalSnapOffsets)) {
> +        if (!decoder.decode(size))
> +            return false;
> +
> +        Vector<LayoutUnit> horizontalSnapOffsets;
> +        for (size_t i = 0; i < size; i++) {
> +            double position = 0;
> +            if (!decoder.decode(position))
> +                return false;
> +
> +            horizontalSnapOffsets.append(position);
> +        }
> +        node.setHorizontalSnapOffsets(horizontalSnapOffsets);

Similarly, I think the vector should just be decodable directly.

[Tim Horton]

Zalan is noting that you should avoid encoding LayoutUnits and instead encode pixel-snapped floats, and avoid LayoutUnits in the UI process at all.

[Wenson Hsieh]

Created attachment 236591 [details]
Patch

[Wenson Hsieh]

Comment on attachment 236591 [details]
Patch

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

> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.h:46
> +T closestSnapOffset(const Vector<T>& snapOffsets, T scrollDestination, float velocity)

This function is an updated version of the one found in the Mac animations patch.

> Source/WebKit2/UIProcess/ios/RemoteScrollingCoordinatorProxyIOS.mm:112
> +bool RemoteScrollingCoordinatorProxy::shouldHorizontalSnapForMainframeScrolling() const

To prevent requiring ScrollTypes, I split these methods into horizontal and vertical versions. Otherwise, I could pass in a WebCore::ScrollEventAxis

[zalan]

Comment on attachment 236591 [details]
Patch

This looks ok to me. Let's see what Tim says.

[Tim Horton]

Comment on attachment 236591 [details]
Patch

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

> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:106
> +        // FIXME: Make sure we snap these pixel values.

Why not do this now? I'm sure Zalan can tell you how, and I think it's not too hard.

> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:108
> +            snapOffsetsAsFloat.append(frameView->horizontalSnapOffsets()->at(i).round());

you're .at()ing again

> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:112
> +    if (frameView->verticalSnapOffsets()) {

perhaps a reusable static snapOffsetsAsFloats or something? this code is here four times

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:86
> +#if ENABLE(CSS_SCROLL_SNAP)

This doesn't belong in the middle of the big block if it has #if, but I don't think it needs the #if, either.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:1306
> +    if (_page->mainFrame() && _page->mainFrame()->page() && _page->mainFrame()->page()->scrollingCoordinatorProxy()) {

I wonder if this code can go somewhere more scrollingy (RemoteScrollingCoordinatorProxy?) and we can just call something->adjustTargetContentOffsetForSnapping(targetContentOffset) or something.

> Source/WebKit2/UIProcess/Scrolling/ios/ScrollingTreeOverflowScrollingNodeIOS.mm:33
> +#if ENABLE(CSS_SCROLL_SNAP)

no #if blocks inside the big block of #imports!

> Source/WebKit2/UIProcess/Scrolling/ios/ScrollingTreeOverflowScrollingNodeIOS.mm:206
> +        // FIXME: In only one axis snaps in 2D scrolling, the other axis will decelerate fast as well. Is this what we want?

there's some mixup at the beginning of this sentence

> Source/WebKit2/UIProcess/ios/RemoteScrollingCoordinatorProxyIOS.mm:40
> +#include <WebCore/ScrollingTreeFrameScrollingNode.h>

this is not sorted right

>> Source/WebKit2/UIProcess/ios/RemoteScrollingCoordinatorProxyIOS.mm:112
>> +bool RemoteScrollingCoordinatorProxy::shouldHorizontalSnapForMainframeScrolling() const
> 
> To prevent requiring ScrollTypes, I split these methods into horizontal and vertical versions. Otherwise, I could pass in a WebCore::ScrollEventAxis

Deduplicate!

> Source/WebKit2/UIProcess/ios/RemoteScrollingCoordinatorProxyIOS.mm:143
> +float RemoteScrollingCoordinatorProxy::closestVerticalSnapOffsetForMainframeScrolling(float scrollDestination, float velocity) const

deduplicate!

[Wenson Hsieh]

Created attachment 236615 [details]
Patch

[Wenson Hsieh]

Comment on attachment 236591 [details]
Patch

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

>> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:106
>> +        // FIXME: Make sure we snap these pixel values.
> 
> Why not do this now? I'm sure Zalan can tell you how, and I think it's not too hard.

Fixed.

>> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:108
>> +            snapOffsetsAsFloat.append(frameView->horizontalSnapOffsets()->at(i).round());
> 
> you're .at()ing again

Good catch. Fixed.

>> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:112
>> +    if (frameView->verticalSnapOffsets()) {
> 
> perhaps a reusable static snapOffsetsAsFloats or something? this code is here four times

So much cleaner :)

>> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:86
>> +#if ENABLE(CSS_SCROLL_SNAP)
> 
> This doesn't belong in the middle of the big block if it has #if, but I don't think it needs the #if, either.

Got it. Removed #if.

>> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:1306
>> +    if (_page->mainFrame() && _page->mainFrame()->page() && _page->mainFrame()->page()->scrollingCoordinatorProxy()) {
> 
> I wonder if this code can go somewhere more scrollingy (RemoteScrollingCoordinatorProxy?) and we can just call something->adjustTargetContentOffsetForSnapping(targetContentOffset) or something.

Added adjustTargetContentOffsetForSnapping to RemoteScrollingCoordinatorProxy.

>> Source/WebKit2/UIProcess/Scrolling/ios/ScrollingTreeOverflowScrollingNodeIOS.mm:33
>> +#if ENABLE(CSS_SCROLL_SNAP)
> 
> no #if blocks inside the big block of #imports!

Got it -- fixed.

>> Source/WebKit2/UIProcess/Scrolling/ios/ScrollingTreeOverflowScrollingNodeIOS.mm:206
>> +        // FIXME: In only one axis snaps in 2D scrolling, the other axis will decelerate fast as well. Is this what we want?
> 
> there's some mixup at the beginning of this sentence

Oops, good catch! (it's supposed to be "if")

>>> Source/WebKit2/UIProcess/ios/RemoteScrollingCoordinatorProxyIOS.mm:112
>>> +bool RemoteScrollingCoordinatorProxy::shouldHorizontalSnapForMainframeScrolling() const
>> 
>> To prevent requiring ScrollTypes, I split these methods into horizontal and vertical versions. Otherwise, I could pass in a WebCore::ScrollEventAxis
> 
> Deduplicate!

Re-merged into 1 method.

>> Source/WebKit2/UIProcess/ios/RemoteScrollingCoordinatorProxyIOS.mm:143
>> +float RemoteScrollingCoordinatorProxy::closestVerticalSnapOffsetForMainframeScrolling(float scrollDestination, float velocity) const
> 
> deduplicate!

Fixed.

[Tim Horton]

Comment on attachment 236615 [details]
Patch

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

> Source/WebCore/rendering/RenderLayer.cpp:3314
> +    // FIXME: Ensure that offsets are also updated in case of programmatic style changes.

Is there a bugzilla bug for this? If so, put the number here.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:3774
> +            if (layer.horizontalSnapOffsets())

if (auto& offsets = layer.horizontalSnapOffsets())
    scrollingGeometry.horizontalSnapOffsets = offsets;

maybe?

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:1304
> +    if (_page->mainFrame() && _page->mainFrame()->page() && _page->mainFrame()->page()->scrollingCoordinatorProxy()) {

All of this incrementally grabbing optional things is an unusual style for us. Usually assignment to a local and then early returning if null is more common, I think.

> Source/WebKit2/UIProcess/ios/RemoteScrollingCoordinatorProxyIOS.mm:41
> +#include <WebCore/AxisScrollSnapOffsets.h>

these should all be #import :| including the ones above :|

> Source/WebKit2/UIProcess/ios/RemoteScrollingCoordinatorProxyIOS.mm:119
> +        float potentialSnapPosition = closestSnapOffsetForMainframeScrolling(WebCore::ScrollEventAxis::Horizontal, targetContentOffset->x, velocity.x);

the f in Mainframe should be capitalized

[Wenson Hsieh]

Created attachment 236637 [details]
Patch

[Benjamin Poulain]

Comment on attachment 236637 [details]
Patch

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

Some minor comments. I haven't looked into correctness at all yet.

> Source/WebCore/WebCore.exp.in:3014
> +#if ENABLE(CSS_SCROLL_SNAP)

That's not a great flag name :(

> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:64
> +static inline void setStateScrollingNodeSnapOffsetsAsFloat(ScrollEventAxis axis, ScrollingStateScrollingNode* node, const Vector<LayoutUnit>& snapOffsets, float deviceScaleFactor)
> +{
> +    ASSERT(node);

Node should be passed as a reference instead of asserting the Node is non null.

It is odd the output parameter is the second one, shouldn't it be the first?

> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:66
> +    snapOffsetsAsFloat.reserveCapacity(snapOffsets.size());

reserveCapacity -> reserveInitialCapacity

> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:68
> +    for (size_t i = 0; i < snapOffsets.size(); i++)
> +        snapOffsetsAsFloat.append(roundToDevicePixel(snapOffsets[i], deviceScaleFactor, false));

i++ -> ++i

This does not seem right. How can you round to device pixel without the current page scale factor?
Shouldn't you convert to float and let the UIProces do the rounding?

> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:117
> +    if (frameView->horizontalSnapOffsets())
> +        setStateScrollingNodeSnapOffsetsAsFloat(ScrollEventAxis::Horizontal, node, *frameView->horizontalSnapOffsets(), frameView->frame().document()->deviceScaleFactor());

if (const Vector<LayoutUnit>* horizontalSnapOffsets = frameView->horizontalSnapOffsets())
   setXXX(..., horizontalSnapOffsets, ...)

> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.h:46
> +T closestSnapOffset(const Vector<T>& snapOffsets, T scrollDestination, float velocity)

This code is used with floats and doubles for the velocity. You should templatize "velocity" (can be implicit).

The value of scrollDestination can also be a CGFloat.

I don't get the template here really.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:3776
> +            if (auto* offsets = layer.horizontalSnapOffsets())
> +                scrollingGeometry.horizontalSnapOffsets = *offsets;
> +            if (auto* offsets = layer.verticalSnapOffsets())

I disagree with auto here, the type is not obvious.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:1305
> +        CGSize maxScrollDimensions = CGSizeMake(scrollView.contentSize.width - scrollView.bounds.size.width, scrollView.contentSize.height - scrollView.bounds.size.height);

This looks very weird, can you explain what you are trying to do?

Don't forget WKWebView has two additional kind of insets.

> Source/WebKit2/UIProcess/Scrolling/ios/ScrollingTreeOverflowScrollingNodeIOS.mm:83
> +    if (_scrollingTreeNode->horizontalSnapOffsets().size() > 0)

size() > 0 -> isEmpty().

> Source/WebKit2/UIProcess/Scrolling/ios/ScrollingTreeOverflowScrollingNodeIOS.mm:85
> +    if (_scrollingTreeNode->verticalSnapOffsets().size() > 0)

ditto.

[Wenson Hsieh]

Comment on attachment 236637 [details]
Patch

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

>> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:64
>> +    ASSERT(node);
> 
> Node should be passed as a reference instead of asserting the Node is non null.
> 
> It is odd the output parameter is the second one, shouldn't it be the first?

Fixed. Thanks!

>> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:66
>> +    snapOffsetsAsFloat.reserveCapacity(snapOffsets.size());
> 
> reserveCapacity -> reserveInitialCapacity

Fixed.

>> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:117
>> +        setStateScrollingNodeSnapOffsetsAsFloat(ScrollEventAxis::Horizontal, node, *frameView->horizontalSnapOffsets(), frameView->frame().document()->deviceScaleFactor());
> 
> if (const Vector<LayoutUnit>* horizontalSnapOffsets = frameView->horizontalSnapOffsets())
>    setXXX(..., horizontalSnapOffsets, ...)

Fixed. Looks much cleaner!

>> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.h:46
>> +T closestSnapOffset(const Vector<T>& snapOffsets, T scrollDestination, float velocity)
> 
> This code is used with floats and doubles for the velocity. You should templatize "velocity" (can be implicit).
> 
> The value of scrollDestination can also be a CGFloat.
> 
> I don't get the template here really.

Added comments explaining the template, as well as distinguished LayoutType from VelocityType.

>> Source/WebCore/rendering/RenderLayerCompositor.cpp:3776
>> +            if (auto* offsets = layer.verticalSnapOffsets())
> 
> I disagree with auto here, the type is not obvious.

Changed back to Vector<LayoutUnit>

>> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:1305
>> +        CGSize maxScrollDimensions = CGSizeMake(scrollView.contentSize.width - scrollView.bounds.size.width, scrollView.contentSize.height - scrollView.bounds.size.height);
> 
> This looks very weird, can you explain what you are trying to do?
> 
> Don't forget WKWebView has two additional kind of insets.

Added a comment describing the role of maxScrollDimensions. We'll probably have to change it to account for the insets.

>> Source/WebKit2/UIProcess/Scrolling/ios/ScrollingTreeOverflowScrollingNodeIOS.mm:83
>> +    if (_scrollingTreeNode->horizontalSnapOffsets().size() > 0)
> 
> size() > 0 -> isEmpty().

Changed. Thanks!

>> Source/WebKit2/UIProcess/Scrolling/ios/ScrollingTreeOverflowScrollingNodeIOS.mm:85
>> +    if (_scrollingTreeNode->verticalSnapOffsets().size() > 0)
> 
> ditto.

Changed.

[Wenson Hsieh]

Created attachment 236651 [details]
Patch

[Wenson Hsieh]

Comment on attachment 236651 [details]
Patch

This patch is covering a lot of different areas, and I think it's a good idea to break it down into several small patches. I'll be leaving comments to what parts should be separated out into individual patches.

[Wenson Hsieh]

Comment on attachment 236651 [details]
Patch

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

> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:-49
> -                LayoutUnit lastPotentialSnapPositionX = LayoutUnit(position.x()) + valueForLength(coordinate.first, viewWidth);

This should be in a separate patch. It seems localToContainerPoint was giving me strange offsets for child elements, so I resorted to using offset(Top|Left) on the child element instead, with a FIXME indicating that using offset(Left|Top) wouldn't take into account transforms.

> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.h:44
> +// closestSnapOffset is a templated function that takes in a Vector representing snap offsets as LayoutTypes (e.g. LayoutUnit or float) and

These fixes to closestSnapOffset add a template for velocity as well as snapOffset.

> Source/WebKit2/UIProcess/Scrolling/ios/ScrollingTreeOverflowScrollingNodeIOS.mm:209
> +            scrollView.decelerationRate = horizontalSnapOffsets().size() || verticalSnapOffsets().size() ? UIScrollViewDecelerationRateFast : UIScrollViewDecelerationRateNormal;

(oops, I meant to change size() here to !isEmpty() as well)

> Source/WebKit2/UIProcess/ios/RemoteScrollingCoordinatorProxyIOS.mm:-26
> -#include "config.h"

As Tim mentioned, these should have been using #import all along instead of #include.

> Source/WebKit2/UIProcess/ios/RemoteScrollingCoordinatorProxyIOS.mm:-32
> -#include "LayerRepresentation.h"

(and these too: we should just have a patch that changes these to use #import)

[Brent Fulgham]

Comment on attachment 236651 [details]
Patch

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

r=me. Please file a bug to implement layout tests for this (as we discussed in person).

> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:64
> +    // FIXME: Incorporate current page scale factor in snapping to device pixel. Perhaps we should just convert to float here and let UI process do the pixel snapping?

Pet peeve: FIXME should have a bug # in it so we don't forget to fix!

> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:67
> +    for (size_t i = 0; i < snapOffsets.size(); ++i)

How about a C++11 iterator here?

> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:47
> +            // FIXME: Investigate why using localToContainerPoint gives the wrong offsets for iOS mainframe. Also, these offsets won't take transforms into account (make sure to test this!)

Bug # please

> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:51
> +            // FIXME: Check that localToContainerPoint works with CSS rotations.

Ditto.

>> Source/WebKit2/UIProcess/Scrolling/ios/ScrollingTreeOverflowScrollingNodeIOS.mm:209
>> +            scrollView.decelerationRate = horizontalSnapOffsets().size() || verticalSnapOffsets().size() ? UIScrollViewDecelerationRateFast : UIScrollViewDecelerationRateNormal;
> 
> (oops, I meant to change size() here to !isEmpty() as well)

Please fix!

> Source/WebKit2/UIProcess/ios/RemoteScrollingCoordinatorProxyIOS.mm:123
> +    // FIXME: We need to account for how the top navigation bar changes in size.

This seems like an important bug to file so we don't forget!

[WebKit Commit Bot]

Comment on attachment 236651 [details]
Patch

Clearing flags on attachment: 236651

Committed r172649: <http://trac.webkit.org/changeset/172649>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.