Bug 145059 – [iOS] Remote scrolling tree needs to coordinate scroll snap state during resize/rotations

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Brent Fulgham]

This bug is an iOS flavor of Bug 142590, where resize events were not properly updating scroll snap state. In this case, the scroll snap positioning is not correct after a screen rotation.

We need to update the RemoteLayerTree code to notify the WebProcess of new scroll snap positions so that after rotation we can be positioned on the right snap point.

[Radar WebKit Bug Importer]

<rdar://problem/20975978>

[Brent Fulgham]

Created attachment 253245 [details]
Patch

[WebKit Commit Bot]

Attachment 253245 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/ios/RemoteScrollingCoordinatorProxyIOS.mm:176:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

Comment on attachment 253245 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=253245&action=review

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:1407
> +        CGRect unobscuredRect = UIEdgeInsetsInsetRect(fullViewRect, [self _computedContentInset]);

Would be nice to do just enough work to compute origin.y, rather than everything.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:1539
> +            CGPoint currentPoint = [_scrollView contentOffset];
> +            CGPoint activePoint = coordinator->nearestActiveSnapPoint(unobscuredRect.origin.y, currentPoint);
> +
> +            if (!CGPointEqualToPoint(activePoint, currentPoint)) {

Does this work with zooming?

> Source/WebKit2/UIProcess/Scrolling/RemoteScrollingCoordinatorProxy.h:90
> +    CGPoint nearestActiveSnapPoint(CGFloat topInset, const CGPoint&) const;

I think it would be good to have "contentOffset" in the name here, maybe contentOffsetOfearestActiveSnapPoint().

> Source/WebKit2/UIProcess/ios/RemoteScrollingCoordinatorProxyIOS.mm:126
> +        float potentialSnapPosition = closestSnapOffsetForMainFrameScrolling(WebCore::ScrollEventAxis::Vertical, targetContentOffset->y, velocity.y, m_currentVerticalSnapPointIndex);
> +        potentialSnapPosition -= topInset;

Are these in the same coordinate space with zooming?

> Source/WebKit2/UIProcess/ios/RemoteScrollingCoordinatorProxyIOS.mm:166
> +    if (!root->isFrameScrollingNode())

is<>

> Source/WebKit2/UIProcess/ios/RemoteScrollingCoordinatorProxyIOS.mm:169
> +    ScrollingTreeFrameScrollingNode* rootFrame = static_cast<ScrollingTreeFrameScrollingNode*>(root);

downcast<>

[Brent Fulgham]

Comment on attachment 253245 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=253245&action=review

>> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:1539
>> +            if (!CGPointEqualToPoint(activePoint, currentPoint)) {
> 
> Does this work with zooming?

Based on my testing, it does. But we (I) should be careful to make sure this is tested thoroughly.

>> Source/WebKit2/UIProcess/Scrolling/RemoteScrollingCoordinatorProxy.h:90
>> +    CGPoint nearestActiveSnapPoint(CGFloat topInset, const CGPoint&) const;
> 
> I think it would be good to have "contentOffset" in the name here, maybe contentOffsetOfearestActiveSnapPoint().

I changed it to 'nearestActiveContentInsetAdjustedSnapPoint', which is a bit of a mouthful, but is more descriptive of what it does.

>> Source/WebKit2/UIProcess/ios/RemoteScrollingCoordinatorProxyIOS.mm:126
>> +        potentialSnapPosition -= topInset;
> 
> Are these in the same coordinate space with zooming?

Yes. The coordinates are both in active view space (both zoomed, or both not-zoomed).

>> Source/WebKit2/UIProcess/ios/RemoteScrollingCoordinatorProxyIOS.mm:166
>> +    if (!root->isFrameScrollingNode())
> 
> is<>

Done!

>> Source/WebKit2/UIProcess/ios/RemoteScrollingCoordinatorProxyIOS.mm:169
>> +    ScrollingTreeFrameScrollingNode* rootFrame = static_cast<ScrollingTreeFrameScrollingNode*>(root);
> 
> downcast<>

Done!

[Brent Fulgham]

Committed r184439: <http://trac.webkit.org/changeset/184439>

[Brent Fulgham]

*** Bug 136005 has been marked as a duplicate of this bug. ***