Changeset 238728 in webkit

Timestamp:

Nov 29, 2018 11:22:00 PM (5 years ago)

Author:

Wenson Hsieh

Message:

REGRESSION (r238635): Dragging a text selection within WKWebView causes the selection highlight to get into a bad state
​https://bugs.webkit.org/show_bug.cgi?id=192165
<rdar://problem/46346682>

Reviewed by Daniel Bates.

Source/WebKit:

Fixes a bug in PageClientImpl::isViewFocused. Consider the following scenario:

1. WKWebView is hosted within the view hierarchy
2. First responder is *not* WKContentView
3. The active focus retain count is nonzero

Before r238635, we would return true, due to condition (3). However, after r238635, we only consider whether the
first responder is WKContentView, since the web view is in the view hierarchy. This breaks scenarios where
WebKit or UIKit attempts to retain focus and later restore the content view to be the first responder (an
example of this is dragging a text selection between editable elements in the same web view).

To fix this, simply bail early and return true if focus is being retained.

• UIProcess/ios/PageClientImplIOS.mm:

(WebKit::PageClientImpl::isViewFocused):

Tools:

Fixes 11 API tests that started failing or timing out after r238635. See below for more details.

• TestWebKitAPI/Tests/WebKitCocoa/WKWebViewEditActions.mm:

(TestWebKitAPI::webViewForEditActionTesting):
(TestWebKitAPI::webViewForEditActionTestingWithPageNamed):

Ensure that the web view becomes first responder before executing edit actions.

• TestWebKitAPI/Tests/WebKitCocoa/autofocus-contenteditable.html:
• TestWebKitAPI/Tests/WebKitCocoa/contenteditable-and-textarea.html:

Tweak these tests to allow selected content to overflow the width of the web view. Without this change,
ContentEditableToContentEditable and ContentEditableToTextarea will sometimes fail because the content causes
the body to scroll horizontally, so we miss the drop destination.

• TestWebKitAPI/Tests/ios/DragAndDropTestsIOS.mm:

(loadTestPageAndEnsureInputSession):

Add a new helper to load a test page with a given name, become first responder, and wait until an input session
starts. Use this in various drag and drop tests to reduce code duplication.

• TestWebKitAPI/cocoa/DragAndDropSimulator.h:
• TestWebKitAPI/ios/DragAndDropSimulatorIOS.mm:

(-[DragAndDropSimulator initWithWebView:]):
(-[DragAndDropSimulator _resetSimulatedState]):
(-[DragAndDropSimulator _concludeDropAndPerformOperationIfNecessary]):
(-[DragAndDropSimulator _advanceProgress]):

To more accurately emulate UIKit behavior, begin focus preservation when starting a drag, and attempt to clear
the focus preservation token when the drag session ends. This allows us to simulate and test the scenario that
regressed with r238635.

(-[DragAndDropSimulator ensureInputSession]):
(-[DragAndDropSimulator _webView:didStartInputSession:]):
(-[DragAndDropSimulator waitForInputSession]): Deleted.

Refactored into -ensureInputSession. Instead of assuming that an input session has not yet been started, simply
wait for an input session to start if needed.

• TestWebKitAPI/ios/UIKitSPI.h:

Add a new SPI declaration.

Location:

trunk

Files:

• Source/WebKit/ChangeLog (modified) (1 diff)
• Source/WebKit/UIProcess/ios/PageClientImplIOS.mm (modified) (1 diff)
• Tools/ChangeLog (modified) (1 diff)
• Tools/TestWebKitAPI/Tests/WebKitCocoa/WKWebViewEditActions.mm (modified) (3 diffs)
• Tools/TestWebKitAPI/Tests/WebKitCocoa/autofocus-contenteditable.html (modified) (1 diff)
• Tools/TestWebKitAPI/Tests/WebKitCocoa/contenteditable-and-textarea.html (modified) (1 diff)
• Tools/TestWebKitAPI/Tests/ios/DragAndDropTestsIOS.mm (modified) (7 diffs)
• Tools/TestWebKitAPI/cocoa/DragAndDropSimulator.h (modified) (1 diff)
• Tools/TestWebKitAPI/ios/DragAndDropSimulatorIOS.mm (modified) (7 diffs)
• Tools/TestWebKitAPI/ios/UIKitSPI.h (modified) (1 diff)

trunk/Source/WebKit/ChangeLog

@@ +1 @@
+2018-11-29  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        REGRESSION (r238635): Dragging a text selection within WKWebView causes the selection highlight to get into a bad state
+        https://bugs.webkit.org/show_bug.cgi?id=192165
+        <rdar://problem/46346682>
+
+        Reviewed by Daniel Bates.
+
+        Fixes a bug in PageClientImpl::isViewFocused. Consider the following scenario:
+        1. WKWebView is hosted within the view hierarchy
+        2. First responder is *not* WKContentView
+        3. The active focus retain count is nonzero
+
+        Before r238635, we would return true, due to condition (3). However, after r238635, we only consider whether the
+        first responder is WKContentView, since the web view is in the view hierarchy. This breaks scenarios where
+        WebKit or UIKit attempts to retain focus and later restore the content view to be the first responder (an
+        example of this is dragging a text selection between editable elements in the same web view).
+
+        To fix this, simply bail early and return true if focus is being retained.
+
+        * UIProcess/ios/PageClientImplIOS.mm:
+        (WebKit::PageClientImpl::isViewFocused):
+
 2018-11-29  Tim Horton  <timothy_horton@apple.com>
 

trunk/Source/WebKit/UIProcess/ios/PageClientImplIOS.mm

@@ -167 +167 @@
 bool PageClientImpl::isViewFocused()
 {
-    if (isViewInWindow() && ![m_webView _isBackground])
-        return [m_webView _contentViewIsFirstResponder];
-    return [m_webView _isRetainingActiveFocusedState];
+    return (isViewInWindow() && ![m_webView _isBackground] && [m_webView _contentViewIsFirstResponder]) || [m_webView _isRetainingActiveFocusedState];
 }
 

trunk/Tools/ChangeLog

@@ +1 @@
+2018-11-29  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        REGRESSION (r238635): Dragging a text selection within WKWebView causes the selection highlight to get into a bad state
+        https://bugs.webkit.org/show_bug.cgi?id=192165
+        <rdar://problem/46346682>
+
+        Reviewed by Daniel Bates.
+
+        Fixes 11 API tests that started failing or timing out after r238635. See below for more details.
+
+        * TestWebKitAPI/Tests/WebKitCocoa/WKWebViewEditActions.mm:
+        (TestWebKitAPI::webViewForEditActionTesting):
+        (TestWebKitAPI::webViewForEditActionTestingWithPageNamed):
+
+        Ensure that the web view becomes first responder before executing edit actions.
+
+        * TestWebKitAPI/Tests/WebKitCocoa/autofocus-contenteditable.html:
+        * TestWebKitAPI/Tests/WebKitCocoa/contenteditable-and-textarea.html:
+
+        Tweak these tests to allow selected content to overflow the width of the web view. Without this change,
+        ContentEditableToContentEditable and ContentEditableToTextarea will sometimes fail because the content causes
+        the body to scroll horizontally, so we miss the drop destination.
+
+        * TestWebKitAPI/Tests/ios/DragAndDropTestsIOS.mm:
+        (loadTestPageAndEnsureInputSession):
+
+        Add a new helper to load a test page with a given name, become first responder, and wait until an input session
+        starts. Use this in various drag and drop tests to reduce code duplication.
+
+        * TestWebKitAPI/cocoa/DragAndDropSimulator.h:
+        * TestWebKitAPI/ios/DragAndDropSimulatorIOS.mm:
+        (-[DragAndDropSimulator initWithWebView:]):
+        (-[DragAndDropSimulator _resetSimulatedState]):
+        (-[DragAndDropSimulator _concludeDropAndPerformOperationIfNecessary]):
+        (-[DragAndDropSimulator _advanceProgress]):
+
+        To more accurately emulate UIKit behavior, begin focus preservation when starting a drag, and attempt to clear
+        the focus preservation token when the drag session ends. This allows us to simulate and test the scenario that
+        regressed with r238635.
+
+        (-[DragAndDropSimulator ensureInputSession]):
+        (-[DragAndDropSimulator _webView:didStartInputSession:]):
+        (-[DragAndDropSimulator waitForInputSession]): Deleted.
+
+        Refactored into -ensureInputSession. Instead of assuming that an input session has not yet been started, simply
+        wait for an input session to start if needed.
+
+        * TestWebKitAPI/ios/UIKitSPI.h:
+
+        Add a new SPI declaration.
+
 2018-11-29  Tim Horton  <timothy_horton@apple.com>
 

trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/WKWebViewEditActions.mm

@@ -79 +79 @@
     [webView synchronouslyLoadHTMLString:markup];
     [webView _setEditable:YES];
+    [webView becomeFirstResponder];
     [webView stringByEvaluatingJavaScript:@"getSelection().setPosition(document.body, 1)"];
     return webView;
@@ -88 +89 @@
     [webView synchronouslyLoadTestPageNamed:testPageName];
     [webView _setEditable:YES];
+    [webView becomeFirstResponder];
     [webView stringByEvaluatingJavaScript:@"getSelection().setPosition(document.body, 1)"];
     return webView;
@@ -244 +246 @@
 {
     auto source = webViewForEditActionTesting();
-    auto destination = webViewForEditActionTesting(@"<div><br></div>");
-
     [source selectAll:nil];
     [source evaluateJavaScript:@"document.execCommand('bold'); document.execCommand('underline'); document.execCommand('italic')" completionHandler:nil];
     [source _synchronouslyExecuteEditCommand:@"Copy" argument:nil];
 
+    auto destination = webViewForEditActionTesting(@"<div><br></div>");
     [destination _pasteAndMatchStyle:nil];
     [destination selectAll:nil];

trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/autofocus-contenteditable.html

@@ -13 +13 @@
             font-size: 200px;
             white-space: nowrap;
+        }
+
+        #source {
+            overflow: hidden;
         }
 

trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/contenteditable-and-textarea.html

@@ -13 +13 @@
             font-size: 200px;
             white-space: nowrap;
+        }
+
+        #source {
+            overflow: hidden;
         }
 

trunk/Tools/TestWebKitAPI/Tests/ios/DragAndDropTestsIOS.mm

@@ -99 +99 @@
 @end
 
+static void loadTestPageAndEnsureInputSession(DragAndDropSimulator *simulator, NSString *testPageName)
+{
+    TestWKWebView *webView = [simulator webView];
+    simulator.allowsFocusToStartInputSession = YES;
+    [webView becomeFirstResponder];
+    [webView synchronouslyLoadTestPageNamed:testPageName];
+    [simulator ensureInputSession];
+}
+
 static NSValue *makeCGRectValue(CGFloat x, CGFloat y, CGFloat width, CGFloat height)
 {
@@ -342 +351 @@
     auto simulator = adoptNS([[DragAndDropSimulator alloc] initWithWebView:webView.get()]);
 
-    [webView loadTestPageNamed:@"autofocus-contenteditable"];
-    [simulator waitForInputSession];
+    loadTestPageAndEnsureInputSession(simulator.get(), @"autofocus-contenteditable");
     [simulator runFrom:CGPointMake(100, 50) to:CGPointMake(100, 300)];
 
@@ -363 +371 @@
     auto simulator = adoptNS([[DragAndDropSimulator alloc] initWithWebView:webView.get()]);
 
-    [webView loadTestPageNamed:@"contenteditable-and-textarea"];
-    [simulator waitForInputSession];
+    loadTestPageAndEnsureInputSession(simulator.get(), @"contenteditable-and-textarea");
     [simulator runFrom:CGPointMake(100, 50) to:CGPointMake(100, 300)];
 
@@ -395 +402 @@
     auto simulator = adoptNS([[DragAndDropSimulator alloc] initWithWebView:webView.get()]);
 
-    [webView loadTestPageNamed:@"two-paragraph-contenteditable"];
-    [simulator waitForInputSession];
+    loadTestPageAndEnsureInputSession(simulator.get(), @"two-paragraph-contenteditable");
     [simulator runFrom:CGPointMake(100, 50) to:CGPointMake(250, 450)];
 
@@ -426 +432 @@
     auto simulator = adoptNS([[DragAndDropSimulator alloc] initWithWebView:webView.get()]);
 
-    [webView loadTestPageNamed:@"textarea-to-input"];
-    [simulator waitForInputSession];
+    loadTestPageAndEnsureInputSession(simulator.get(), @"textarea-to-input");
     [simulator runFrom:CGPointMake(100, 50) to:CGPointMake(100, 300)];
 
@@ -441 +446 @@
     auto simulator = adoptNS([[DragAndDropSimulator alloc] initWithWebView:webView.get()]);
 
-    [webView loadTestPageNamed:@"textarea-to-input"];
-    [simulator waitForInputSession];
+    loadTestPageAndEnsureInputSession(simulator.get(), @"textarea-to-input");
     [webView stringByEvaluatingJavaScript:@"source.value = 'pneumonoultramicroscopicsilicovolcanoconiosis'"];
     [webView stringByEvaluatingJavaScript:@"source.selectionStart = 0"];
@@ -464 +468 @@
     auto simulator = adoptNS([[DragAndDropSimulator alloc] initWithWebView:webView.get()]);
 
-    [webView loadTestPageNamed:@"textarea-to-input"];
-    [simulator waitForInputSession];
+    loadTestPageAndEnsureInputSession(simulator.get(), @"textarea-to-input");
     [webView stringByEvaluatingJavaScript:@"source.value = 'https://webkit.org/'"];
     [webView stringByEvaluatingJavaScript:@"source.selectionStart = 0"];

trunk/Tools/TestWebKitAPI/cocoa/DragAndDropSimulator.h

@@ -90 +90 @@
 - (instancetype)initWithWebView:(TestWKWebView *)webView;
 - (void)runFrom:(CGPoint)startLocation to:(CGPoint)endLocation additionalItemRequestLocations:(ProgressToCGPointValueMap)additionalItemRequestLocations;
-- (void)waitForInputSession;
+- (void)ensureInputSession;
 
 @property (nonatomic, readonly) DragAndDropPhase phase;

trunk/Tools/TestWebKitAPI/ios/DragAndDropSimulatorIOS.mm

@@ -311 +311 @@
     RetainPtr<NSMutableArray<_WKAttachment *>> _removedAttachments;
 
-    bool _isDoneWaitingForInputSession;
+    bool _hasStartedInputSession;
     double _currentProgress;
     bool _isDoneWithCurrentRun;
@@ -339 +339 @@
         _shouldEnsureUIApplication = NO;
         _shouldAllowMoveOperation = YES;
-        _isDoneWaitingForInputSession = true;
         [_webView setUIDelegate:self];
         [_webView _setInputDelegate:self];
@@ -374 +373 @@
     _queuedAdditionalItemRequestLocations = adoptNS([[NSMutableArray alloc] init]);
     _liftPreviews = adoptNS([[NSMutableArray alloc] init]);
+    _hasStartedInputSession = false;
 }
 
@@ -460 +460 @@
     [[_webView dropInteractionDelegate] dropInteraction:[_webView dropInteraction] sessionDidEnd:_dropSession.get()];
 
-    if (_dragSession)
-        [[_webView dragInteractionDelegate] dragInteraction:[_webView dragInteraction] session:_dragSession.get() didEndWithOperation:operation];
+    if (_dragSession) {
+        auto delegate = [_webView dragInteractionDelegate];
+        [delegate dragInteraction:[_webView dragInteraction] session:_dragSession.get() didEndWithOperation:operation];
+        if ([delegate respondsToSelector:@selector(_clearToken:)])
+            [(id <UITextInputMultiDocument>)delegate _clearToken:nil];
+        [_webView becomeFirstResponder];
+    }
 }
 
@@ -544 +549 @@
         }
 
-        [[_webView dragInteractionDelegate] dragInteraction:[_webView dragInteraction] sessionWillBegin:_dragSession.get()];
+        auto delegate = [_webView dragInteractionDelegate];
+        if ([delegate respondsToSelector:@selector(_preserveFocusWithToken:destructively:)])
+            [(id <UITextInputMultiDocument>)delegate _preserveFocusWithToken:nil destructively:NO];
+
+        [_webView resignFirstResponder];
+
+        [delegate dragInteraction:[_webView dragInteraction] sessionWillBegin:_dragSession.get()];
 
         RetainPtr<WKWebView> retainedWebView = _webView;
@@ -619 +630 @@
 }
 
-- (void)waitForInputSession
-{
-    _isDoneWaitingForInputSession = false;
-
-    // Waiting for an input session implies that we should allow input sessions to begin.
-    self.allowsFocusToStartInputSession = YES;
-
-    Util::run(&_isDoneWaitingForInputSession);
+- (void)ensureInputSession
+{
+    Util::run(&_hasStartedInputSession);
 }
 
@@ -708 +714 @@
 - (void)_webView:(WKWebView *)webView didStartInputSession:(id <_WKFormInputSession>)inputSession
 {
-    _isDoneWaitingForInputSession = true;
+    _hasStartedInputSession = true;
 }
 

trunk/Tools/TestWebKitAPI/ios/UIKitSPI.h

@@ -84 +84 @@
 - (void)_preserveFocusWithToken:(id <NSCopying, NSSecureCoding>)token destructively:(BOOL)destructively;
 - (BOOL)_restoreFocusWithToken:(id <NSCopying, NSSecureCoding>)token;
+- (void)_clearToken:(id <NSCopying, NSSecureCoding>)token;
 @end