Overflow regions with scroll snap points are not reliably rubber banding

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Brent Fulgham]

The rubber band animation is not reliably triggering when scrolling regions with scroll snap points.

This seems to be due to the scroll snap point animation. Probably, the rubber band animation is getting tossed aside in favor of the scroll snap point animation. We probably need to be checking if our scroll takes us past the snap point bounds, so that we can ignore snap point animation and make sure we trigger the rubber band operation.

[Radar WebKit Bug Importer]

<rdar://problem/20100726>

[Brent Fulgham]

This is happening because we clamp our scroll targets to scroll-snap-points, which means we are guaranteed to never reach the rubber-band phase of scrolling.

Instead, we need to recognize when our gesture will take us past either extreme of the scrolling container, and skip scroll-snap for that case.

[Brent Fulgham]

Created attachment 255033 [details]
Patch

[WebKit Commit Bot]

Attachment 255033 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/Scrolling/ios/ScrollingTreeOverflowScrollingNodeIOS.mm:87:  The parameter name ">" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebKit2/UIProcess/Scrolling/ios/ScrollingTreeOverflowScrollingNodeIOS.mm:89:  The parameter name ">" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 255033 [details]
Patch

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

> Source/WebCore/page/scrolling/mac/ScrollingTreeFrameScrollingNodeMac.mm:593
> +    return LayoutSize(totalContentsSize());

I don’t think the explicit conversion to LayoutSize is needed here.

> Source/WebCore/platform/ScrollAnimator.cpp:220
> +    return LayoutSize(m_scrollableArea.contentsSize());

I don’t think the explicit conversion to LayoutSize is needed here.

> Source/WebCore/platform/cocoa/ScrollController.h:33
> +#include "LayoutSize.h"

To compile something with the return value LayoutSize, we only need a forward declaration of LayoutSize, not the entire LayoutSize.h header.

> Source/WebCore/platform/cocoa/ScrollController.h:107
> +    virtual LayoutSize scrollExtents() const = 0;

I think the right name for this is scrollExtent, not scrollExtents.

[Brent Fulgham]

Comment on attachment 255033 [details]
Patch

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

>> Source/WebCore/page/scrolling/mac/ScrollingTreeFrameScrollingNodeMac.mm:593
>> +    return LayoutSize(totalContentsSize());
> 
> I don’t think the explicit conversion to LayoutSize is needed here.

OK.

>> Source/WebCore/platform/ScrollAnimator.cpp:220
>> +    return LayoutSize(m_scrollableArea.contentsSize());
> 
> I don’t think the explicit conversion to LayoutSize is needed here.

OK.

>> Source/WebCore/platform/cocoa/ScrollController.h:33
>> +#include "LayoutSize.h"
> 
> To compile something with the return value LayoutSize, we only need a forward declaration of LayoutSize, not the entire LayoutSize.h header.

OK.

>> Source/WebCore/platform/cocoa/ScrollController.h:107
>> +    virtual LayoutSize scrollExtents() const = 0;
> 
> I think the right name for this is scrollExtent, not scrollExtents.

OK.

[Brent Fulgham]

Committed r185681: <http://trac.webkit.org/changeset/185681>