Changeset 150291 in webkit

Timestamp:

May 17, 2013 1:04:43 PM (11 years ago)

Author:

ap@apple.com

Message:

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.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/platform/mac-wk2/TestExpectations (modified) (1 diff)
• LayoutTests/platform/mac/editing/input/resources (added)
• LayoutTests/platform/mac/editing/input/resources/first-page.html (added)
• LayoutTests/platform/mac/editing/input/resources/other-page.html (added)
• LayoutTests/platform/mac/editing/input/unconfirmed-text-navigation-with-page-cache-expected.txt (added)
• LayoutTests/platform/mac/editing/input/unconfirmed-text-navigation-with-page-cache.html (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/loader/FrameLoader.cpp (modified) (2 diffs)
• Source/WebCore/loader/FrameLoader.h (modified) (1 diff)
• Source/WebKit2/ChangeLog (modified) (1 diff)
• Source/WebKit2/UIProcess/API/mac/PageClientImpl.h (modified) (1 diff)
• Source/WebKit2/UIProcess/API/mac/PageClientImpl.mm (modified) (2 diffs)
• Source/WebKit2/UIProcess/API/mac/WKView.mm (modified) (1 diff)
• Source/WebKit2/UIProcess/API/mac/WKViewInternal.h (modified) (1 diff)
• Source/WebKit2/UIProcess/PageClient.h (modified) (1 diff)
• Source/WebKit2/UIProcess/WebPageProxy.cpp (modified) (5 diffs)
• Source/WebKit2/UIProcess/WebPageProxy.h (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+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>
 

trunk/LayoutTests/platform/mac-wk2/TestExpectations

@@ -320 +320 @@
 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
 ########################################

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WebCore/loader/FrameLoader.cpp

@@ -475 +475 @@
 }
 
+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()
 {
@@ -1681 +1693 @@
         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.

trunk/Source/WebCore/loader/FrameLoader.h

@@ -368 +368 @@
     void provisionalLoadStarted();
 
+    void willTransitionToCommitted();
     bool didOpenURL();
 

trunk/Source/WebKit2/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WebKit2/UIProcess/API/mac/PageClientImpl.h

@@ -83 +83 @@
     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;

trunk/Source/WebKit2/UIProcess/API/mac/PageClientImpl.mm

@@ -327 +327 @@
 }
 
-void PageClientImpl::updateTextInputState(bool updateSecureInputState)
-{
-    [m_wkView _updateTextInputStateIncludingSecureInputState:updateSecureInputState];
+void PageClientImpl::updateSecureInputState()
+{
+    [m_wkView _updateSecureInputState];
 }
 
@@ -503 +503 @@
 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();
 }

trunk/Source/WebKit2/UIProcess/API/mac/WKView.mm

@@ -2901 +2901 @@
 }
 
-- (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
 {

trunk/Source/WebKit2/UIProcess/API/mac/WKViewInternal.h

@@ -85 +85 @@
 - (void)_resetSecureInputState;
 - (void)_notifyInputContextAboutDiscardedComposition;
-- (void)_updateTextInputStateIncludingSecureInputState:(BOOL)updateSecureInputState;
 
 - (WebKit::ColorSpaceData)_colorSpace;

trunk/Source/WebKit2/UIProcess/PageClient.h

@@ -157 +157 @@
     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;

trunk/Source/WebKit2/UIProcess/WebPageProxy.cpp

@@ -246 +246 @@
     , m_backForwardList(WebBackForwardList::create(this))
     , m_loadStateAtProcessExit(WebFrameProxy::LoadStateFinished)
+    , m_temporarilyClosedComposition(false)
     , m_textZoomFactor(1)
     , m_pageZoomFactor(1)
@@ -2261 +2262 @@
 #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);
@@ -3070 +3070 @@
 #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
 
@@ -3075 +3077 @@
 
 #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();
@@ -3916 +3932 @@
     m_touchEventQueue.clear();
 #endif
+
+    // FIXME: Reset m_editorState.
+    // FIXME: Notify input methods about abandoned composition.
+    m_temporarilyClosedComposition = false;
 
 #if PLATFORM(MAC)

trunk/Source/WebKit2/UIProcess/WebPageProxy.h

@@ -1132 +1132 @@
 
     EditorState m_editorState;
+    bool m_temporarilyClosedComposition; // Editor state changed from hasComposition to !hasComposition, but that was only with shouldIgnoreCompositionSelectionChange yet.
 
     double m_textZoomFactor;