REGRESSION: [ iOS ] Layout Test editing/input/ios/rtl-keyboard-input-on-focus.html...
authorwenson_hsieh@apple.com <wenson_hsieh@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 20 Feb 2019 23:34:50 +0000 (23:34 +0000)
committerwenson_hsieh@apple.com <wenson_hsieh@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 20 Feb 2019 23:34:50 +0000 (23:34 +0000)
https://bugs.webkit.org/show_bug.cgi?id=194601
<rdar://problem/48080316>

Reviewed by Tim Horton.

Following r241311, if a web view becomes first responder and is then moved offscreen (or obscured, hidden, or in
the case of WebKitTestRunner, its UIWindow loses its status as keyWindow), we end up holding on to the input
view update deferral token indefinitely, waiting for the current focused element to be blurred or refocused.

This also manifests other user-facing bugs, the most common of which is the keyboard occasionally remaining
onscreen after typing a URL in the unified field in MobileSafari and hitting Return, in the case where there is
no autofocused element on the page.

To fix this, when becoming the first responder, additionally install a callback to detect when the page is
finished handling the activity state change, and invalidate the input deferral token then. This retains the
behavior where calling -becomeFirstResponder on the web view while a different view is focused will keep the
keyboard stable, since the focused element message from the web process should be dispatched when handling the
activity state change within the web process.

Of course, the web process may not be responsive at all while the web view is still in the view hierarchy, in
which case we may still end up deferring input view updates indefinitely. In this case, we maintain a separate
watchdog timer with a short delay, after which we unconditionally invalidate the token.

* UIProcess/WebPageProxy.cpp:
(WebKit::WebPageProxy::installActivityStateChangeCompletionHandler):

Move the implementation of installActivityStateChangeCompletionHandler into cross-platform code.

* UIProcess/WebPageProxy.h:
* UIProcess/ios/WKContentView.mm:
* UIProcess/ios/WKContentViewInteraction.mm:
(-[WKContentView cleanupInteraction]):
(-[WKContentView _cancelPreviousResetInputViewDeferralRequest]):
(-[WKContentView _scheduleResetInputViewDeferralAfterBecomingFirstResponder]):
(-[WKContentView _resetInputViewDeferral]):
(-[WKContentView becomeFirstResponderForWebView]):
(-[WKContentView resignFirstResponderForWebView]):
(-[WKContentView _commitPotentialTapFailed]):
(-[WKContentView _didNotHandleTapAsClick:]):
(-[WKContentView _didCompleteSyntheticClick]):

Funnel all existing calls that reset _inputViewDeferralToken to nullptr, such that they go through a helper
method instead that also cancels any scheduled requests to clear the token.

* WebProcess/WebPage/RemoteLayerTree/RemoteLayerTreeDrawingArea.mm:
(WebKit::RemoteLayerTreeDrawingArea::activityStateDidChange):

Respond to all pending callbacks after handling the activity state change.

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

Source/WebKit/ChangeLog
Source/WebKit/UIProcess/WebPageProxy.cpp
Source/WebKit/UIProcess/WebPageProxy.h
Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm
Source/WebKit/WebProcess/WebPage/RemoteLayerTree/RemoteLayerTreeDrawingArea.mm

index 405e9ad..f355f37 100644 (file)
@@ -1,3 +1,55 @@
+2019-02-20  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        REGRESSION: [ iOS ] Layout Test editing/input/ios/rtl-keyboard-input-on-focus.html is a Timeout
+        https://bugs.webkit.org/show_bug.cgi?id=194601
+        <rdar://problem/48080316>
+
+        Reviewed by Tim Horton.
+
+        Following r241311, if a web view becomes first responder and is then moved offscreen (or obscured, hidden, or in
+        the case of WebKitTestRunner, its UIWindow loses its status as keyWindow), we end up holding on to the input
+        view update deferral token indefinitely, waiting for the current focused element to be blurred or refocused.
+
+        This also manifests other user-facing bugs, the most common of which is the keyboard occasionally remaining
+        onscreen after typing a URL in the unified field in MobileSafari and hitting Return, in the case where there is
+        no autofocused element on the page.
+
+        To fix this, when becoming the first responder, additionally install a callback to detect when the page is
+        finished handling the activity state change, and invalidate the input deferral token then. This retains the
+        behavior where calling -becomeFirstResponder on the web view while a different view is focused will keep the
+        keyboard stable, since the focused element message from the web process should be dispatched when handling the
+        activity state change within the web process.
+
+        Of course, the web process may not be responsive at all while the web view is still in the view hierarchy, in
+        which case we may still end up deferring input view updates indefinitely. In this case, we maintain a separate
+        watchdog timer with a short delay, after which we unconditionally invalidate the token.
+
+        * UIProcess/WebPageProxy.cpp:
+        (WebKit::WebPageProxy::installActivityStateChangeCompletionHandler):
+
+        Move the implementation of installActivityStateChangeCompletionHandler into cross-platform code.
+
+        * UIProcess/WebPageProxy.h:
+        * UIProcess/ios/WKContentView.mm:
+        * UIProcess/ios/WKContentViewInteraction.mm:
+        (-[WKContentView cleanupInteraction]):
+        (-[WKContentView _cancelPreviousResetInputViewDeferralRequest]):
+        (-[WKContentView _scheduleResetInputViewDeferralAfterBecomingFirstResponder]):
+        (-[WKContentView _resetInputViewDeferral]):
+        (-[WKContentView becomeFirstResponderForWebView]):
+        (-[WKContentView resignFirstResponderForWebView]):
+        (-[WKContentView _commitPotentialTapFailed]):
+        (-[WKContentView _didNotHandleTapAsClick:]):
+        (-[WKContentView _didCompleteSyntheticClick]):
+
+        Funnel all existing calls that reset _inputViewDeferralToken to nullptr, such that they go through a helper
+        method instead that also cancels any scheduled requests to clear the token.
+
+        * WebProcess/WebPage/RemoteLayerTree/RemoteLayerTreeDrawingArea.mm:
+        (WebKit::RemoteLayerTreeDrawingArea::activityStateDidChange):
+
+        Respond to all pending callbacks after handling the activity state change.
+
 2019-02-20  Chris Dumez  <cdumez@apple.com>
 
         Regression(PSON) "Reload without content extensions" does not work when the main resource is blocked
index 5a7a933..b08a1bf 100644 (file)
@@ -8004,20 +8004,6 @@ NSObject *WebPageProxy::immediateActionAnimationControllerForHitTestResult(RefPt
     return pageClient().immediateActionAnimationControllerForHitTestResult(hitTestResult, type, userData);
 }
 
-void WebPageProxy::installActivityStateChangeCompletionHandler(WTF::Function<void ()>&& completionHandler)
-{
-    if (!isValid()) {
-        completionHandler();
-        return;
-    }
-
-    auto voidCallback = VoidCallback::create([completionHandler = WTFMove(completionHandler)] (CallbackBase::Error) {
-        completionHandler();
-    }, m_process->throttler().backgroundActivityToken());
-    auto callbackID = m_callbacks.put(WTFMove(voidCallback));
-    m_nextActivityStateChangeCallbacks.append(callbackID);
-}
-
 void WebPageProxy::handleAcceptedCandidate(WebCore::TextCheckingResult acceptedCandidate)
 {
     m_process->send(Messages::WebPage::HandleAcceptedCandidate(acceptedCandidate), m_pageID);
@@ -8052,6 +8038,20 @@ void WebPageProxy::setFooterBannerHeightForTesting(int height)
 
 #endif
 
+void WebPageProxy::installActivityStateChangeCompletionHandler(Function<void()>&& completionHandler)
+{
+    if (!isValid()) {
+        completionHandler();
+        return;
+    }
+
+    auto voidCallback = VoidCallback::create([completionHandler = WTFMove(completionHandler)] (auto) {
+        completionHandler();
+    }, m_process->throttler().backgroundActivityToken());
+    auto callbackID = m_callbacks.put(WTFMove(voidCallback));
+    m_nextActivityStateChangeCallbacks.append(callbackID);
+}
+
 void WebPageProxy::imageOrMediaDocumentSizeChanged(const WebCore::IntSize& newSize)
 {
     m_uiClient->imageOrMediaDocumentSizeChanged(newSize);
index df67eec..faaa5b7 100644 (file)
@@ -1288,8 +1288,6 @@ public:
 
     NSObject *immediateActionAnimationControllerForHitTestResult(RefPtr<API::HitTestResult>, uint64_t, RefPtr<API::Object>);
 
-    void installActivityStateChangeCompletionHandler(WTF::Function<void ()>&&);
-
     void handleAcceptedCandidate(WebCore::TextCheckingResult);
     void didHandleAcceptedCandidate();
 
@@ -1297,6 +1295,8 @@ public:
     void setFooterBannerHeightForTesting(int);
 #endif
 
+    void installActivityStateChangeCompletionHandler(Function<void()>&&);
+
 #if USE(UNIFIED_TEXT_CHECKING)
     void checkTextOfParagraph(const String& text, OptionSet<WebCore::TextCheckingType> checkingTypes, int32_t insertionPoint, Vector<WebCore::TextCheckingResult>& results);
 #endif
index da150d8..3d9fa6e 100644 (file)
@@ -868,7 +868,7 @@ static inline bool hasFocusedElement(WebKit::FocusedElementInformation focusedEl
     }
 #endif
 
-    _inputViewUpdateDeferrer = nullptr;
+    [self _resetInputViewDeferral];
     _focusedElementInformation = { };
     
     [_keyboardScrollingAnimator invalidate];
@@ -1072,6 +1072,25 @@ static inline bool hasFocusedElement(WebKit::FocusedElementInformation focusedEl
 #endif
 }
 
+- (void)_cancelPreviousResetInputViewDeferralRequest
+{
+    [NSObject cancelPreviousPerformRequestsWithTarget:self selector:@selector(_resetInputViewDeferral) object:nil];
+}
+
+- (void)_scheduleResetInputViewDeferralAfterBecomingFirstResponder
+{
+    [self _cancelPreviousResetInputViewDeferralRequest];
+
+    const NSTimeInterval inputViewDeferralWatchdogTimerDuration = 0.5;
+    [self performSelector:@selector(_resetInputViewDeferral) withObject:self afterDelay:inputViewDeferralWatchdogTimerDuration];
+}
+
+- (void)_resetInputViewDeferral
+{
+    [self _cancelPreviousResetInputViewDeferralRequest];
+    _inputViewUpdateDeferrer = nullptr;
+}
+
 - (BOOL)canBecomeFirstResponder
 {
     return _becomingFirstResponder;
@@ -1106,12 +1125,22 @@ static inline bool hasFocusedElement(WebKit::FocusedElementInformation focusedEl
     }
 
     if (didBecomeFirstResponder) {
+        _page->installActivityStateChangeCompletionHandler([weakSelf = WeakObjCPtr<WKContentView>(self)] {
+            if (!weakSelf)
+                return;
+
+            auto strongSelf = weakSelf.get();
+            [strongSelf _resetInputViewDeferral];
+        });
+
         _page->activityStateDidChange(WebCore::ActivityState::IsFocused);
 
         if ([self canShowNonEmptySelectionView])
             [_textSelectionAssistant activateSelection];
+
+        [self _scheduleResetInputViewDeferralAfterBecomingFirstResponder];
     } else
-        _inputViewUpdateDeferrer = nullptr;
+        [self _resetInputViewDeferral];
 
     return didBecomeFirstResponder;
 }
@@ -1137,7 +1166,7 @@ static inline bool hasFocusedElement(WebKit::FocusedElementInformation focusedEl
     [self _cancelInteraction];
     [_textSelectionAssistant deactivateSelection];
     
-    _inputViewUpdateDeferrer = nullptr;
+    [self _resetInputViewDeferral];
 
     // If the user explicitly dismissed the keyboard then we will lose first responder
     // status only to gain it back again. Just don't resign in that case.
@@ -2177,12 +2206,12 @@ static void cancelPotentialTapIfNecessary(WKContentView* contentView)
 {
     [self _cancelInteraction];
     
-    _inputViewUpdateDeferrer = nullptr;
+    [self _resetInputViewDeferral];
 }
 
 - (void)_didNotHandleTapAsClick:(const WebCore::IntPoint&)point
 {
-    _inputViewUpdateDeferrer = nullptr;
+    [self _resetInputViewDeferral];
 
     // FIXME: we should also take into account whether or not the UI delegate
     // has handled this notification.
@@ -2202,7 +2231,7 @@ static void cancelPotentialTapIfNecessary(WKContentView* contentView)
 
 - (void)_didCompleteSyntheticClick
 {
-    _inputViewUpdateDeferrer = nullptr;
+    [self _resetInputViewDeferral];
 }
 
 - (void)_singleTapCommited:(UITapGestureRecognizer *)gestureRecognizer
index ce2d5cb..8e961e0 100644 (file)
@@ -36,6 +36,7 @@
 #import "RemoteScrollingCoordinator.h"
 #import "RemoteScrollingCoordinatorTransaction.h"
 #import "WebPage.h"
+#import "WebPageProxyMessages.h"
 #import "WebProcess.h"
 #import <QuartzCore/QuartzCore.h>
 #import <WebCore/DebugPageOverlays.h>
@@ -499,7 +500,7 @@ void RemoteLayerTreeDrawingArea::BackingStoreFlusher::flush()
     m_connection->sendMessage(WTFMove(m_commitEncoder), { });
 }
 
-void RemoteLayerTreeDrawingArea::activityStateDidChange(OptionSet<WebCore::ActivityState::Flag>, ActivityStateChangeID activityStateChangeID, const Vector<CallbackID>&)
+void RemoteLayerTreeDrawingArea::activityStateDidChange(OptionSet<WebCore::ActivityState::Flag>, ActivityStateChangeID activityStateChangeID, const Vector<CallbackID>& callbackIDs)
 {
     // FIXME: Should we suspend painting while not visible, like TiledCoreAnimationDrawingArea? Probably.
 
@@ -508,6 +509,11 @@ void RemoteLayerTreeDrawingArea::activityStateDidChange(OptionSet<WebCore::Activ
         m_activityStateChangeID = activityStateChangeID;
         scheduleCompositingLayerFlushImmediately();
     }
+
+    // FIXME: We may want to match behavior in TiledCoreAnimationDrawingArea by firing these callbacks after the next compositing flush, rather than immediately after
+    // handling an activity state change.
+    for (auto& callbackID : callbackIDs)
+        m_webPage.send(Messages::WebPageProxy::VoidCallback(callbackID));
 }
 
 void RemoteLayerTreeDrawingArea::addTransactionCallbackID(CallbackID callbackID)