Source/WebCore:

[iOS] scroll snap points are animating to the wrong positions... https://bugs.webkit.org/show_bug.cgi?id=142705 <rdar://problem/20136946> Reviewed by Simon Fraser. Avoid adding an extra '0' snap point to our set. We always start with one zero; this extra append just forces us to do more steps in our search for nearest snap point. * page/scrolling/AxisScrollSnapOffsets.cpp: (WebCore::updateFromStyle): Remove extra '0' appended to offsets. Source/WebKit2: [iOS] scroll snap points are animating to the wrong positions. https://bugs.webkit.org/show_bug.cgi?id=142705 <rdar://problem/20136946> Reviewed by Simon Fraser. Scroll snapping was landing in the wrong place on iOS because of two problems: (1) It was searching for the closest snap offset point using unscaled 'screen' pixels, which caused it to always choose one of the earliest snap point options. (2) It was then selecting a scaled snap point coordinate and passing it back to UIKit to animate the snap. This caused it to select a target point beyond the 'screen' pixel we want to hit. The solution to both problems are to scale the scroll destination UIKit suggests so that we search among the scaled points with a valid value. Then, we need to scale the returned value back to screen units before handing it back to UIKit to process. * UIProcess/API/Cocoa/WKWebView.mm: (-[WKWebView scrollViewWillBeginDragging:]): Drive-by fix. Get rid of extra ';' at the end of the line. * UIProcess/ios/RemoteScrollingCoordinatorProxyIOS.mm: (WebKit::RemoteScrollingCoordinatorProxy::closestSnapOffsetForMainFrameScrolling): Canonical link: https://commits.webkit.org/160707@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@181504 268f45cc-cd09-0410-ab3c-d52691b4dbfc
WebKit · Mar 15, 2015 · 7bc2485 · 7bc2485
1 parent d79ebab
commit 7bc2485
Show file tree

Hide file tree

Showing 5 changed files with 48 additions and 3 deletions.
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
@@ -1,3 +1,17 @@
+2015-03-14  Brent Fulgham  <bfulgham@apple.com>
+
+        [iOS] scroll snap points are animating to the wrong positions...
+        https://bugs.webkit.org/show_bug.cgi?id=142705
+        <rdar://problem/20136946>
+
+        Reviewed by Simon Fraser.
+
+        Avoid adding an extra '0' snap point to our set. We always start with one zero; this
+        extra append just forces us to do more steps in our search for nearest snap point.
+
+        * page/scrolling/AxisScrollSnapOffsets.cpp:
+        (WebCore::updateFromStyle): Remove extra '0' appended to offsets.
+
 2015-03-14  Dean Jackson  <dino@apple.com>
 
         Feature flag for Animations Level 2

diff --git a/Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp b/Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp
@@ -83,7 +83,6 @@ static void updateFromStyle(Vector<LayoutUnit>& snapOffsets, const RenderStyle&
     LayoutUnit curSnapPositionShift = 0;
     LayoutUnit maxScrollOffset = scrollSize - viewSize;
     LayoutUnit lastSnapPosition = curSnapPositionShift;
-    snapOffsets.append(0);
     do {
         for (auto& snapPosition : snapOffsetSubsequence) {
             LayoutUnit potentialSnapPosition = curSnapPositionShift + snapPosition - destinationOffset;
@@ -98,6 +97,10 @@ static void updateFromStyle(Vector<LayoutUnit>& snapOffsets, const RenderStyle&
         }
         curSnapPositionShift = lastSnapPosition + repeatOffset;
     } while (hasRepeat && curSnapPositionShift < maxScrollOffset);
+
+    if (snapOffsets.isEmpty())
+        snapOffsets.append(0);
+
     // Always put a snap point on the maximum scroll offset.
     // Not a part of the spec, but necessary to prevent unreachable content when snapping.
     if (snapOffsets.last() != maxScrollOffset)

diff --git a/Source/WebKit2/ChangeLog b/Source/WebKit2/ChangeLog
@@ -1,3 +1,28 @@
+2015-03-14  Brent Fulgham  <bfulgham@apple.com>
+
+        [iOS] scroll snap points are animating to the wrong positions.
+        https://bugs.webkit.org/show_bug.cgi?id=142705
+        <rdar://problem/20136946>
+
+        Reviewed by Simon Fraser.
+
+        Scroll snapping was landing in the wrong place on iOS because of two problems:
+        (1) It was searching for the closest snap offset point using unscaled 'screen' pixels,
+        which caused it to always choose one of the earliest snap point options.
+        (2) It was then selecting a scaled snap point coordinate and passing it back to UIKit
+        to animate the snap. This caused it to select a target point beyond the 'screen' pixel
+        we want to hit.
+
+        The solution to both problems are to scale the scroll destination UIKit suggests so that
+        we search among the scaled points with a valid value. Then, we need to scale the returned
+        value back to screen units before handing it back to UIKit to process.
+
+        * UIProcess/API/Cocoa/WKWebView.mm:
+        (-[WKWebView scrollViewWillBeginDragging:]): Drive-by fix. Get rid of extra ';' at
+        the end of the line.
+        * UIProcess/ios/RemoteScrollingCoordinatorProxyIOS.mm:
+        (WebKit::RemoteScrollingCoordinatorProxy::closestSnapOffsetForMainFrameScrolling):
+
 2015-03-14  Dean Jackson  <dino@apple.com>
 
         Feature flag for Animations Level 2

diff --git a/Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm b/Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm
@@ -1339,7 +1339,7 @@ - (void)scrollViewWillBeginDragging:(UIScrollView *)scrollView
     // FIXME: We will want to detect whether snapping will occur before beginning to drag. See WebPageProxy::didCommitLayerTree.
     WebKit::RemoteScrollingCoordinatorProxy* coordinator = _page->scrollingCoordinatorProxy();
     ASSERT(scrollView == _scrollView.get());
-    scrollView.decelerationRate = (coordinator && coordinator->shouldSetScrollViewDecelerationRateFast()) ? UIScrollViewDecelerationRateFast : [_scrollView preferredScrollDecelerationFactor];;
+    scrollView.decelerationRate = (coordinator && coordinator->shouldSetScrollViewDecelerationRateFast()) ? UIScrollViewDecelerationRateFast : [_scrollView preferredScrollDecelerationFactor];
 #endif
 }
 

diff --git a/Source/WebKit2/UIProcess/ios/RemoteScrollingCoordinatorProxyIOS.mm b/Source/WebKit2/UIProcess/ios/RemoteScrollingCoordinatorProxyIOS.mm
@@ -149,7 +149,10 @@ static LayerRepresentation layerRepresentationFromLayerOrView(LayerOrView *layer
     ASSERT(root && root->isFrameScrollingNode());
     ScrollingTreeFrameScrollingNode* rootFrame = static_cast<ScrollingTreeFrameScrollingNode*>(root);
     const Vector<float>& snapOffsets = axis == ScrollEventAxis::Horizontal ? rootFrame->horizontalSnapOffsets() : rootFrame->verticalSnapOffsets();
-    return closestSnapOffset<float, float>(snapOffsets, scrollDestination, velocity);
+
+    float scaledScrollDestination = scrollDestination / m_webPageProxy.displayedContentScale();
+    float rawClosestSnapOffset = closestSnapOffset<float, float>(snapOffsets, scaledScrollDestination, velocity);
+    return rawClosestSnapOffset * m_webPageProxy.displayedContentScale();
 }
 #endif