Bug 142522 - Overflow regions with scroll snap points are not reliably rubber banding
Summary: Overflow regions with scroll snap points are not reliably rubber banding
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac All
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-03-09 19:22 PDT by Brent Fulgham
Modified: 2015-06-17 17:33 PDT (History)
7 users (show)

See Also:


Attachments
Patch (10.11 KB, patch)
2015-06-17 13:39 PDT, Brent Fulgham
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 2015-03-09 19:22:28 PDT
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.
Comment 1 Radar WebKit Bug Importer 2015-03-09 19:23:15 PDT
<rdar://problem/20100726>
Comment 2 Brent Fulgham 2015-06-17 13:28:22 PDT
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.
Comment 3 Brent Fulgham 2015-06-17 13:39:47 PDT
Created attachment 255033 [details]
Patch
Comment 4 WebKit Commit Bot 2015-06-17 13:41:30 PDT
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.
Comment 5 Darin Adler 2015-06-17 16:02:05 PDT
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.
Comment 6 Brent Fulgham 2015-06-17 17:07:41 PDT
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.
Comment 7 Brent Fulgham 2015-06-17 17:33:12 PDT
Committed r185681: <http://trac.webkit.org/changeset/185681>