Text input is largely broken when there are subframes loading
authorap@apple.com <ap@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 17 May 2013 20:04:43 +0000 (20:04 +0000)
committerap@apple.com <ap@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 17 May 2013 20:04:43 +0000 (20:04 +0000)
        http://bugs.webkit.org/show_bug.cgi?id=59121
        <rdar://problem/9320468>

        Reviewed by Darin Adler.

        * UIProcess/PageClient.h:
        * UIProcess/API/mac/PageClientImpl.h:
        * UIProcess/API/mac/PageClientImpl.mm:
        (WebKit::PageClientImpl::updateSecureInputState): Separated secure input state
        updating into a separate function. Removed updateTextInputState, we don't need
        to go through PageClient to implement its behavior at all.
        (WebKit::PageClientImpl::dismissDictionaryLookupPanel): Added a FIXME.

        * UIProcess/API/mac/WKView.mm:
        * UIProcess/API/mac/WKViewInternal.h:
        Removed _updateTextInputStateIncludingSecureInputState.

        * UIProcess/WebPageProxy.h: Added m_temporarilyClosedComposition, which helps
        to figure out that WebCore decided to close a composition. The issue is that WebCore
        would first send an EditorState with hasComposition set to false, and with
        shouldIgnoreCompositionSelectionChange set to true, at which time we forget the
        previous m_editorState, but can't make any decisions based on this transient state.
        We should find a way to simplify this (maybe not send these updates with
        shouldIgnoreCompositionSelectionChange at all?)

        * UIProcess/WebPageProxy.cpp:
        (WebKit::WebPageProxy::WebPageProxy): Initialize m_temporarilyClosedComposition.
        (WebKit::WebPageProxy::didCommitLoadForFrame): Removed the code to kill a composition
        when any frame commits a load, which made no sense (along with surrounding code,
        which will unfortunately survive longer).
        (WebKit::WebPageProxy::editorStateChanged): Implemented state updating here,
        we don't need to go to WKView.mm to implement this logic. Figure out when WebCore
        discards a composition, and notify input methods about this.
        (WebKit::WebPageProxy::resetStateAfterProcessExited): Reset m_temporarilyClosedComposition.
        Added some FIXMEs.

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

17 files changed:
LayoutTests/ChangeLog
LayoutTests/platform/mac-wk2/TestExpectations
LayoutTests/platform/mac/editing/input/resources/first-page.html [new file with mode: 0644]
LayoutTests/platform/mac/editing/input/resources/other-page.html [new file with mode: 0644]
LayoutTests/platform/mac/editing/input/unconfirmed-text-navigation-with-page-cache-expected.txt [new file with mode: 0644]
LayoutTests/platform/mac/editing/input/unconfirmed-text-navigation-with-page-cache.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/loader/FrameLoader.cpp
Source/WebCore/loader/FrameLoader.h
Source/WebKit2/ChangeLog
Source/WebKit2/UIProcess/API/mac/PageClientImpl.h
Source/WebKit2/UIProcess/API/mac/PageClientImpl.mm
Source/WebKit2/UIProcess/API/mac/WKView.mm
Source/WebKit2/UIProcess/API/mac/WKViewInternal.h
Source/WebKit2/UIProcess/PageClient.h
Source/WebKit2/UIProcess/WebPageProxy.cpp
Source/WebKit2/UIProcess/WebPageProxy.h

index 76d868e..019ba0c 100644 (file)
@@ -1,3 +1,21 @@
+2013-05-16  Alexey Proskuryakov  <ap@apple.com>
+
+        Text input is largely broken when there are subframes loading
+        http://bugs.webkit.org/show_bug.cgi?id=59121
+        <rdar://problem/9320468>
+
+        Reviewed by Darin Adler.
+
+        The new test's result are affected by DRT and WTR deficiencies, but it still
+        verifies that a tricky crash I had during development doesn't start to happen again.
+
+        * platform/mac-wk2/TestExpectations:
+        * platform/mac/editing/input/resources: Added.
+        * platform/mac/editing/input/resources/first-page.html: Added.
+        * platform/mac/editing/input/resources/other-page.html: Added.
+        * platform/mac/editing/input/unconfirmed-text-navigation-with-page-cache-expected.txt: Added.
+        * platform/mac/editing/input/unconfirmed-text-navigation-with-page-cache.html: Added.
+
 2013-05-17  Ryosuke Niwa  <rniwa@webkit.org>
 
         Mac rebaselines; also remove some entries.
index 940f29b..365d068 100644 (file)
@@ -319,6 +319,9 @@ sputnik/Unicode/Unicode_218/S7.6_A5.2_T1.html
 # Unclear yet why this test fails.
 editing/secure-input/password-input-focusing-to-different-frame.html [ Failure ]
 
+# textInputController.hasMarkedText is implemented, but gives a wrong result for some reason.
+platform/mac/editing/input/unconfirmed-text-navigation-with-page-cache.html
+
 ### END OF (3) Unclassified failures
 ########################################
 
diff --git a/LayoutTests/platform/mac/editing/input/resources/first-page.html b/LayoutTests/platform/mac/editing/input/resources/first-page.html
new file mode 100644 (file)
index 0000000..c482361
--- /dev/null
@@ -0,0 +1,45 @@
+<html>
+<body>
+<div contenteditable></div>
+
+<script>
+// Notify opener.
+window.onpageshow = function() { opener.postMessage('first-page', '*'); }
+
+// Our opener will tell us to perform various loads.
+window.addEventListener('message', function(event) {
+
+    // Navigate to other page.
+    if (event.data === 'navigate-other-page') {
+        window.location = 'other-page.html';
+        return;
+    }
+
+    if (event.data === 'add-unconfirmed-text') {
+        textInputController.setMarkedText("test", 0, 4);
+        return;
+    }
+
+    if (event.data === 'has-marked-text') {
+        var result;
+        // DumpRenderTree does not restore textInputController after navigating back.
+        // FIXME: Fix textInputController to work in restored pages. 
+        try {
+            // Workaround for a DRT vs. WTR difference - one returns a number, and another a boolean.
+            // FIXME: Fix the tools instead of working around the difference.
+            result = textInputController.hasMarkedText() ? "1" : "0";
+        } catch (ex) {
+            result = ex;
+        }
+        opener.postMessage('has-marked-text: ' + result, '*');
+        return;
+    }
+
+}, false);
+
+var input = document.getElementsByTagName("div")[0];
+input.focus();
+</script>
+
+</body>
+</html>
diff --git a/LayoutTests/platform/mac/editing/input/resources/other-page.html b/LayoutTests/platform/mac/editing/input/resources/other-page.html
new file mode 100644 (file)
index 0000000..fdec9d0
--- /dev/null
@@ -0,0 +1,25 @@
+<html>
+<body>
+<script>
+// Notify opener.
+window.onpageshow = function() { opener.postMessage('other-page', '*'); }
+
+// Our opener will tell us to perform various loads.
+window.addEventListener('message', function(event) {
+
+    // Navigate first resource.
+    if (event.data === 'navigate-first-page') {
+        window.location = 'first-page.html';
+        return;
+    }
+
+    // Navigate back.
+    if (event.data === 'navigate-back') {
+        window.history.back();
+        return;
+    }
+
+}, false);
+</script>
+</body>
+</html>
diff --git a/LayoutTests/platform/mac/editing/input/unconfirmed-text-navigation-with-page-cache-expected.txt b/LayoutTests/platform/mac/editing/input/unconfirmed-text-navigation-with-page-cache-expected.txt
new file mode 100644 (file)
index 0000000..fde29b9
--- /dev/null
@@ -0,0 +1,9 @@
+Adding unconfirmed inline text...
+PASS event.data is 'has-marked-text: 1'
+Navigating away from a page with unconfirmed inline input...
+Navigating back to the first page...
+FAIL event.data should be has-marked-text: 0. Was has-marked-text: ReferenceError: Trying to access object from destroyed plug-in..
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/platform/mac/editing/input/unconfirmed-text-navigation-with-page-cache.html b/LayoutTests/platform/mac/editing/input/unconfirmed-text-navigation-with-page-cache.html
new file mode 100644 (file)
index 0000000..ab00a1e
--- /dev/null
@@ -0,0 +1,61 @@
+<html>
+<head>
+<title>Test what happens when navigating from a page with unconfirmed inline input when the page is cacheable</title>
+<script src="../../../../fast/js/resources/js-test-pre.js"></script>
+</head>
+<body>
+<script>
+jsTestIsAsync = true;
+
+if (window.testRunner) {
+    testRunner.setCanOpenWindows();
+    testRunner.overridePreference("WebKitUsesPageCachePreferenceKey", 1);
+}
+
+// Window we will be controlling.
+var target;
+
+// Counter for visits to first page.
+var firstPageVisits = 0;
+
+window.addEventListener('message', function(event) {
+
+    if (event.data === 'first-page') {
+        firstPageVisits++;
+        if (firstPageVisits == 1) {
+            debug("Adding unconfirmed inline text...");
+            target.postMessage('add-unconfirmed-text', '*');
+            target.postMessage('has-marked-text', '*');
+        } else {
+            target.postMessage('has-marked-text', '*');
+        }
+        return;
+    }
+
+    if (event.data.indexOf('has-marked-text: ') == 0) {
+        if (firstPageVisits == 1) {
+            shouldBe("event.data", "'has-marked-text: 1'");
+            debug("Navigating away from a page with unconfirmed inline input...");
+            target.postMessage('navigate-other-page', '*');
+        } else {
+            shouldBe("event.data", "'has-marked-text: 0'");
+            finishJSTest();
+        }
+        return;
+    }
+
+    if (event.data === 'other-page') {
+        debug("Navigating back to the first page...");
+        target.postMessage('navigate-back', '*');
+        return;
+    }
+
+}, false);
+
+// Open the target window and we will start to exchange messages.
+onload = function() { target = window.open('resources/first-page.html'); }
+
+</script>
+<script src="../../../../fast/js/resources/js-test-post.js"></script>
+</body>
+</html>
index aa95954..9ebf7c8 100644 (file)
@@ -1,3 +1,30 @@
+2013-05-17  Alexey Proskuryakov  <ap@apple.com>
+
+        Text input is largely broken when there are subframes loading
+        http://bugs.webkit.org/show_bug.cgi?id=59121
+        <rdar://problem/9320468>
+
+        Reviewed by Darin Adler.
+
+        This addresses text input being abandoned when another frame in a page is navigated.
+
+        There are still many opportunities for improvement:
+        - Track other cases where WebCore interferes may want to cancel input without
+        direct user action (e.g. deleting the whole editable element on a timer).
+        - Fix how dictionary panel and autocorrection are dismissed (they still have the
+        same issue, and get dismissed with any frame navigation).
+
+        Test: platform/mac/editing/input/unconfirmed-text-navigation-with-page-cache.html
+
+        * loader/FrameLoader.h:
+        * loader/FrameLoader.cpp:
+        (WebCore::FrameLoader::willTransitionToCommitted): Make sure that we
+        do not keep an inline session in a frame that's no longer active, as WebKit2 no
+        longer takes care of this case (and more of the logic should be in WebCore anyway).
+        (WebCore::FrameLoader::commitProvisionalLoad): Added a hook that gets invoked right
+        before transitioning to committed state starts. We may want to move more code here
+        eventually, e.g. from Frame::setView.
+
 2013-05-17  Christophe Dumez  <ch.dumez@sisa.samsung.com>
 
         Get rid of [CustomGetter] for global named constructors
index 9172dd8..15e5e34 100644 (file)
@@ -474,6 +474,18 @@ void FrameLoader::stop()
     icon()->stopLoader();
 }
 
+void FrameLoader::willTransitionToCommitted()
+{
+    // This function is called when a frame is still fully in place (not cached, not detached), but will be replaced.
+
+    if (m_frame->editor()->hasComposition()) {
+        // The text was already present in DOM, so it's better to confirm than to cancel the composition.
+        m_frame->editor()->confirmComposition();
+        if (EditorClient* editorClient = m_frame->editor()->client())
+            editorClient->respondToChangedSelection(m_frame);
+    }
+}
+
 bool FrameLoader::closeURL()
 {
     history()->saveDocumentState();
@@ -1681,6 +1693,8 @@ void FrameLoader::commitProvisionalLoad()
         m_frame->document() ? m_frame->document()->url().elidedString().utf8().data() : "",
         pdl ? pdl->url().elidedString().utf8().data() : "<no provisional DocumentLoader>");
 
+    willTransitionToCommitted();
+
     // Check to see if we need to cache the page we are navigating away from into the back/forward cache.
     // We are doing this here because we know for sure that a new page is about to be loaded.
     HistoryItem* item = history()->currentItem();
index deeaf0d..1ca0696 100644 (file)
@@ -367,6 +367,7 @@ private:
     void prepareForLoadStart();
     void provisionalLoadStarted();
 
+    void willTransitionToCommitted();
     bool didOpenURL();
 
     void scheduleCheckCompleted();
index c930af4..fc22bb9 100644 (file)
@@ -1,3 +1,42 @@
+2013-05-16  Alexey Proskuryakov  <ap@apple.com>
+
+        Text input is largely broken when there are subframes loading
+        http://bugs.webkit.org/show_bug.cgi?id=59121
+        <rdar://problem/9320468>
+
+        Reviewed by Darin Adler.
+
+        * UIProcess/PageClient.h:
+        * UIProcess/API/mac/PageClientImpl.h:
+        * UIProcess/API/mac/PageClientImpl.mm:
+        (WebKit::PageClientImpl::updateSecureInputState): Separated secure input state
+        updating into a separate function. Removed updateTextInputState, we don't need
+        to go through PageClient to implement its behavior at all.
+        (WebKit::PageClientImpl::dismissDictionaryLookupPanel): Added a FIXME.
+
+        * UIProcess/API/mac/WKView.mm:
+        * UIProcess/API/mac/WKViewInternal.h:
+        Removed _updateTextInputStateIncludingSecureInputState.
+
+        * UIProcess/WebPageProxy.h: Added m_temporarilyClosedComposition, which helps
+        to figure out that WebCore decided to close a composition. The issue is that WebCore
+        would first send an EditorState with hasComposition set to false, and with
+        shouldIgnoreCompositionSelectionChange set to true, at which time we forget the
+        previous m_editorState, but can't make any decisions based on this transient state.
+        We should find a way to simplify this (maybe not send these updates with
+        shouldIgnoreCompositionSelectionChange at all?)
+
+        * UIProcess/WebPageProxy.cpp:
+        (WebKit::WebPageProxy::WebPageProxy): Initialize m_temporarilyClosedComposition.
+        (WebKit::WebPageProxy::didCommitLoadForFrame): Removed the code to kill a composition
+        when any frame commits a load, which made no sense (along with surrounding code,
+        which will unfortunately survive longer).
+        (WebKit::WebPageProxy::editorStateChanged): Implemented state updating here,
+        we don't need to go to WKView.mm to implement this logic. Figure out when WebCore
+        discards a composition, and notify input methods about this.
+        (WebKit::WebPageProxy::resetStateAfterProcessExited): Reset m_temporarilyClosedComposition.
+        Added some FIXMEs.
+
 2013-05-17  Manuel Rego Casasnovas  <rego@igalia.com>
 
         [WK2] Add support for selectTrailingWhitespaceEnabled setting
index b78d274..518ed2c 100644 (file)
@@ -82,7 +82,7 @@ private:
     virtual void setDragImage(const WebCore::IntPoint& clientPosition, PassRefPtr<ShareableBitmap> dragImage, bool isLinkDrag);
     virtual void setPromisedData(const String& pasteboardName, PassRefPtr<WebCore::SharedBuffer> imageBuffer, const String& filename, const String& extension, const String& title,
                                  const String& url, const String& visibleUrl, PassRefPtr<WebCore::SharedBuffer> archiveBuffer);
-    virtual void updateTextInputState(bool updateSecureInputState);
+    virtual void updateSecureInputState() OVERRIDE;
     virtual void resetSecureInputState() OVERRIDE;
     virtual void notifyInputContextAboutDiscardedComposition() OVERRIDE;
 
index 28bf821..64dbe8c 100644 (file)
@@ -326,9 +326,9 @@ void PageClientImpl::setPromisedData(const String& pasteboardName, PassRefPtr<Sh
     [m_wkView _setPromisedData:image.get() withFileName:filename withExtension:extension withTitle:title withURL:url withVisibleURL:visibleUrl withArchive:archiveBuffer.get() forPasteboard:pasteboardName];
 }
 
-void PageClientImpl::updateTextInputState(bool updateSecureInputState)
+void PageClientImpl::updateSecureInputState()
 {
-    [m_wkView _updateTextInputStateIncludingSecureInputState:updateSecureInputState];
+    [m_wkView _updateSecureInputState];
 }
 
 void PageClientImpl::resetSecureInputState()
@@ -502,6 +502,7 @@ void PageClientImpl::didPerformDictionaryLookup(const AttributedString& text, co
 
 void PageClientImpl::dismissDictionaryLookupPanel()
 {
+    // FIXME: We don't know which panel we are dismissing, it may not even be in the current page (see <rdar://problem/13875766>).
     WKHideWordDefinitionWindow();
 }
 
index a9fa904..c76e659 100644 (file)
@@ -2900,23 +2900,6 @@ static NSString *pathWithUniqueFilenameForPath(NSString *path)
     _data->_inSecureInputState = isInPasswordField;
 }
 
-- (void)_updateTextInputStateIncludingSecureInputState:(BOOL)updateSecureInputState
-{
-    const EditorState& editorState = _data->_page->editorState();
-    if (updateSecureInputState) {
-        // This is a temporary state when editing. Flipping secure input state too quickly can expose race conditions.
-        if (!editorState.selectionIsNone)
-            [self _updateSecureInputState];
-    }
-
-    if (!editorState.hasComposition || editorState.shouldIgnoreCompositionSelectionChange)
-        return;
-
-    _data->_page->cancelComposition();
-
-    [self _notifyInputContextAboutDiscardedComposition];
-}
-
 - (void)_resetSecureInputState
 {
     if (_data->_inSecureInputState) {
index b3d986e..75f074f 100644 (file)
@@ -84,7 +84,6 @@ namespace WebKit {
 - (void)_updateSecureInputState;
 - (void)_resetSecureInputState;
 - (void)_notifyInputContextAboutDiscardedComposition;
-- (void)_updateTextInputStateIncludingSecureInputState:(BOOL)updateSecureInputState;
 
 - (WebKit::ColorSpaceData)_colorSpace;
 
index 52a3d30..9d6235a 100644 (file)
@@ -156,7 +156,7 @@ public:
     virtual bool interpretKeyEvent(const NativeWebKeyboardEvent&, Vector<WebCore::KeypressCommand>&) = 0;
     virtual bool executeSavedCommandBySelector(const String& selector) = 0;
     virtual void setDragImage(const WebCore::IntPoint& clientPosition, PassRefPtr<ShareableBitmap> dragImage, bool isLinkDrag) = 0;
-    virtual void updateTextInputState(bool updateSecureInputState) = 0;
+    virtual void updateSecureInputState() = 0;
     virtual void resetSecureInputState() = 0;
     virtual void notifyInputContextAboutDiscardedComposition() = 0;
     virtual void makeFirstResponder() = 0;
index 925206c..97fac3f 100644 (file)
@@ -245,6 +245,7 @@ WebPageProxy::WebPageProxy(PageClient* pageClient, PassRefPtr<WebProcessProxy> p
     , m_isVisible(m_pageClient->isViewVisible())
     , m_backForwardList(WebBackForwardList::create(this))
     , m_loadStateAtProcessExit(WebFrameProxy::LoadStateFinished)
+    , m_temporarilyClosedComposition(false)
     , m_textZoomFactor(1)
     , m_pageZoomFactor(1)
     , m_pageScaleFactor(1)
@@ -2260,8 +2261,7 @@ void WebPageProxy::didCommitLoadForFrame(uint64_t frameID, const String& mimeTyp
 
 #if PLATFORM(MAC)
     // FIXME (bug 59111): didCommitLoadForFrame comes too late when restoring a page from b/f cache, making us disable secure event mode in password fields.
-    // FIXME (bug 59121): A load going on in one frame shouldn't affect typing in sibling frames.
-    m_pageClient->notifyInputContextAboutDiscardedComposition();
+    // FIXME: A load going on in one frame shouldn't affect text editing in other frames on the page.
     m_pageClient->resetSecureInputState();
     dismissCorrectionPanel(ReasonForDismissingAlternativeTextIgnored);
     m_pageClient->dismissDictionaryLookupPanel();
@@ -3069,12 +3069,28 @@ void WebPageProxy::editorStateChanged(const EditorState& editorState)
 {
 #if PLATFORM(MAC)
     bool couldChangeSecureInputState = m_editorState.isInPasswordField != editorState.isInPasswordField || m_editorState.selectionIsNone;
+    bool closedComposition = !editorState.shouldIgnoreCompositionSelectionChange && !editorState.hasComposition && (m_editorState.hasComposition || m_temporarilyClosedComposition);
+    m_temporarilyClosedComposition = editorState.shouldIgnoreCompositionSelectionChange && (m_temporarilyClosedComposition || m_editorState.hasComposition) && !editorState.hasComposition;
 #endif
 
     m_editorState = editorState;
 
 #if PLATFORM(MAC)
-    m_pageClient->updateTextInputState(couldChangeSecureInputState);
+    // Selection being none is a temporary state when editing. Flipping secure input state too quickly was causing trouble (not fully understood).
+    if (couldChangeSecureInputState && !editorState.selectionIsNone)
+        m_pageClient->updateSecureInputState();
+
+    if (editorState.shouldIgnoreCompositionSelectionChange)
+        return;
+
+    if (closedComposition)
+        m_pageClient->notifyInputContextAboutDiscardedComposition();
+    if (editorState.hasComposition) {
+        // Abandon the current inline input session if selection changed for any other reason but an input method changing the composition.
+        // FIXME: This logic should be in WebCore, no need to round-trip to UI process to cancel the composition.
+        cancelComposition();
+        m_pageClient->notifyInputContextAboutDiscardedComposition();
+    }
 #elif PLATFORM(QT) || PLATFORM(EFL) || PLATFORM(GTK)
     m_pageClient->updateTextInputState();
 #endif
@@ -3916,6 +3932,10 @@ void WebPageProxy::resetStateAfterProcessExited()
     m_touchEventQueue.clear();
 #endif
 
+    // FIXME: Reset m_editorState.
+    // FIXME: Notify input methods about abandoned composition.
+    m_temporarilyClosedComposition = false;
+
 #if PLATFORM(MAC)
     dismissCorrectionPanel(ReasonForDismissingAlternativeTextIgnored);
     m_pageClient->dismissDictionaryLookupPanel();
index 576a85e..80b20bb 100644 (file)
@@ -1131,6 +1131,7 @@ private:
     WebFrameProxy::LoadState m_loadStateAtProcessExit;
 
     EditorState m_editorState;
+    bool m_temporarilyClosedComposition; // Editor state changed from hasComposition to !hasComposition, but that was only with shouldIgnoreCompositionSelectionChange yet.
 
     double m_textZoomFactor;
     double m_pageZoomFactor;