Bug 142522

Summary: Overflow regions with scroll snap points are not reliably rubber banding
Product: WebKit Reporter: Brent Fulgham <bfulgham>
Component: Layout and RenderingAssignee: Brent Fulgham <bfulgham>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, cmarcelo, commit-queue, jamesr, luiz, tonikitoo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: All   
Attachments:
Description Flags
Patch darin: review+

Brent Fulgham
Reported 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.
Attachments
Patch (10.11 KB, patch)
2015-06-17 13:39 PDT, Brent Fulgham 		darin: review+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2015-03-09 19:23:15 PDT
<rdar://problem/20100726>
Brent Fulgham
Comment 2 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.
Brent Fulgham
Comment 3 2015-06-17 13:39:47 PDT
Created attachment 255033 [details] Patch
WebKit Commit Bot
Comment 4 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.
Darin Adler
Comment 5 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.
Brent Fulgham
Comment 6 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.
Brent Fulgham
Comment 7 2015-06-17 17:33:12 PDT
Committed r185681: <http://trac.webkit.org/changeset/185681>
Note You need to log in before you can comment on or make changes to this bug.