Changeset 181504 in webkit

Timestamp:

Mar 14, 2015, 10:11:19 PM (11 years ago)

Author:

Brent Fulgham

Message:

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):

Location:

trunk/Source

Files:

• WebCore/ChangeLog (modified) (1 diff)
• WebCore/page/scrolling/AxisScrollSnapOffsets.cpp (modified) (2 diffs)
• WebKit2/ChangeLog (modified) (1 diff)
• WebKit2/UIProcess/API/Cocoa/WKWebView.mm (modified) (1 diff)
• WebKit2/UIProcess/ios/RemoteScrollingCoordinatorProxyIOS.mm (modified) (1 diff)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp

@@ -84 +84 @@
     LayoutUnit maxScrollOffset = scrollSize - viewSize;
     LayoutUnit lastSnapPosition = curSnapPositionShift;
-    snapOffsets.append(0);
     do {
         for (auto& snapPosition : snapOffsetSubsequence) {
@@ -99 +98 @@
         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.

trunk/Source/WebKit2/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm

@@ -1340 +1340 @@
     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
 }

trunk/Source/WebKit2/UIProcess/ios/RemoteScrollingCoordinatorProxyIOS.mm

@@ -150 +150 @@
     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