m_focusedElement != &element in WebPage::elementDidBlur() sometimes
authordbates@webkit.org <dbates@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 24 Jun 2019 19:47:31 +0000 (19:47 +0000)
committerdbates@webkit.org <dbates@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 24 Jun 2019 19:47:31 +0000 (19:47 +0000)
https://bugs.webkit.org/show_bug.cgi?id=198928
<rdar://problem/51814327>

Reviewed by Brent Fulgham.

Source/WebKit:

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.

LayoutTests:

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:

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

LayoutTests/ChangeLog
LayoutTests/TestExpectations
LayoutTests/fast/events/ios/should-be-able-to-dismiss-form-accessory-after-tapping-outside-iframe-with-focused-field-expected.txt
Source/WebKit/ChangeLog
Source/WebKit/Shared/ios/InteractionInformationRequest.cpp
Source/WebKit/Shared/ios/InteractionInformationRequest.h
Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm
Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm
Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm

index 4f111d8..668f5d1 100644 (file)
@@ -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
index e73d995..126fd01 100644 (file)
@@ -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 ]
index 906fc25..806179f 100644 (file)
@@ -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
 
index 98ed8be..a891cdc 100644 (file)
@@ -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
index 3d704e6..0bd8416 100644 (file)
@@ -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;
     
index b318b40..9e2cfcf 100644 (file)
@@ -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)
     {
index bbeaef4..c77c25c 100644 (file)
@@ -6675,7 +6675,6 @@ static WebCore::UserInterfaceLayoutDirection toUserInterfaceLayoutDirection(UISe
 {
     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]);
index 702ea88..53b0be9 100644 (file)
@@ -1316,7 +1316,6 @@ inline static UIKeyModifierFlags gestureRecognizerModifierFlags(UIGestureRecogni
         _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];
index ad17b6e..044e11f 100644 (file)
@@ -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)) {