Attachment 372379 Details for Bug 198928 – Patch

[patch] Patch

bug-198928-20190618135708.patch (text/plain), 10.16 KB, created by Daniel Bates on 2019-06-18 13:57:09 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Daniel Bates

Created: 2019-06-18 13:57:09 PDT

Size: 10.16 KB

patch

obsolete

>Subversion Revision: 246325
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index 4a486fad1b7ec6ae9cdde96d33c11fb125f49717..9badf8ac14700ec215eb8a73ac82e8aa61b2b1c9 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,51 @@
>+2019-06-18  Daniel Bates  <dabates@apple.com>
>+
>+        m_focusedElement != &element in WebPage::elementDidBlur() sometimes
>+        https://bugs.webkit.org/show_bug.cgi?id=198928
>+        <rdar://problem/51814327>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        This can happen when the focused editable element is inside a nested frame and a person
>+        taps outside that frame. For reasons that seem lost to time, WebKit2 on iOS would mutate
>+        the focused frame whenever computing selection positioning information (say, for a tap).
>+        This "quirk" was added in r163476, but that code has go through multiple iterations and
>+        is no longer comparable to the current code. Yet, the original mutation of the focused
>+        frame remained. As a result the UI process and Web process go out of sync with respect
>+        to what each thinks is the focused element and this visually manifest itself in at least
>+        two ways:
>+
>+            1. A non-sensical DOM focus event would be dispatched at the frame tapped, but
>+            we would keep the focused element focused.
>+
>+            2. Because we would keep the focused element focused in (1), even though its frame
>+            is not focused, the keyboard would be active (software keyboard on screen or candidate bar
>+            on screen if a hardware keyboard is attached), but appear unresponsive: any keys pressed
>+            would not cause text to be typed into the editable field.
>+
>+        Because of (1) it was possible for m_focusedElement != &element in WebPage::elementDidBlur().
>+        When this happens the UI process would never receive an ElementDidBlur message and hence would
>+        not clear out the focused element state and hide the keyboard.
>+ 
>+        We neither do this frame focus mutation in Legacy WebKit on iOS nor Mac. Let's remove this quirk.
>+        If it turns out that it causes a compatibility issue then we will be in a better position to
>+        understand its purpose and consider bringing this quirk back, if needed.
>+
>+        * Shared/ios/InteractionInformationRequest.cpp:
>+        (WebKit::InteractionInformationRequest::encode const):
>+        (WebKit::InteractionInformationRequest::decode):
>+        (WebKit::InteractionInformationRequest::isValidForRequest):
>+        (WebKit::InteractionInformationRequest::isApproximatelyValidForRequest):
>+        * Shared/ios/InteractionInformationRequest.h:
>+        * UIProcess/API/Cocoa/WKWebView.mm:
>+        (-[WKWebView _requestActivatedElementAtPosition:completionBlock:]):
>+        * UIProcess/ios/WKContentViewInteraction.mm:
>+        (-[WKContentView _webTouchEventsRecognized:]):
>+        Remove the readOnly field.
>+
>+        * WebProcess/WebPage/ios/WebPageIOS.mm:
>+        (WebKit::selectionPositionInformation): Remove code to mutate the focused frame.
>+
> 2019-06-18  Daniel Bates  <dabates@apple.com>
> 
>         REGRESSION (r240757): Cannot dismiss the keyboard on http://apple.com/apple-tv-plus
>diff --git a/Source/WebKit/Shared/ios/InteractionInformationRequest.cpp b/Source/WebKit/Shared/ios/InteractionInformationRequest.cpp
>index 7f1063afa16b7cda43cc82b4066a31622cf240be..fe58a3f1e6920ce3cdbaa344191e4552c49cadda 100644
>--- a/Source/WebKit/Shared/ios/InteractionInformationRequest.cpp
>+++ b/Source/WebKit/Shared/ios/InteractionInformationRequest.cpp
>@@ -38,7 +38,6 @@ void InteractionInformationRequest::encode(IPC::Encoder& encoder) const
>     encoder << point;
>     encoder << includeSnapshot;
>     encoder << includeLinkIndicator;
>-    encoder << readonly;
> }
> 
> bool InteractionInformationRequest::decode(IPC::Decoder& decoder, InteractionInformationRequest& result)
>@@ -52,9 +51,6 @@ bool InteractionInformationRequest::decode(IPC::Decoder& decoder, InteractionInf
>     if (!decoder.decode(result.includeLinkIndicator))
>         return false;
> 
>-    if (!decoder.decode(result.readonly))
>-        return false;
>-
>     return true;
> }
> 
>@@ -69,9 +65,6 @@ bool InteractionInformationRequest::isValidForRequest(const InteractionInformati
>     if (other.includeLinkIndicator && !includeLinkIndicator)
>         return false;
> 
>-    if (!other.readonly && readonly)
>-        return false;
>-
>     return true;
> }
>     
>@@ -83,9 +76,6 @@ bool InteractionInformationRequest::isApproximatelyValidForRequest(const Interac
>     if (other.includeLinkIndicator && !includeLinkIndicator)
>         return false;
> 
>-    if (!other.readonly && readonly)
>-        return false;
>-    
>     return (other.point - point).diagonalLengthSquared() <= 4;
> }
> 
>diff --git a/Source/WebKit/Shared/ios/InteractionInformationRequest.h b/Source/WebKit/Shared/ios/InteractionInformationRequest.h
>index 15bbe0943848e31df4299b3e9ea1cc607af213ba..d274babec7a103fcc8e6c9d1922479fc03f85790 100644
>--- a/Source/WebKit/Shared/ios/InteractionInformationRequest.h
>+++ b/Source/WebKit/Shared/ios/InteractionInformationRequest.h
>@@ -42,11 +42,6 @@ struct InteractionInformationRequest {
>     bool includeSnapshot { false };
>     bool includeLinkIndicator { false };
> 
>-    // FIXME: This readonly flag ought to be true by default, but there are a number of interactions (e.g. selection) that currently
>-    // rely on the fact that interaction information requests additionally change the focused frame. We should explicitly turn the
>-    // readonly bit off in these scenarios, and make sure that all other position information requests don't move focus around.
>-    bool readonly { false };
>-
>     InteractionInformationRequest() { }
>     explicit InteractionInformationRequest(WebCore::IntPoint point)
>     {
>diff --git a/Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm b/Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm
>index a1beb77b0ec1fd680e8c3824feb83187359a0a4a..6b39acb0db6ed5905e8ba2957da79d8bfeff1a62 100644
>--- a/Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm
>+++ b/Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm
>@@ -6657,7 +6657,6 @@ - (void)_requestActivatedElementAtPosition:(CGPoint)position completionBlock:(vo
> {
>     auto infoRequest = WebKit::InteractionInformationRequest(WebCore::roundedIntPoint(position));
>     infoRequest.includeSnapshot = true;
>-    infoRequest.readonly = true;
> 
>     [_contentView doAfterPositionInformationUpdate:[capturedBlock = makeBlockPtr(block)] (WebKit::InteractionInformationAtPosition information) {
>         capturedBlock([_WKActivatedElementInfo activatedElementInfoWithInteractionInformationAtPosition:information]);
>diff --git a/Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm b/Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm
>index 1d68a030d5b7bc386c65e161373ee67fff45e97b..7b5dc57b52682f365e0a31f72893193d10ac7abb 100644
>--- a/Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm
>+++ b/Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm
>@@ -1308,7 +1308,6 @@ - (void)_webTouchEventsRecognized:(UIWebTouchEventsGestureRecognizer *)gestureRe
>         _layerTreeTransactionIdAtLastTouchStart = downcast<WebKit::RemoteLayerTreeDrawingAreaProxy>(*_page->drawingArea()).lastCommittedLayerTreeTransactionID();
> 
>         WebKit::InteractionInformationRequest positionInformationRequest { WebCore::IntPoint(_lastInteractionLocation) };
>-        positionInformationRequest.readonly = true;
>         [self doAfterPositionInformationUpdate:[assistant = WeakObjCPtr<WKActionSheetAssistant>(_actionSheetAssistant.get())] (WebKit::InteractionInformationAtPosition information) {
>             [assistant interactionDidStartWithPositionInformation:information];
>         } forRequest:positionInformationRequest];
>diff --git a/Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm b/Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm
>index 9f702b01c2308bca4a77dd1d6954eca77413002b..38e1060ffbe98211dc6db73ff63b9a12060d28ca 100644
>--- a/Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm
>+++ b/Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm
>@@ -2587,9 +2587,6 @@ static void selectionPositionInformation(WebPage& page, const InteractionInforma
>         return;
> 
>     RenderObject* renderer = hitNode->renderer();
>-    if (!request.readonly)
>-        page.corePage()->focusController().setFocusedFrame(result.innerNodeFrame());
>-
>     info.bounds = renderer->absoluteBoundingBoxRect(true);
>     // We don't want to select blocks that are larger than 97% of the visible area of the document.
>     if (is<HTMLAttachmentElement>(*hitNode)) {
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index d634ed99197f3a53d7238f3be179456be54e7fe5..2eaf19c91f488620068b00c97550c59e3b1dc43d 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,15 @@
>+2019-06-18  Daniel Bates  <dabates@apple.com>
>+
>+        m_focusedElement != &element in WebPage::elementDidBlur() sometimes
>+        https://bugs.webkit.org/show_bug.cgi?id=198928
>+        <rdar://problem/51814327>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Update test result now that we do not mutate the focused frame on tap.
>+
>+        * fast/events/ios/should-be-able-to-dismiss-form-accessory-after-tapping-outside-iframe-with-focused-field-expected.txt:
>+
> 2019-06-18  Daniel Bates  <dabates@apple.com>
> 
>         REGRESSION (r240757): Cannot dismiss the keyboard on http://apple.com/apple-tv-plus
>diff --git a/LayoutTests/fast/events/ios/should-be-able-to-dismiss-form-accessory-after-tapping-outside-iframe-with-focused-field-expected.txt b/LayoutTests/fast/events/ios/should-be-able-to-dismiss-form-accessory-after-tapping-outside-iframe-with-focused-field-expected.txt
>index 906fc2538b441a9b260bae61628f8f96c0e293f9..806179fe54572413f047f886c483e59341b601b9 100644
>--- a/LayoutTests/fast/events/ios/should-be-able-to-dismiss-form-accessory-after-tapping-outside-iframe-with-focused-field-expected.txt
>+++ b/LayoutTests/fast/events/ios/should-be-able-to-dismiss-form-accessory-after-tapping-outside-iframe-with-focused-field-expected.txt
>@@ -5,7 +5,7 @@ On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE
> 
> dispatched blur to main frame
> dispatched focus to <input>
>-dispatched focus to main frame
>+dispatched blur to <input>
> PASS dismissed form accessory
> PASS successfullyParsed is true
>

Actions: View | Formatted Diff | Diff

Attachments on bug 198928: 372379 | 372387 | 372406 | 372787