Attachment 356132 Details for Bug 192165 – Patch for landing

[patch] Patch for landing

bug-192165-20181129214711.patch (text/plain), 17.82 KB, created by Wenson Hsieh on 2018-11-29 21:47:12 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Wenson Hsieh

Created: 2018-11-29 21:47:12 PST

Size: 17.82 KB

patch

obsolete

>Subversion Revision: 238722
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index 45c5dda11f9abe7b2d68a70a30fad9b4d638f2c5..471efee72414959ef04489df828ec9af5c668781 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,26 @@
>+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  Ryan Haddad  <ryanhaddad@apple.com>
> 
>         Unreviewed, rolling out r238713.
>diff --git a/Source/WebKit/UIProcess/ios/PageClientImplIOS.mm b/Source/WebKit/UIProcess/ios/PageClientImplIOS.mm
>index a2e39a70cd94c1ff77557278cb8926da8592bc33..968fde96328ef126d36b80ff18a5de161ec6a111 100644
>--- a/Source/WebKit/UIProcess/ios/PageClientImplIOS.mm
>+++ b/Source/WebKit/UIProcess/ios/PageClientImplIOS.mm
>@@ -166,9 +166,7 @@ bool PageClientImpl::isViewWindowActive()
> 
> 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];
> }
> 
> bool PageClientImpl::isViewVisible()
>diff --git a/Tools/ChangeLog b/Tools/ChangeLog
>index 2a468ac4c7b29a498614fd7942dc7b3a0ab7037d..a19aae1565aa1eca5c5a0dfe431f471167d96782 100644
>--- a/Tools/ChangeLog
>+++ b/Tools/ChangeLog
>@@ -1,3 +1,54 @@
>+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  Fujii Hironori  <Hironori.Fujii@sony.com>
> 
>         REGRESSION(r238445)[Buildbot] Unknown builder 'GTK Linux 32-bit Release' in scheduler 'trunk'
>diff --git a/Tools/TestWebKitAPI/Tests/WebKitCocoa/WKWebViewEditActions.mm b/Tools/TestWebKitAPI/Tests/WebKitCocoa/WKWebViewEditActions.mm
>index 33700e043dc73a632cfd49a928ed3e589a55b1ab..1019482054f8a38d601b9c7aeab0cf166057cfef 100644
>--- a/Tools/TestWebKitAPI/Tests/WebKitCocoa/WKWebViewEditActions.mm
>+++ b/Tools/TestWebKitAPI/Tests/WebKitCocoa/WKWebViewEditActions.mm
>@@ -78,6 +78,7 @@ static RetainPtr<TestWKWebView> webViewForEditActionTesting(NSString *markup)
>     auto webView = adoptNS([[TestWKWebView alloc] initWithFrame:NSMakeRect(0, 0, 400, 400)]);
>     [webView synchronouslyLoadHTMLString:markup];
>     [webView _setEditable:YES];
>+    [webView becomeFirstResponder];
>     [webView stringByEvaluatingJavaScript:@"getSelection().setPosition(document.body, 1)"];
>     return webView;
> }
>@@ -87,6 +88,7 @@ static RetainPtr<TestWKWebView> webViewForEditActionTestingWithPageNamed(NSStrin
>     auto webView = adoptNS([[TestWKWebView alloc] initWithFrame:NSMakeRect(0, 0, 400, 400)]);
>     [webView synchronouslyLoadTestPageNamed:testPageName];
>     [webView _setEditable:YES];
>+    [webView becomeFirstResponder];
>     [webView stringByEvaluatingJavaScript:@"getSelection().setPosition(document.body, 1)"];
>     return webView;
> }
>@@ -243,12 +245,11 @@ TEST(WKWebViewEditActions, PasteAsQuotation)
> TEST(WKWebViewEditActions, PasteAndMatchStyle)
> {
>     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];
>     EXPECT_FALSE([destination stringByEvaluatingJavaScript:@"document.queryCommandState('bold')"].boolValue);
>diff --git a/Tools/TestWebKitAPI/Tests/WebKitCocoa/autofocus-contenteditable.html b/Tools/TestWebKitAPI/Tests/WebKitCocoa/autofocus-contenteditable.html
>index e03635f94e30bd7f9fe34423c07e85811c4edb9e..7fc2bc3dfcc197ef269952b4ba5813a090213620 100644
>--- a/Tools/TestWebKitAPI/Tests/WebKitCocoa/autofocus-contenteditable.html
>+++ b/Tools/TestWebKitAPI/Tests/WebKitCocoa/autofocus-contenteditable.html
>@@ -14,6 +14,10 @@
>             white-space: nowrap;
>         }
> 
>+        #source {
>+            overflow: hidden;
>+        }
>+
>         #editor {
>             border: black 1px solid;
>         }
>diff --git a/Tools/TestWebKitAPI/Tests/WebKitCocoa/contenteditable-and-textarea.html b/Tools/TestWebKitAPI/Tests/WebKitCocoa/contenteditable-and-textarea.html
>index 41489fc3946475ff948faffd5382d0726f38b2c1..a4b417ce27dc9c663b3618eac8cac16f0abd802b 100644
>--- a/Tools/TestWebKitAPI/Tests/WebKitCocoa/contenteditable-and-textarea.html
>+++ b/Tools/TestWebKitAPI/Tests/WebKitCocoa/contenteditable-and-textarea.html
>@@ -14,6 +14,10 @@
>             white-space: nowrap;
>         }
> 
>+        #source {
>+            overflow: hidden;
>+        }
>+
>         #editor {
>             border: black 1px solid;
>         }
>diff --git a/Tools/TestWebKitAPI/Tests/ios/DragAndDropTestsIOS.mm b/Tools/TestWebKitAPI/Tests/ios/DragAndDropTestsIOS.mm
>index b879c0fdd16ee91341ed265b8e278bf0beb9b13e..29ef6747a3cfaf131c1904a973724bb78b1160c5 100644
>--- a/Tools/TestWebKitAPI/Tests/ios/DragAndDropTestsIOS.mm
>+++ b/Tools/TestWebKitAPI/Tests/ios/DragAndDropTestsIOS.mm
>@@ -98,6 +98,15 @@ - (NSString *)editorValue
> 
> @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)
> {
>     return [NSValue valueWithCGRect:CGRectMake(x, y, width, height)];
>@@ -341,8 +350,7 @@ TEST(DragAndDropTests, ContentEditableToContentEditable)
>     auto webView = adoptNS([[TestWKWebView alloc] initWithFrame:CGRectMake(0, 0, 320, 500)]);
>     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)];
> 
>     EXPECT_TRUE([simulator suppressedSelectionCommandsDuringDrop]);
>@@ -362,8 +370,7 @@ TEST(DragAndDropTests, ContentEditableToTextarea)
>     auto webView = adoptNS([[TestWKWebView alloc] initWithFrame:CGRectMake(0, 0, 320, 500)]);
>     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)];
> 
>     EXPECT_TRUE([simulator suppressedSelectionCommandsDuringDrop]);
>@@ -394,8 +401,7 @@ TEST(DragAndDropTests, ContentEditableMoveParagraphs)
>     auto webView = adoptNS([[TestWKWebView alloc] initWithFrame:CGRectMake(0, 0, 320, 500)]);
>     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)];
> 
>     NSString *finalTextContent = [webView stringByEvaluatingJavaScript:@"editor.textContent"];
>@@ -425,8 +431,7 @@ TEST(DragAndDropTests, TextAreaToInput)
>     auto webView = adoptNS([[TestWKWebView alloc] initWithFrame:CGRectMake(0, 0, 320, 500)]);
>     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)];
> 
>     EXPECT_TRUE([simulator suppressedSelectionCommandsDuringDrop]);
>@@ -440,8 +445,7 @@ TEST(DragAndDropTests, SinglePlainTextWordTypeIdentifiers)
>     auto webView = adoptNS([[TestWKWebView alloc] initWithFrame:CGRectMake(0, 0, 320, 500)]);
>     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"];
>     [webView stringByEvaluatingJavaScript:@"source.selectionEnd = source.value.length"];
>@@ -463,8 +467,7 @@ TEST(DragAndDropTests, SinglePlainTextURLTypeIdentifiers)
>     auto webView = adoptNS([[TestWKWebView alloc] initWithFrame:CGRectMake(0, 0, 320, 500)]);
>     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"];
>     [webView stringByEvaluatingJavaScript:@"source.selectionEnd = source.value.length"];
>diff --git a/Tools/TestWebKitAPI/cocoa/DragAndDropSimulator.h b/Tools/TestWebKitAPI/cocoa/DragAndDropSimulator.h
>index a398f982a344f0ad7d4509ea1e69cdc88fa97857..4fe0b8428c08be74d98a991aae39624bb94c0749 100644
>--- a/Tools/TestWebKitAPI/cocoa/DragAndDropSimulator.h
>+++ b/Tools/TestWebKitAPI/cocoa/DragAndDropSimulator.h
>@@ -89,7 +89,7 @@ typedef NSDictionary<NSNumber *, NSValue *> *ProgressToCGPointValueMap;
> 
> - (instancetype)initWithWebView:(TestWKWebView *)webView;
> - (void)runFrom:(CGPoint)startLocation to:(CGPoint)endLocation additionalItemRequestLocations:(ProgressToCGPointValueMap)additionalItemRequestLocations;
>-- (void)waitForInputSession;
>+- (void)ensureInputSession;
> 
> @property (nonatomic, readonly) DragAndDropPhase phase;
> @property (nonatomic) BOOL allowsFocusToStartInputSession;
>diff --git a/Tools/TestWebKitAPI/ios/DragAndDropSimulatorIOS.mm b/Tools/TestWebKitAPI/ios/DragAndDropSimulatorIOS.mm
>index 2b00c9f5586e6f23d422d6171c1be87fdd0451e9..3d23efcb0a837f0f4aee55b3eeef2df387c0666c 100644
>--- a/Tools/TestWebKitAPI/ios/DragAndDropSimulatorIOS.mm
>+++ b/Tools/TestWebKitAPI/ios/DragAndDropSimulatorIOS.mm
>@@ -310,7 +310,7 @@ @implementation DragAndDropSimulator {
>     RetainPtr<NSMutableArray<_WKAttachment *>> _insertedAttachments;
>     RetainPtr<NSMutableArray<_WKAttachment *>> _removedAttachments;
> 
>-    bool _isDoneWaitingForInputSession;
>+    bool _hasStartedInputSession;
>     double _currentProgress;
>     bool _isDoneWithCurrentRun;
>     DragAndDropPhase _phase;
>@@ -338,7 +338,6 @@ - (instancetype)initWithWebView:(TestWKWebView *)webView
>         _webView = webView;
>         _shouldEnsureUIApplication = NO;
>         _shouldAllowMoveOperation = YES;
>-        _isDoneWaitingForInputSession = true;
>         [_webView setUIDelegate:self];
>         [_webView _setInputDelegate:self];
>     }
>@@ -373,6 +372,7 @@ - (void)_resetSimulatedState
>     _remainingAdditionalItemRequestLocationsByProgress = nil;
>     _queuedAdditionalItemRequestLocations = adoptNS([[NSMutableArray alloc] init]);
>     _liftPreviews = adoptNS([[NSMutableArray alloc] init]);
>+    _hasStartedInputSession = false;
> }
> 
> - (NSArray *)observedEventNames
>@@ -459,8 +459,13 @@ - (void)_concludeDropAndPerformOperationIfNecessary
> 
>     [[_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];
>+    }
> }
> 
> - (void)_enqueuePendingAdditionalItemRequestLocations
>@@ -543,7 +548,13 @@ - (void)_advanceProgress
>             return;
>         }
> 
>-        [[_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;
>         dispatch_async(dispatch_get_main_queue(), ^() {
>@@ -618,14 +629,9 @@ - (CGRect)lastKnownDragCaretRect
>     return _lastKnownDragCaretRect;
> }
> 
>-- (void)waitForInputSession
>+- (void)ensureInputSession
> {
>-    _isDoneWaitingForInputSession = false;
>-
>-    // Waiting for an input session implies that we should allow input sessions to begin.
>-    self.allowsFocusToStartInputSession = YES;
>-
>-    Util::run(&_isDoneWaitingForInputSession);
>+    Util::run(&_hasStartedInputSession);
> }
> 
> - (NSArray<_WKAttachment *> *)insertedAttachments
>@@ -707,7 +713,7 @@ - (BOOL)_webView:(WKWebView *)webView focusShouldStartInputSession:(id <_WKFocus
> 
> - (void)_webView:(WKWebView *)webView didStartInputSession:(id <_WKFormInputSession>)inputSession
> {
>-    _isDoneWaitingForInputSession = true;
>+    _hasStartedInputSession = true;
> }
> 
> @end
>diff --git a/Tools/TestWebKitAPI/ios/UIKitSPI.h b/Tools/TestWebKitAPI/ios/UIKitSPI.h
>index b84d945e8bd56ff461d2001886553744ed08e6af..f7bd9398df39d5b1a9358048c88391c03df02f0a 100644
>--- a/Tools/TestWebKitAPI/ios/UIKitSPI.h
>+++ b/Tools/TestWebKitAPI/ios/UIKitSPI.h
>@@ -83,6 +83,7 @@ WTF_EXTERN_C_END
> @optional
> - (void)_preserveFocusWithToken:(id <NSCopying, NSSecureCoding>)token destructively:(BOOL)destructively;
> - (BOOL)_restoreFocusWithToken:(id <NSCopying, NSSecureCoding>)token;
>+- (void)_clearToken:(id <NSCopying, NSSecureCoding>)token;
> @end
> 
> @interface NSURL ()

Actions: View | Formatted Diff | Diff

Attachments on bug 192165: 356106 | 356107 | 356132