[iOS] Hit-testing on icloud.com is offset after closing a tab
authorwenson_hsieh@apple.com <wenson_hsieh@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 8 Jun 2020 16:47:06 +0000 (16:47 +0000)
committerwenson_hsieh@apple.com <wenson_hsieh@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 8 Jun 2020 16:47:06 +0000 (16:47 +0000)
https://bugs.webkit.org/show_bug.cgi?id=212890
<rdar://problem/58775297>

Reviewed by Simon Fraser.

Source/WebKit:

After tapping to create a new tab on the icloud.com settings page (with no other tabs open) and then closing the
new tab, all hit-testing on the page is offset by -33px until the page is reloaded. This bug appears to be a
corner case triggered by a combination of the following three changes:

- <https://trac.webkit.org/r170463>
- <https://trac.webkit.org/r245006>
- <rdar://problem/53660458> (which is an iOS-specific Safari change)

We start on icloud.com's settings page. When closing the newly created tab, Safari hides the tab bar, causing
both the top content inset and top obscured inset to decrease by 33 points. When applying the top content inset
change, Safari (after <rdar://problem/53660458>) temporarily sets `_automaticContentOffsetAdjustmentEnabled` on
`WKScrollView` to `NO`, which means that the scroll view doesn't automatically adjust its content offset to -70;
instead, it remains at -103 (the content offset when the tab bar is shown).

Because of this, during the next remote layer tree commit where the content size changes,
`-_setContentSizePreservingContentOffsetDuringRubberband:` will believe that we're currently rubber-banding by
33px, and therefore try to adjust the content offset of the scroll view to be -103 instead of allowing it to be
adjusted back to -70 (see r170463 and `-_restoreContentOffsetWithRubberbandAmount:`).

This results in the scroll position on the page (`document.scrollingElement.scrollTop`) reporting -33, even
though the top of the page is flush with the bottom of Safari's browser chrome. Finally, because `WKScrollView`
is made unscrollable due to `overflow: hidden;` (r245006), we end up permanently stuck in this state until the
next page load, rather than the next remote layer tree commit that causes any scrolling.

To fix this, add an additional restriction on the rubberbanding offset restoration code in
`-_setContentSizePreservingContentOffsetDuringRubberband:`, such that we only try to preserve the current
rubberbanded offset if we're actually rubberbanding (that is, dragging or bouncing against the edge of the
scroll view). Note that when rubberbanding against the top of the scroll view on iPad using a trackpad, the
`-isDragging` property is also true, which matches behavior when panning via touch.

Test: ScrollViewInsetTests.ChangeInsetWithoutAutomaticAdjustmentWhileWebProcessIsUnresponsive

* Platform/spi/ios/UIKitSPI.h:
* UIProcess/ios/WKScrollView.mm:
(-[WKScrollView _setContentSizePreservingContentOffsetDuringRubberband:]):

Also, split out the `CGSizeEqualToSize(currentContentSize, contentSize)` case into a separate early return that
skips the call to `-setContentSize:` altogether.

Tools:

Add a new API test to verify that the scroll position doesn't get stuck at -30px after shifting both the top
content inset and top obscured inset by 30px. See WebKit ChangeLog for more details.

* TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:
* TestWebKitAPI/Tests/WebKitCocoa/overflow-hidden.html: Added.
* TestWebKitAPI/Tests/ios/ScrollViewInsetTests.mm:
(-[ScrollViewDelegate scrollViewDidScroll:]):
(TestWebKitAPI::TEST):
* TestWebKitAPI/ios/UIKitSPI.h:

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@262721 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebKit/ChangeLog
Source/WebKit/Platform/spi/ios/UIKitSPI.h
Source/WebKit/UIProcess/ios/WKScrollView.mm
Tools/ChangeLog
Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj
Tools/TestWebKitAPI/Tests/WebKitCocoa/overflow-hidden.html [new file with mode: 0644]
Tools/TestWebKitAPI/Tests/ios/ScrollViewInsetTests.mm
Tools/TestWebKitAPI/ios/UIKitSPI.h

index 7040312..eb7e865 100644 (file)
@@ -1,3 +1,50 @@
+2020-06-08  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        [iOS] Hit-testing on icloud.com is offset after closing a tab
+        https://bugs.webkit.org/show_bug.cgi?id=212890
+        <rdar://problem/58775297>
+
+        Reviewed by Simon Fraser.
+
+        After tapping to create a new tab on the icloud.com settings page (with no other tabs open) and then closing the
+        new tab, all hit-testing on the page is offset by -33px until the page is reloaded. This bug appears to be a
+        corner case triggered by a combination of the following three changes:
+
+        - <https://trac.webkit.org/r170463>
+        - <https://trac.webkit.org/r245006>
+        - <rdar://problem/53660458> (which is an iOS-specific Safari change)
+
+        We start on icloud.com's settings page. When closing the newly created tab, Safari hides the tab bar, causing
+        both the top content inset and top obscured inset to decrease by 33 points. When applying the top content inset
+        change, Safari (after <rdar://problem/53660458>) temporarily sets `_automaticContentOffsetAdjustmentEnabled` on
+        `WKScrollView` to `NO`, which means that the scroll view doesn't automatically adjust its content offset to -70;
+        instead, it remains at -103 (the content offset when the tab bar is shown).
+
+        Because of this, during the next remote layer tree commit where the content size changes,
+        `-_setContentSizePreservingContentOffsetDuringRubberband:` will believe that we're currently rubber-banding by
+        33px, and therefore try to adjust the content offset of the scroll view to be -103 instead of allowing it to be
+        adjusted back to -70 (see r170463 and `-_restoreContentOffsetWithRubberbandAmount:`).
+
+        This results in the scroll position on the page (`document.scrollingElement.scrollTop`) reporting -33, even
+        though the top of the page is flush with the bottom of Safari's browser chrome. Finally, because `WKScrollView`
+        is made unscrollable due to `overflow: hidden;` (r245006), we end up permanently stuck in this state until the
+        next page load, rather than the next remote layer tree commit that causes any scrolling.
+
+        To fix this, add an additional restriction on the rubberbanding offset restoration code in
+        `-_setContentSizePreservingContentOffsetDuringRubberband:`, such that we only try to preserve the current
+        rubberbanded offset if we're actually rubberbanding (that is, dragging or bouncing against the edge of the
+        scroll view). Note that when rubberbanding against the top of the scroll view on iPad using a trackpad, the
+        `-isDragging` property is also true, which matches behavior when panning via touch.
+
+        Test: ScrollViewInsetTests.ChangeInsetWithoutAutomaticAdjustmentWhileWebProcessIsUnresponsive
+
+        * Platform/spi/ios/UIKitSPI.h:
+        * UIProcess/ios/WKScrollView.mm:
+        (-[WKScrollView _setContentSizePreservingContentOffsetDuringRubberband:]):
+
+        Also, split out the `CGSizeEqualToSize(currentContentSize, contentSize)` case into a separate early return that
+        skips the call to `-setContentSize:` altogether.
+
 2020-06-08  Andy Estes  <aestes@apple.com>
 
         [Apple Pay] Remove -respondsToSelector: check before calling -[PKPaymentRequest setBoundInterfaceIdentifier:]
index abee255..ee5ac19 100644 (file)
@@ -400,6 +400,8 @@ typedef enum {
 - (double)_verticalVelocity;
 - (void)_flashScrollIndicatorsForAxes:(UIAxis)axes persistingPreviousFlashes:(BOOL)persisting;
 @property (nonatomic, getter=isZoomEnabled) BOOL zoomEnabled;
+@property (nonatomic, readonly, getter=_isVerticalBouncing) BOOL isVerticalBouncing;
+@property (nonatomic, readonly, getter=_isHorizontalBouncing) BOOL isHorizontalBouncing;
 @property (nonatomic, readonly, getter=_isAnimatingZoom) BOOL isAnimatingZoom;
 @property (nonatomic, readonly, getter=_isAnimatingScroll) BOOL isAnimatingScroll;
 @property (nonatomic) CGFloat horizontalScrollDecelerationFactor;
index 1d30be4..9a0ddb5 100644 (file)
@@ -344,7 +344,11 @@ static inline bool valuesAreWithinOnePixel(CGFloat a, CGFloat b)
 {
     CGSize currentContentSize = [self contentSize];
 
-    if (CGSizeEqualToSize(currentContentSize, CGSizeZero) || CGSizeEqualToSize(currentContentSize, contentSize) || self.zoomScale < self.minimumZoomScale) {
+    if (CGSizeEqualToSize(currentContentSize, contentSize))
+        return;
+
+    BOOL mightBeRubberbanding = self.isDragging || self.isVerticalBouncing || self.isHorizontalBouncing;
+    if (!mightBeRubberbanding || CGSizeEqualToSize(currentContentSize, CGSizeZero) || self.zoomScale < self.minimumZoomScale) {
         [self setContentSize:contentSize];
         return;
     }
index a29ebb0..f202dc3 100644 (file)
@@ -1,3 +1,21 @@
+2020-06-08  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        [iOS] Hit-testing on icloud.com is offset after closing a tab
+        https://bugs.webkit.org/show_bug.cgi?id=212890
+        <rdar://problem/58775297>
+
+        Reviewed by Simon Fraser.
+
+        Add a new API test to verify that the scroll position doesn't get stuck at -30px after shifting both the top
+        content inset and top obscured inset by 30px. See WebKit ChangeLog for more details.
+
+        * TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:
+        * TestWebKitAPI/Tests/WebKitCocoa/overflow-hidden.html: Added.
+        * TestWebKitAPI/Tests/ios/ScrollViewInsetTests.mm:
+        (-[ScrollViewDelegate scrollViewDidScroll:]):
+        (TestWebKitAPI::TEST):
+        * TestWebKitAPI/ios/UIKitSPI.h:
+
 2020-06-08  Philippe Normand  <pnormand@igalia.com>
 
         [Flatpak SDK] Add flatpak-run-nightly
index 260a50d..abfb840 100644 (file)
                F4856CA31E649EA8009D7EE7 /* attachment-element.html in Copy Resources */ = {isa = PBXBuildFile; fileRef = F4856CA21E6498A8009D7EE7 /* attachment-element.html */; };
                F486B1D01F67952300F34BDD /* DataTransfer-setDragImage.html in Copy Resources */ = {isa = PBXBuildFile; fileRef = F486B1CF1F6794FF00F34BDD /* DataTransfer-setDragImage.html */; };
                F48D6C10224B377000E3E2FB /* PreferredContentMode.mm in Sources */ = {isa = PBXBuildFile; fileRef = F48D6C0F224B377000E3E2FB /* PreferredContentMode.mm */; };
+               F49992C6248DABFD00034167 /* overflow-hidden.html in Copy Resources */ = {isa = PBXBuildFile; fileRef = F49992C5248DABE400034167 /* overflow-hidden.html */; };
                F4A32EC41F05F3850047C544 /* dragstart-change-selection-offscreen.html in Copy Resources */ = {isa = PBXBuildFile; fileRef = F4A32EC31F05F3780047C544 /* dragstart-change-selection-offscreen.html */; };
                F4A32ECB1F0643370047C544 /* contenteditable-in-iframe.html in Copy Resources */ = {isa = PBXBuildFile; fileRef = F4A32ECA1F0642F40047C544 /* contenteditable-in-iframe.html */; };
                F4A9202F1FEE34E900F59590 /* apple-data-url.html in Copy Resources */ = {isa = PBXBuildFile; fileRef = F4A9202E1FEE34C800F59590 /* apple-data-url.html */; };
                                41848F4424891879000E2588 /* open-window-with-file-url-with-host.html in Copy Resources */,
                                931C281E22BC579A001D98C4 /* opendatabase-always-exists.html in Copy Resources */,
                                290A9BB91735F63800D71BBC /* OpenNewWindow.html in Copy Resources */,
+                               F49992C6248DABFD00034167 /* overflow-hidden.html in Copy Resources */,
                                0F340779230382870060A1A0 /* overflow-scroll.html in Copy Resources */,
                                83148B09202AC78D00BADE99 /* override-builtins-test.html in Copy Resources */,
                                CEBCA1391E3A807A00C73293 /* page-with-csp-iframe.html in Copy Resources */,
                F486B1CF1F6794FF00F34BDD /* DataTransfer-setDragImage.html */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text.html; path = "DataTransfer-setDragImage.html"; sourceTree = "<group>"; };
                F48D6C0F224B377000E3E2FB /* PreferredContentMode.mm */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.objcpp; path = PreferredContentMode.mm; sourceTree = "<group>"; };
                F493247C1F44DF8D006F4336 /* UIKitSPI.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = UIKitSPI.h; sourceTree = "<group>"; };
+               F49992C5248DABE400034167 /* overflow-hidden.html */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text.html; path = "overflow-hidden.html"; sourceTree = "<group>"; };
                F4A32EC31F05F3780047C544 /* dragstart-change-selection-offscreen.html */ = {isa = PBXFileReference; lastKnownFileType = text.html; path = "dragstart-change-selection-offscreen.html"; sourceTree = "<group>"; };
                F4A32ECA1F0642F40047C544 /* contenteditable-in-iframe.html */ = {isa = PBXFileReference; lastKnownFileType = text.html; path = "contenteditable-in-iframe.html"; sourceTree = "<group>"; };
                F4A9202E1FEE34C800F59590 /* apple-data-url.html */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text.html; path = "apple-data-url.html"; sourceTree = "<group>"; };
                                93E2D2751ED7D51700FA76F6 /* offscreen-iframe-of-media-document.html */,
                                7CCB99221D3B44E7003922F6 /* open-multiple-external-url.html */,
                                931C281B22BC5583001D98C4 /* opendatabase-always-exists.html */,
+                               F49992C5248DABE400034167 /* overflow-hidden.html */,
                                CEBCA1341E3A803400C73293 /* page-with-csp-iframe.html */,
                                CEBCA1351E3A803400C73293 /* page-with-csp.html */,
                                CEBCA1361E3A803400C73293 /* page-without-csp-iframe.html */,
diff --git a/Tools/TestWebKitAPI/Tests/WebKitCocoa/overflow-hidden.html b/Tools/TestWebKitAPI/Tests/WebKitCocoa/overflow-hidden.html
new file mode 100644 (file)
index 0000000..afebc30
--- /dev/null
@@ -0,0 +1,26 @@
+<!DOCTYPE html>
+<html>
+<head>
+    <meta name="viewport" content="width=device-width, initial-scale=1">
+    <script>
+    function hangFor(ms) {
+        const startTime = Date.now();
+        while (1) {
+            if (Date.now() - startTime > ms)
+                break;
+        }
+    }
+
+    function setBodyHeight(height) {
+        document.body.style.height = `${height}px`;
+    }
+    </script>
+    <style>
+        html, body {
+            overflow: hidden;
+        }
+    </style>
+</head>
+<body>
+</body>
+</html>
index f164278..7bbde5f 100644 (file)
 #import "TestWKWebView.h"
 #import "UIKitSPI.h"
 #import <WebKit/WKWebViewPrivate.h>
+#import <wtf/Vector.h>
+
+@interface ScrollViewDelegate : NSObject<UIScrollViewDelegate> {
+    @public Vector<CGPoint> _contentOffsetHistory;
+}
+@end
+
+@implementation ScrollViewDelegate
+
+- (void)scrollViewDidScroll:(UIScrollView *)scrollView
+{
+    auto contentOffset = scrollView.contentOffset;
+    if (_contentOffsetHistory.isEmpty() || !CGPointEqualToPoint(_contentOffsetHistory.last(), contentOffset))
+        _contentOffsetHistory.append(contentOffset);
+}
+
+@end
 
 @interface AsyncPolicyDelegateForInsetTest : NSObject<WKNavigationDelegate> {
     @public bool _navigationComplete;
@@ -206,6 +223,42 @@ TEST(ScrollViewInsetTests, RestoreInitialContentOffsetAfterCrashWithAsyncPolicyD
     EXPECT_EQ(-400, initialContentOffset.y);
 }
 
+TEST(ScrollViewInsetTests, ChangeInsetWithoutAutomaticAdjustmentWhileWebProcessIsUnresponsive)
+{
+    constexpr CGFloat initialTopInset = 100;
+    constexpr CGFloat updatedTopInset = 70;
+
+    auto scrollViewDelegate = adoptNS([[ScrollViewDelegate alloc] init]);
+    auto webView = adoptNS([[TestWKWebView alloc] initWithFrame:CGRectMake(0, 0, 800, 600)]);
+    [webView scrollView].scrollEnabled = NO;
+    [webView scrollView].delegate = scrollViewDelegate.get();
+    [webView _setObscuredInsets:UIEdgeInsetsMake(initialTopInset, 0, 0, 0)];
+    [webView scrollView].contentInset = UIEdgeInsetsMake(initialTopInset, 0, 0, 0);
+    [webView synchronouslyLoadTestPageNamed:@"overflow-hidden"];
+
+    UIWindow *window = [webView window];
+    [webView evaluateJavaScript:@"setBodyHeight(2000); hangFor(500)" completionHandler:nil];
+    [webView removeFromSuperview];
+
+    __block bool done = false;
+    dispatch_async(dispatch_get_main_queue(), ^{
+        [window addSubview:webView.get()];
+        [webView _setObscuredInsets:UIEdgeInsetsMake(updatedTopInset, 0, 0, 0)];
+        [[webView scrollView] _setAutomaticContentOffsetAdjustmentEnabled:NO];
+        [webView scrollView].contentInset = UIEdgeInsetsMake(updatedTopInset, 0, 0, 0);
+        [[webView scrollView] _setAutomaticContentOffsetAdjustmentEnabled:YES];
+        done = true;
+    });
+
+    TestWebKitAPI::Util::run(&done);
+    [webView waitForNextPresentationUpdate];
+
+    EXPECT_EQ(2U, scrollViewDelegate->_contentOffsetHistory.size());
+    EXPECT_EQ(-100, scrollViewDelegate->_contentOffsetHistory.first().y);
+    EXPECT_EQ(-70, scrollViewDelegate->_contentOffsetHistory.last().y);
+    EXPECT_EQ(0, [[webView stringByEvaluatingJavaScript:@"document.scrollingElement.scrollTop"] intValue]);
+}
+
 } // namespace TestWebKitAPI
 
 #endif // PLATFORM(IOS_FAMILY)
index 42282fe..ad70dd5 100644 (file)
@@ -36,6 +36,7 @@
 #import <UIKit/UIKeyboard_Private.h>
 #import <UIKit/UIResponder_Private.h>
 #import <UIKit/UIScreen_Private.h>
+#import <UIKit/UIScrollView_Private.h>
 #import <UIKit/UITextAutofillSuggestion.h>
 #import <UIKit/UITextInputMultiDocument.h>
 #import <UIKit/UITextInputTraits_Private.h>
@@ -215,6 +216,10 @@ IGNORE_WARNINGS_END
 - (void)setSuggestions:(NSArray <UITextSuggestion*> *)suggestions;
 @end
 
+@interface UIScrollView (SPI)
+@property (nonatomic, getter=_isAutomaticContentOffsetAdjustmentEnabled, setter=_setAutomaticContentOffsetAdjustmentEnabled:) BOOL isAutomaticContentOffsetAdjustmentEnabled;
+@end
+
 #endif // USE(APPLE_INTERNAL_SDK)
 
 #define UIWKDocumentRequestMarkedTextRects (1 << 5)