Attachment 372787 Details for Bug 198928 – To land

[patch] To land

bug-198928-20190624124647.patch (text/plain), 11.10 KB, created by Daniel Bates on 2019-06-24 12:46:48 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Daniel Bates

Created: 2019-06-24 12:46:48 PDT

Size: 11.10 KB

patch

obsolete

>Subversion Revision: 246754
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index 98ed8be8cf36b8bb33d622eae1c26acd39463733..a891cdceba038b3d3a9708edb8821ffc12d8f942 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,51 @@
>+2019-06-24  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 Brent Fulgham.
>+
>+        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-24  Per Arne Vollan  <pvollan@apple.com>
> 
>         [Cocoa] Avoid creating a PlatformMediaSessionManager when the WebProcess is suspended or resumed
>diff --git a/Source/WebKit/Shared/ios/InteractionInformationRequest.cpp b/Source/WebKit/Shared/ios/InteractionInformationRequest.cpp
>index 3d704e6e6e6da38a8bc7dd4021af1e1b5343625b..0bd841619f748c34fab94ebc2da626cbeb560c9d 100644
>--- a/Source/WebKit/Shared/ios/InteractionInformationRequest.cpp
>+++ b/Source/WebKit/Shared/ios/InteractionInformationRequest.cpp
>@@ -39,7 +39,6 @@ void InteractionInformationRequest::encode(IPC::Encoder& encoder) const
>     encoder << includeSnapshot;
>     encoder << includeLinkIndicator;
>     encoder << linkIndicatorShouldHaveLegacyMargins;
>-    encoder << readonly;
> }
> 
> bool InteractionInformationRequest::decode(IPC::Decoder& decoder, InteractionInformationRequest& result)
>@@ -56,9 +55,6 @@ bool InteractionInformationRequest::decode(IPC::Decoder& decoder, InteractionInf
>     if (!decoder.decode(result.linkIndicatorShouldHaveLegacyMargins))
>         return false;
> 
>-    if (!decoder.decode(result.readonly))
>-        return false;
>-
>     return true;
> }
> 
>@@ -76,9 +72,6 @@ bool InteractionInformationRequest::isValidForRequest(const InteractionInformati
>     if (other.linkIndicatorShouldHaveLegacyMargins != linkIndicatorShouldHaveLegacyMargins)
>         return false;
> 
>-    if (!other.readonly && readonly)
>-        return false;
>-
>     return true;
> }
>     
>@@ -90,9 +83,6 @@ bool InteractionInformationRequest::isApproximatelyValidForRequest(const Interac
>     if (other.includeLinkIndicator && !includeLinkIndicator)
>         return false;
> 
>-    if (!other.readonly && readonly)
>-        return false;
>-
>     if (other.linkIndicatorShouldHaveLegacyMargins != linkIndicatorShouldHaveLegacyMargins)
>         return false;
>     
>diff --git a/Source/WebKit/Shared/ios/InteractionInformationRequest.h b/Source/WebKit/Shared/ios/InteractionInformationRequest.h
>index b318b40e31ae712aedb7140f91a20df3834df4bb..9e2cfcf5d8b4b3819e841ca99198992127292ecb 100644
>--- a/Source/WebKit/Shared/ios/InteractionInformationRequest.h
>+++ b/Source/WebKit/Shared/ios/InteractionInformationRequest.h
>@@ -44,11 +44,6 @@ struct InteractionInformationRequest {
> 
>     bool linkIndicatorShouldHaveLegacyMargins { 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 bbeaef4865e1da30bdf1ac443adee07108f86397..c77c25ced57de5a7157695df38e85ce13b16cb73 100644
>--- a/Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm
>+++ b/Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm
>@@ -6675,7 +6675,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 702ea88737ccd606726277d52e64866838adc453..53b0be997b51aa919962cfa44c5b52c903863bfa 100644
>--- a/Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm
>+++ b/Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm
>@@ -1316,7 +1316,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 ad17b6e73ac7f2ab603193fbefd6f9aa1275ccaf..044e11f017876901c81d39124bf68fde38e1cfa0 100644
>--- a/Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm
>+++ b/Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm
>@@ -2620,9 +2620,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 4f111d89d98287ddb5838662ce4b2d925e3d67f7..668f5d1d7137ac25574448b472a082266c014887 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,18 @@
>+2019-06-24  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 Brent Fulgham.
>+
>+        Update test result now that we do not mutate the focused frame on tap.
>+
>+        * TestExpectations: Skip problematic test editing/deleting/smart-delete-paragraph-003.html;
>+        See <https://bugs.webkit.org/show_bug.cgi?id=198928#c16>, <https://bugs.webkit.org/show_bug.cgi?id=198928#c17>,
>+        and <https://bugs.webkit.org/show_bug.cgi?id=199039> for more details.
>+        * fast/events/ios/should-be-able-to-dismiss-form-accessory-after-tapping-outside-iframe-with-focused-field-expected.txt:
>+
> 2019-06-24  Antoine Quint  <graouts@apple.com>
> 
>         [Pointer Events WPT] Unskip imported/w3c/web-platform-tests/pointerevents/pointerlock/pointerevent_coordinates_when_locked.html
>diff --git a/LayoutTests/TestExpectations b/LayoutTests/TestExpectations
>index e73d995fae93fef508a2138ad83e1d8dace3c7dd..126fd018691b2dabfe6c249d4bc3544ec0250080 100644
>--- a/LayoutTests/TestExpectations
>+++ b/LayoutTests/TestExpectations
>@@ -3438,3 +3438,5 @@ imported/w3c/web-platform-tests/websockets/Secure-Send-unpaired-surrogates.any.w
> 
> # iOS only
> fast/dom/linkify-phone-numbers.html [ ImageOnlyFailure ]
>+
>+webkit.org/b/199039 editing/deleting/smart-delete-paragraph-003.html [ Skip ]
>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