Bug 142523

<rdar://problem/20100753>

Created attachment 255031 [details] Evenly spaced 2D grid of elements Might be a useful example for testing 2D snapping behavior in an overflow div. The top-left corner of each element should snap to the top-left corner of the parent container.

Created attachment 255309 [details] Patch

Created attachment 255331 [details] Patch

Created attachment 255334 [details] Patch

Comment on attachment 255334 [details] Patch Attachment 255334 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/4709663559909376 New failing tests: platform/mac-wk2/tiled-drawing/scrolling/scroll-snap/scroll-snap-mandatory-mainframe-slow-vertical.html platform/mac-wk2/tiled-drawing/scrolling/scroll-snap/scroll-snap-mandatory-mainframe-horizontal.html platform/mac-wk2/tiled-drawing/scrolling/scroll-snap/scroll-snap-mandatory-mainframe-slow-horizontal.html platform/mac-wk2/tiled-drawing/scrolling/scroll-snap/scroll-snap-mandatory-mainframe-vertical.html

Created attachment 255335 [details] Archive of layout-test-results from ews106 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5

Created attachment 255337 [details] Patch

Comment on attachment 255337 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255337&action=review This looks very good! I feel like some adjustments to structure would be helpful to clean things up and making stuff easier to find. But I think this is looking great! > Source/WebCore/ChangeLog:3 > + Snap point regions containing X and Y snap points should do a better job animating "do a better job" -> "do a better job" (extra space) > Source/WebCore/platform/cocoa/ScrollController.mm:661 > + if (snapOnHorizontalAxis && ((m_horizontalScrollSnapState->m_targetOffset - m_horizontalScrollSnapState->m_initialOffset).toFloat() * m_horizontalScrollSnapState->m_initialScrollDelta < 0)) { This would be easier to read if the math involving the m_horizontalScrollSnapState was a static function. Then it could be shared with the vertical code, below. What is this calculation saying? Is it "remainingDistanceToTarget(m_horizontalScrollSnapState)"? Does this properly account for zoom state? > Source/WebCore/platform/cocoa/ScrollController.mm:672 > + } Need a blank line here. > Source/WebCore/platform/cocoa/ScrollController.mm:674 > + float timeProgress = (currentTime - m_scrollSnapCurveState->m_startTime) / scrollSnapDuration; What if this was encapsulated as "float timeProgress = m_scrollSnapCurveState->currentTimeProgress()"? Then scrollSnapDuration could be encapsulated in the scroll snap curve state object? > Source/WebCore/platform/cocoa/ScrollController.mm:689 > + } Need a blank line here. > Source/WebCore/platform/cocoa/ScrollController.mm:692 > + float curveProgress = fmin(1, curveMagnitude * (1 - pow(decayFactor, -60 * scrollSnapDuration * timeProgress))); std::min and std::pow, please. We're using C++ after all! :-) Also, what if this was a method on the ScrollSnapAnimationCurveState structure? "m_scrollSnapCurveState->currentCurveProgress()" or something like that? Then 'curveMagnitude' and 'decayFactor' wouldn't need to be shown here. > Source/WebCore/platform/cocoa/ScrollController.mm:697 > + horizontalDelta = m_horizontalScrollSnapState->m_initialOffset + curveProgress * (m_horizontalScrollSnapState->m_targetOffset - m_horizontalScrollSnapState->m_initialOffset) - m_client.scrollOffsetOnAxis(ScrollEventAxis::Horizontal); It seems like this calculation could be something m_horizontalScrollSnapState could be responsible for. Likewise for the vertical case, below. > Source/WebCore/platform/cocoa/ScrollController.mm:703 > + for (int i = 0; i < 4; i++) Our style guide is for "++i" > Source/WebCore/platform/cocoa/ScrollController.mm:704 > + interpolatedVector += pow(curveProgress, i) * m_scrollSnapCurveState->m_snapAnimationCurveCoefficients[i]; Seems like this calculation should be encapsulated in scrollSnapCurveState. Also, please use "std::pow". > Source/WebCore/platform/cocoa/ScrollController.mm:-843 > -float ScrollController::computeSnapDelta(ScrollEventAxis axis) const I'm sad to see these comments go, as I found them to be helpful when understanding the original code. > Source/WebCore/platform/cocoa/ScrollController.mm:854 > + float initialProgress = fmaxf(minScrollSnapInitialProgress, fminf(initialDeltaMagnitude / startToEndDistance, maxScrollSnapInitialProgress)); std::max, std::min. > Source/WebCore/platform/cocoa/ScrollController.mm:856 > + float decayFactor; Please initialize this to zero or something. > Source/WebCore/platform/cocoa/ScrollController.mm:858 > + for (int i = 0; i < maxNumScrollSnapParameterEstimationIterations; i++) { Our style guide is to use "++i" > Source/WebCore/platform/cocoa/ScrollController.mm:860 > + curveMagnitude = 1 / (1 - powf(decayFactor, -60 * scrollSnapDuration)); std::pow, please. You might need to write "-60.0f" > Source/WebCore/platform/cocoa/ScrollController.mm:861 > + if (fabsf(decayFactor - previousDecayFactor) < scrollSnapDecayFactorConvergenceThreshold) std::abs, please. > Source/WebCore/platform/cocoa/ScrollController.mm:869 > + m_scrollSnapCurveState->m_shouldAnimateDirectlyToSnapPoint = true; It feels like all of the math involved in setting up these curve state parameters should live inside the scrollSnapCurveState object. I also feel like you should have comments on the function like you used to explaining what these different terms mean. > Source/WebCore/platform/cocoa/ScrollController.mm:874 > + float cosTheta = initialDelta.isZero() ? 0 : (initialDelta.width() * startToEndVector.width() + initialDelta.height() * startToEndVector.height()) / (fmaxf(1, initialDeltaMagnitude) * startToEndDistance); std::max. You might need to write 1.0f. > Source/WebCore/platform/cocoa/ScrollController.mm:878 > + float sideLength = startToEndDistance / (2 * cosTheta + 1); You might wand to write (2.0f * cosTheta + 1.0f) > Source/WebCore/platform/cocoa/ScrollController.mm:885 > + m_scrollSnapCurveState->m_shouldAnimateDirectlyToSnapPoint = false; Again, the setup of these scrollSnapCurveState parameters seem like they should be done in a method on the scrollSnapCurveState class, with comments about what we are doing here. > Source/WebCore/platform/cocoa/ScrollController.mm:-890 > -float ScrollController::computeGlideDelta(ScrollEventAxis axis) const Again, I feel like we've lost some documentation of what's going on in all of this code. > Source/WebCore/platform/cocoa/ScrollController.mm:891 > + return m_horizontalScrollSnapState && (m_horizontalScrollSnapState->m_currentState == ScrollSnapState::Snapping || m_horizontalScrollSnapState->m_currentState == ScrollSnapState::Gliding); I suggest a method on scrollSnapState, "isSnapping" that encapsulates the check for Snapping and Gliding. Then this would just be "return m_horizontalScrollSnapState && m_horizontalScrollSnapState->isSnapping()". Ditto below. > Source/WebCore/platform/cocoa/ScrollSnapAnimatorState.mm:56 > + for (int i = 0; i < m_numWheelDeltasTracked; i++) { We usually write ++i.

Comment on attachment 255337 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255337&action=review Thanks for the review! I've addressed these issues, and I'll have a new patch up soon (with tests as well). >> Source/WebCore/platform/cocoa/ScrollController.mm:661 >> + if (snapOnHorizontalAxis && ((m_horizontalScrollSnapState->m_targetOffset - m_horizontalScrollSnapState->m_initialOffset).toFloat() * m_horizontalScrollSnapState->m_initialScrollDelta < 0)) { > > This would be easier to read if the math involving the m_horizontalScrollSnapState was a static function. Then it could be shared with the vertical code, below. > > What is this calculation saying? Is it "remainingDistanceToTarget(m_horizontalScrollSnapState)"? Does this properly account for zoom state? This has been fixed. The purpose of this code was simply to check whether or not the scroll direction was compatible with the initial and target offsets (i.e. the user isn't scrolling one way when the snap point is the opposite direction). I made this more explicit by moving this out to ScrollSnapAnimatorState::canReachTargetWithCurrentInitialScrollDelta. >> Source/WebCore/platform/cocoa/ScrollController.mm:672 >> + } > > Need a blank line here. Fixed! >> Source/WebCore/platform/cocoa/ScrollController.mm:674 >> + float timeProgress = (currentTime - m_scrollSnapCurveState->m_startTime) / scrollSnapDuration; > > What if this was encapsulated as "float timeProgress = m_scrollSnapCurveState->currentTimeProgress()"? Then scrollSnapDuration could be encapsulated in the scroll snap curve state object? Fixed! Added ScrollSnapAnimationCurveState::animationProgressAtTime. >> Source/WebCore/platform/cocoa/ScrollController.mm:689 >> + } > > Need a blank line here. Fixed! >> Source/WebCore/platform/cocoa/ScrollController.mm:692 >> + float curveProgress = fmin(1, curveMagnitude * (1 - pow(decayFactor, -60 * scrollSnapDuration * timeProgress))); > > std::min and std::pow, please. We're using C++ after all! :-) > > Also, what if this was a method on the ScrollSnapAnimationCurveState structure? "m_scrollSnapCurveState->currentCurveProgress()" or something like that? Then 'curveMagnitude' and 'decayFactor' wouldn't need to be shown here. Good point. Added ScrollSnapAnimationCurveState::animationProgressAtTime. (also added the std::) >> Source/WebCore/platform/cocoa/ScrollController.mm:697 >> + horizontalDelta = m_horizontalScrollSnapState->m_initialOffset + curveProgress * (m_horizontalScrollSnapState->m_targetOffset - m_horizontalScrollSnapState->m_initialOffset) - m_client.scrollOffsetOnAxis(ScrollEventAxis::Horizontal); > > It seems like this calculation could be something m_horizontalScrollSnapState could be responsible for. Likewise for the vertical case, below. Fixed! Added ScrollSnapAnimatorState::interpolatedOffsetAtProgress. >> Source/WebCore/platform/cocoa/ScrollController.mm:703 >> + for (int i = 0; i < 4; i++) > > Our style guide is for "++i" Fixed! >> Source/WebCore/platform/cocoa/ScrollController.mm:704 >> + interpolatedVector += pow(curveProgress, i) * m_scrollSnapCurveState->m_snapAnimationCurveCoefficients[i]; > > Seems like this calculation should be encapsulated in scrollSnapCurveState. Also, please use "std::pow". Fixed! >> Source/WebCore/platform/cocoa/ScrollController.mm:-843 >> -float ScrollController::computeSnapDelta(ScrollEventAxis axis) const > > I'm sad to see these comments go, as I found them to be helpful when understanding the original code. No worries! I've added in comments detailing my new animation scheme to ScrollSnapAnimatorState.mm. >> Source/WebCore/platform/cocoa/ScrollController.mm:854 >> + float initialProgress = fmaxf(minScrollSnapInitialProgress, fminf(initialDeltaMagnitude / startToEndDistance, maxScrollSnapInitialProgress)); > > std::max, std::min. Fixed! >> Source/WebCore/platform/cocoa/ScrollController.mm:856 >> + float decayFactor; > > Please initialize this to zero or something. Fixed! (removed this variable) >> Source/WebCore/platform/cocoa/ScrollController.mm:858 >> + for (int i = 0; i < maxNumScrollSnapParameterEstimationIterations; i++) { > > Our style guide is to use "++i" Fixed! >> Source/WebCore/platform/cocoa/ScrollController.mm:860 >> + curveMagnitude = 1 / (1 - powf(decayFactor, -60 * scrollSnapDuration)); > > std::pow, please. You might need to write "-60.0f" Fixed! >> Source/WebCore/platform/cocoa/ScrollController.mm:861 >> + if (fabsf(decayFactor - previousDecayFactor) < scrollSnapDecayFactorConvergenceThreshold) > > std::abs, please. Fixed! >> Source/WebCore/platform/cocoa/ScrollController.mm:869 >> + m_scrollSnapCurveState->m_shouldAnimateDirectlyToSnapPoint = true; > > It feels like all of the math involved in setting up these curve state parameters should live inside the scrollSnapCurveState object. I also feel like you should have comments on the function like you used to explaining what these different terms mean. Got it. Moved the logic over to the curve state and added documentation. >> Source/WebCore/platform/cocoa/ScrollController.mm:874 >> + float cosTheta = initialDelta.isZero() ? 0 : (initialDelta.width() * startToEndVector.width() + initialDelta.height() * startToEndVector.height()) / (fmaxf(1, initialDeltaMagnitude) * startToEndDistance); > > std::max. You might need to write 1.0f. Fixed! >> Source/WebCore/platform/cocoa/ScrollController.mm:878 >> + float sideLength = startToEndDistance / (2 * cosTheta + 1); > > You might wand to write (2.0f * cosTheta + 1.0f) Fixed! >> Source/WebCore/platform/cocoa/ScrollController.mm:885 >> + m_scrollSnapCurveState->m_shouldAnimateDirectlyToSnapPoint = false; > > Again, the setup of these scrollSnapCurveState parameters seem like they should be done in a method on the scrollSnapCurveState class, with comments about what we are doing here. Got it! Moved this logic out into a separate method in the animation curve state, with documentation. >> Source/WebCore/platform/cocoa/ScrollController.mm:-890 >> -float ScrollController::computeGlideDelta(ScrollEventAxis axis) const > > Again, I feel like we've lost some documentation of what's going on in all of this code. I removed this big comment, since I'm no longer using this equation for my snapping animation. I'll add in some documentation that explains my new animation scheme (see ScrollSnapAnimatorState.h). >> Source/WebCore/platform/cocoa/ScrollController.mm:891 >> + return m_horizontalScrollSnapState && (m_horizontalScrollSnapState->m_currentState == ScrollSnapState::Snapping || m_horizontalScrollSnapState->m_currentState == ScrollSnapState::Gliding); > > I suggest a method on scrollSnapState, "isSnapping" that encapsulates the check for Snapping and Gliding. Then this would just be "return m_horizontalScrollSnapState && m_horizontalScrollSnapState->isSnapping()". Ditto below. Sounds good! Much more readable. >> Source/WebCore/platform/cocoa/ScrollSnapAnimatorState.mm:56 >> + for (int i = 0; i < m_numWheelDeltasTracked; i++) { > > We usually write ++i. Fixed!

Created attachment 255399 [details] Patch

Created attachment 255414 [details] Patch

Comment on attachment 255414 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255414&action=review > Source/WebCore/ChangeLog:4 > + https://bugs.webkit.org/show_bug.cgi?id=142523 Please add <rdar://problem/20100753> here, followed by a blank line. > Source/WebCore/ChangeLog:5 > + Reviewed by NOBODY (OOPS!). Need a blank line after this. > Source/WebCore/ChangeLog:10 > + the snapping animation across both axes. Note: We should probably make this fixed time configurable. We have some notes to discuss this with the css working group. > Source/WebCore/ChangeLog:29 > + to unify both snap scrolling and rubber banding timers to solve this issue. Can you file a bug about this so we don't forget? > Source/WebCore/ChangeLog:36 > + (WebCore::ScrollController::startScrollSnapTimer): Refactored to use a use a what!?! :-) > Source/WebCore/platform/cocoa/ScrollSnapAnimatorState.mm:97 > +static const CFTimeInterval scrollSnapAnimationDuration = 0.5; Note: These seem like they should be configureable. (Not in this patch). CSS styling for animation might be a good model for allowing web authors to control behavior.

Comment on attachment 255414 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255414&action=review > Source/WebCore/platform/cocoa/ScrollController.mm:668 > + CFTimeInterval currentTime = [NSDate timeIntervalSinceReferenceDate]; This should use monotonicallyIncreasingTime() or std::chrono > Source/WebCore/platform/cocoa/ScrollSnapAnimatorState.h:84 > + bool shouldCompleteSnapAnimationImmediatelyAtTime(CFTimeInterval) const; > + float animationProgressAtTime(CFTimeInterval) const; These should not use CFTImeInterval; use double or std::chrono. > Source/WebCore/platform/cocoa/ScrollSnapAnimatorState.h:90 > + CFTimeInterval m_startTime { 0 }; > + float m_snapAnimationCurveMagnitude { 0 }; > + float m_snapAnimationDecayFactor { 0 }; > + FloatSize m_snapAnimationCurveCoefficients[4] { }; > + bool m_shouldAnimateDirectlyToSnapPoint { false }; We don't generally prefix public struct members with m_ > Source/WebCore/platform/cocoa/ScrollSnapAnimatorState.mm:122 > + * Computes and sets parameters required for tracking the progress of a snap animation curve, interpolated > + * or linear. The progress curve s(t) maps time t to progress s; both variables are in the interval [0, 1]. > + * The time input t is 0 when the current time is the start of the animation, t = m_startTime, and 1 when the > + * current time is at or after the end of the animation, t = m_startTime + m_scrollSnapAnimationDuration. > + * > + * In this exponential progress model, s(t) = A - A * b^(-kt), where k = 60T is the number of frames in the > + * animation (assuming 60 FPS and an animation duration of T) and A, b are reals greater than or equal to 1. > + * Also note that we are given the initial progress, a value indicating the portion of the curve which our > + * initial scroll delta takes us. This is important when matching the initial speed of the animation to the > + * user's initial momentum scrolling speed. Let this initial progress amount equal v_0. I clamp this initial > + * progress amount to a minimum or maximum value. > + * > + * A is referred to as the curve magnitude, while b is referred to as the decay factor. We solve for A and b, > + * keeping the following constraints in mind: > + * 1. s(0) = 0 > + * 2. s(1) = 1 > + * 3. s(1/k) = v_0 > + * > + * First, observe that s(0) = 0 holds for appropriate values of A, b. Solving for the remaining constraints > + * yields a nonlinear system of two equations. In lieu of a purely analytical solution, an alternating > + * optimization scheme is used to approximate A and b. This technique converges quickly (within 5 iterations > + * or so) for appropriate values of v_0. The optimization terminates early when the decay factor changes by > + * less than a threshold between one iteration and the next. I don't know if we should be doing all this ourselves. We really want to match AppKit animations. > Source/WebCore/platform/cocoa/ScrollSnapAnimatorState.mm:137 > + m_startTime = [NSDate timeIntervalSinceReferenceDate]; monotonicallyIncreasingTime or std::chrono please.

Comment on attachment 255414 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255414&action=review Thanks for the reviews, Simon and Brent! I've fixed the more straightforward issues, but I'm not sure what the best course of action right now will be regarding the math-heavy scroll animation code. Perhaps one of you might be free this week for a brief chat about it? >> Source/WebCore/ChangeLog:4 >> + https://bugs.webkit.org/show_bug.cgi?id=142523 > > Please add <rdar://problem/20100753> here, followed by a blank line. Added. >> Source/WebCore/ChangeLog:5 >> + Reviewed by NOBODY (OOPS!). > > Need a blank line after this. Fixed! >> Source/WebCore/ChangeLog:10 >> + the snapping animation across both axes. > > Note: We should probably make this fixed time configurable. We have some notes to discuss this with the css working group. Sounds good. I'll keep that in mind. The animation duration is a constant declared in ScrollSnapAnimatorState.mm, and can be easily tweaked to be passed in via a CSS property. >> Source/WebCore/ChangeLog:29 >> + to unify both snap scrolling and rubber banding timers to solve this issue. > > Can you file a bug about this so we don't forget? Will do! >> Source/WebCore/ChangeLog:36 >> + (WebCore::ScrollController::startScrollSnapTimer): Refactored to use a > > use a what!?! :-) Whoops, good catch! Fixed. >> Source/WebCore/platform/cocoa/ScrollController.mm:668 >> + CFTimeInterval currentTime = [NSDate timeIntervalSinceReferenceDate]; > > This should use monotonicallyIncreasingTime() or std::chrono Refactored to use monotonicallyIncreasingTime and double type. Thanks for the heads up! >> Source/WebCore/platform/cocoa/ScrollSnapAnimatorState.h:84 >> + float animationProgressAtTime(CFTimeInterval) const; > > These should not use CFTImeInterval; use double or std::chrono. Refactored to use monotonicallyIncreasingTime and double type. Thanks for the heads up! >> Source/WebCore/platform/cocoa/ScrollSnapAnimatorState.h:90 >> + bool m_shouldAnimateDirectlyToSnapPoint { false }; > > We don't generally prefix public struct members with m_ Got it. I made most of the members private, now that the logic concerning them is in ScrollSnapAnimationCurveState. It looks like the ScrollSnapAnimatorState public members also appear to be prefixed with m_ -- do you think I should also rename those as well? >> Source/WebCore/platform/cocoa/ScrollSnapAnimatorState.mm:97 >> +static const CFTimeInterval scrollSnapAnimationDuration = 0.5; > > Note: These seem like they should be configureable. (Not in this patch). CSS styling for animation might be a good model for allowing web authors to control behavior. Good idea. I'll remember this (is there someone I should contact?) >> Source/WebCore/platform/cocoa/ScrollSnapAnimatorState.mm:122 >> + * less than a threshold between one iteration and the next. > > I don't know if we should be doing all this ourselves. We really want to match AppKit animations. That would definitely be preferable. I looked for a built-in way of generating an animation curve given initial velocity and destination, but I wasn't able to find what I needed (for iOS, I retargeted some parameters passed into a delegate method scrollViewWillEndDragging, but that seems to only be in UIKit). >> Source/WebCore/platform/cocoa/ScrollSnapAnimatorState.mm:137 >> + m_startTime = [NSDate timeIntervalSinceReferenceDate]; > > monotonicallyIncreasingTime or std::chrono please. Fixed!

Created attachment 255447 [details] Patch

Created attachment 255951 [details] Patch

Comment on attachment 255951 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255951&action=review r=me. > Source/WebCore/ChangeLog:32 > + to unify both snap scrolling and rubber banding timers to solve this issue. Does this mean that rubber banding is broken at the edge of a scroll snap container?

Comment on attachment 255951 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255951&action=review >> Source/WebCore/ChangeLog:32 >> + to unify both snap scrolling and rubber banding timers to solve this issue. > > Does this mean that rubber banding is broken at the edge of a scroll snap container? With this patch, rubber banding effectively is disabled when scrolling diagonally while hitting the edge of a scroll snap container (it still works when scroll snapping in 1D).

Comment on attachment 255951 [details] Patch Rejecting attachment 255951 [details] from commit-queue. wenson_hsieh@apple.com does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json. - If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your committer rights.

Comment on attachment 255951 [details] Patch Clearing flags on attachment: 255951 Committed r186469: <http://trac.webkit.org/changeset/186469>

All reviewed patches have been landed. Closing bug.