Attachment 362548 Details for Bug 194601 – Patch for landing

[patch] Patch for landing

bug-194601-20190220145718.patch (text/plain), 11.45 KB, created by Wenson Hsieh on 2019-02-20 14:57:18 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Wenson Hsieh

Created: 2019-02-20 14:57:18 PST

Size: 11.45 KB

patch

obsolete

>Subversion Revision: 241754
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index 6256c8af0edd9565666e9df373d167b55e74ad0c..607c559e44edd87a06b5905bf6bcf5bd8b0fce23 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -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-18  Alex Christensen  <achristensen@webkit.org>
> 
>         Revert functional part of r241451
>diff --git a/Source/WebKit/UIProcess/WebPageProxy.cpp b/Source/WebKit/UIProcess/WebPageProxy.cpp
>index 52b4c8e227a3bd3d7e457bf6c5dcd0e36b187bda..43d5603809ffd89059361b49c22323dc902fe28e 100644
>--- a/Source/WebKit/UIProcess/WebPageProxy.cpp
>+++ b/Source/WebKit/UIProcess/WebPageProxy.cpp
>@@ -7984,20 +7984,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);
>@@ -8032,6 +8018,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);
>diff --git a/Source/WebKit/UIProcess/WebPageProxy.h b/Source/WebKit/UIProcess/WebPageProxy.h
>index b2b0e85d2e0d159b08d09b33817c8fc126c0d84f..4aab802578aa4ad70fb1773fc5c5461047cd9617 100644
>--- a/Source/WebKit/UIProcess/WebPageProxy.h
>+++ b/Source/WebKit/UIProcess/WebPageProxy.h
>@@ -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
>diff --git a/Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm b/Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm
>index 7bd80fdd826ca364294ace7254d4f07c9ffd4df8..2b8390a7db359031b3215f98ff930a2bd7cefcfa 100644
>--- a/Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm
>+++ b/Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm
>@@ -868,7 +868,7 @@ - (void)cleanupInteraction
>     }
> #endif
> 
>-    _inputViewUpdateDeferrer = nullptr;
>+    [self _resetInputViewDeferral];
>     _focusedElementInformation = { };
>     
>     [_keyboardScrollingAnimator invalidate];
>@@ -1072,6 +1072,25 @@ - (void)_endEditing
> #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 @@ - (BOOL)becomeFirstResponderForWebView
>     }
> 
>     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 @@ - (BOOL)resignFirstResponderForWebView
>     [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.
>@@ -2181,12 +2210,12 @@ - (void)_commitPotentialTapFailed
> {
>     [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.
>@@ -2206,7 +2235,7 @@ - (void)_didNotHandleTapAsClick:(const WebCore::IntPoint&)point
> 
> - (void)_didCompleteSyntheticClick
> {
>-    _inputViewUpdateDeferrer = nullptr;
>+    [self _resetInputViewDeferral];
> }
> 
> - (void)_singleTapCommited:(UITapGestureRecognizer *)gestureRecognizer
>diff --git a/Source/WebKit/WebProcess/WebPage/RemoteLayerTree/RemoteLayerTreeDrawingArea.mm b/Source/WebKit/WebProcess/WebPage/RemoteLayerTree/RemoteLayerTreeDrawingArea.mm
>index ce2d5cbdece8d9dc5177168df056109a30ee89b2..8e961e014c53bd0f95d9e1699db1ab4498117412 100644
>--- a/Source/WebKit/WebProcess/WebPage/RemoteLayerTree/RemoteLayerTreeDrawingArea.mm
>+++ b/Source/WebKit/WebProcess/WebPage/RemoteLayerTree/RemoteLayerTreeDrawingArea.mm
>@@ -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)

Actions: View | Formatted Diff | Diff

Attachments on bug 194601: 362478 | 362531 | 362548