Changeset 150291 in webkit


Ignore:
Timestamp:
May 17, 2013, 1:04:43 PM (12 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:
5 added
13 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r150290 r150291  
     12013-05-16  Alexey Proskuryakov  <ap@apple.com>
     2
     3        Text input is largely broken when there are subframes loading
     4        http://bugs.webkit.org/show_bug.cgi?id=59121
     5        <rdar://problem/9320468>
     6
     7        Reviewed by Darin Adler.
     8
     9        The new test's result are affected by DRT and WTR deficiencies, but it still
     10        verifies that a tricky crash I had during development doesn't start to happen again.
     11
     12        * platform/mac-wk2/TestExpectations:
     13        * platform/mac/editing/input/resources: Added.
     14        * platform/mac/editing/input/resources/first-page.html: Added.
     15        * platform/mac/editing/input/resources/other-page.html: Added.
     16        * platform/mac/editing/input/unconfirmed-text-navigation-with-page-cache-expected.txt: Added.
     17        * platform/mac/editing/input/unconfirmed-text-navigation-with-page-cache.html: Added.
     18
    1192013-05-17  Ryosuke Niwa  <rniwa@webkit.org>
    220

  • trunk/LayoutTests/platform/mac-wk2/TestExpectations

    r150235 r150291  
    320320editing/secure-input/password-input-focusing-to-different-frame.html [ Failure ]
    321321
     322# textInputController.hasMarkedText is implemented, but gives a wrong result for some reason.
     323platform/mac/editing/input/unconfirmed-text-navigation-with-page-cache.html
     324
    322325### END OF (3) Unclassified failures
    323326########################################

  • trunk/Source/WebCore/ChangeLog

    r150283 r150291  
     12013-05-17  Alexey Proskuryakov  <ap@apple.com>
     2
     3        Text input is largely broken when there are subframes loading
     4        http://bugs.webkit.org/show_bug.cgi?id=59121
     5        <rdar://problem/9320468>
     6
     7        Reviewed by Darin Adler.
     8
     9        This addresses text input being abandoned when another frame in a page is navigated.
     10
     11        There are still many opportunities for improvement:
     12        - Track other cases where WebCore interferes may want to cancel input without
     13        direct user action (e.g. deleting the whole editable element on a timer).
     14        - Fix how dictionary panel and autocorrection are dismissed (they still have the
     15        same issue, and get dismissed with any frame navigation).
     16
     17        Test: platform/mac/editing/input/unconfirmed-text-navigation-with-page-cache.html
     18
     19        * loader/FrameLoader.h:
     20        * loader/FrameLoader.cpp:
     21        (WebCore::FrameLoader::willTransitionToCommitted): Make sure that we
     22        do not keep an inline session in a frame that's no longer active, as WebKit2 no
     23        longer takes care of this case (and more of the logic should be in WebCore anyway).
     24        (WebCore::FrameLoader::commitProvisionalLoad): Added a hook that gets invoked right
     25        before transitioning to committed state starts. We may want to move more code here
     26        eventually, e.g. from Frame::setView.
     27
    1282013-05-17  Christophe Dumez  <ch.dumez@sisa.samsung.com>
    229

  • trunk/Source/WebCore/loader/FrameLoader.cpp

    r150214 r150291  
    475475}
    476476
     477void FrameLoader::willTransitionToCommitted()
     478{
     479    // This function is called when a frame is still fully in place (not cached, not detached), but will be replaced.
     480
     481    if (m_frame->editor()->hasComposition()) {
     482        // The text was already present in DOM, so it's better to confirm than to cancel the composition.
     483        m_frame->editor()->confirmComposition();
     484        if (EditorClient* editorClient = m_frame->editor()->client())
     485            editorClient->respondToChangedSelection(m_frame);
     486    }
     487}
     488
    477489bool FrameLoader::closeURL()
    478490{
    … …  
    16811693        m_frame->document() ? m_frame->document()->url().elidedString().utf8().data() : "",
    16821694        pdl ? pdl->url().elidedString().utf8().data() : "<no provisional DocumentLoader>");
     1695
     1696    willTransitionToCommitted();
    16831697
    16841698    // 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

    r150214 r150291  
    368368    void provisionalLoadStarted();
    369369
     370    void willTransitionToCommitted();
    370371    bool didOpenURL();
    371372

  • trunk/Source/WebKit2/ChangeLog

    r150289 r150291  
     12013-05-16  Alexey Proskuryakov  <ap@apple.com>
     2
     3        Text input is largely broken when there are subframes loading
     4        http://bugs.webkit.org/show_bug.cgi?id=59121
     5        <rdar://problem/9320468>
     6
     7        Reviewed by Darin Adler.
     8
     9        * UIProcess/PageClient.h:
     10        * UIProcess/API/mac/PageClientImpl.h:
     11        * UIProcess/API/mac/PageClientImpl.mm:
     12        (WebKit::PageClientImpl::updateSecureInputState): Separated secure input state
     13        updating into a separate function. Removed updateTextInputState, we don't need
     14        to go through PageClient to implement its behavior at all.
     15        (WebKit::PageClientImpl::dismissDictionaryLookupPanel): Added a FIXME.
     16
     17        * UIProcess/API/mac/WKView.mm:
     18        * UIProcess/API/mac/WKViewInternal.h:
     19        Removed _updateTextInputStateIncludingSecureInputState.
     20
     21        * UIProcess/WebPageProxy.h: Added m_temporarilyClosedComposition, which helps
     22        to figure out that WebCore decided to close a composition. The issue is that WebCore
     23        would first send an EditorState with hasComposition set to false, and with
     24        shouldIgnoreCompositionSelectionChange set to true, at which time we forget the
     25        previous m_editorState, but can't make any decisions based on this transient state.
     26        We should find a way to simplify this (maybe not send these updates with
     27        shouldIgnoreCompositionSelectionChange at all?)
     28
     29        * UIProcess/WebPageProxy.cpp:
     30        (WebKit::WebPageProxy::WebPageProxy): Initialize m_temporarilyClosedComposition.
     31        (WebKit::WebPageProxy::didCommitLoadForFrame): Removed the code to kill a composition
     32        when any frame commits a load, which made no sense (along with surrounding code,
     33        which will unfortunately survive longer).
     34        (WebKit::WebPageProxy::editorStateChanged): Implemented state updating here,
     35        we don't need to go to WKView.mm to implement this logic. Figure out when WebCore
     36        discards a composition, and notify input methods about this.
     37        (WebKit::WebPageProxy::resetStateAfterProcessExited): Reset m_temporarilyClosedComposition.
     38        Added some FIXMEs.
     39
    1402013-05-17  Manuel Rego Casasnovas  <rego@igalia.com>
    241

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

    r150132 r150291  
    8383    virtual void setPromisedData(const String& pasteboardName, PassRefPtr<WebCore::SharedBuffer> imageBuffer, const String& filename, const String& extension, const String& title,
    8484                                 const String& url, const String& visibleUrl, PassRefPtr<WebCore::SharedBuffer> archiveBuffer);
    85     virtual void updateTextInputState(bool updateSecureInputState);
     85    virtual void updateSecureInputState() OVERRIDE;
    8686    virtual void resetSecureInputState() OVERRIDE;
    8787    virtual void notifyInputContextAboutDiscardedComposition() OVERRIDE;

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

    r150230 r150291  
    327327}
    328328
    329 void PageClientImpl::updateTextInputState(bool updateSecureInputState)
    330 {
    331     [m_wkView _updateTextInputStateIncludingSecureInputState:updateSecureInputState];
     329void PageClientImpl::updateSecureInputState()
     330{
     331    [m_wkView _updateSecureInputState];
    332332}
    333333
    … …  
    503503void PageClientImpl::dismissDictionaryLookupPanel()
    504504{
     505    // FIXME: We don't know which panel we are dismissing, it may not even be in the current page (see <rdar://problem/13875766>).
    505506    WKHideWordDefinitionWindow();
    506507}

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

    r150132 r150291  
    29012901}
    29022902
    2903 - (void)_updateTextInputStateIncludingSecureInputState:(BOOL)updateSecureInputState
    2904 {
    2905     const EditorState& editorState = _data->_page->editorState();
    2906     if (updateSecureInputState) {
    2907         // This is a temporary state when editing. Flipping secure input state too quickly can expose race conditions.
    2908         if (!editorState.selectionIsNone)
    2909             [self _updateSecureInputState];
    2910     }
    2911 
    2912     if (!editorState.hasComposition || editorState.shouldIgnoreCompositionSelectionChange)
    2913         return;
    2914 
    2915     _data->_page->cancelComposition();
    2916 
    2917     [self _notifyInputContextAboutDiscardedComposition];
    2918 }
    2919 
    29202903- (void)_resetSecureInputState
    29212904{

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

    r150132 r150291  
    8585- (void)_resetSecureInputState;
    8686- (void)_notifyInputContextAboutDiscardedComposition;
    87 - (void)_updateTextInputStateIncludingSecureInputState:(BOOL)updateSecureInputState;
    8887
    8988- (WebKit::ColorSpaceData)_colorSpace;

  • trunk/Source/WebKit2/UIProcess/PageClient.h

    r150132 r150291  
    157157    virtual bool executeSavedCommandBySelector(const String& selector) = 0;
    158158    virtual void setDragImage(const WebCore::IntPoint& clientPosition, PassRefPtr<ShareableBitmap> dragImage, bool isLinkDrag) = 0;
    159     virtual void updateTextInputState(bool updateSecureInputState) = 0;
     159    virtual void updateSecureInputState() = 0;
    160160    virtual void resetSecureInputState() = 0;
    161161    virtual void notifyInputContextAboutDiscardedComposition() = 0;

  • trunk/Source/WebKit2/UIProcess/WebPageProxy.cpp

    r150271 r150291  
    246246    , m_backForwardList(WebBackForwardList::create(this))
    247247    , m_loadStateAtProcessExit(WebFrameProxy::LoadStateFinished)
     248    , m_temporarilyClosedComposition(false)
    248249    , m_textZoomFactor(1)
    249250    , m_pageZoomFactor(1)
    … …  
    22612262#if PLATFORM(MAC)
    22622263    // FIXME (bug 59111): didCommitLoadForFrame comes too late when restoring a page from b/f cache, making us disable secure event mode in password fields.
    2263     // FIXME (bug 59121): A load going on in one frame shouldn't affect typing in sibling frames.
    2264     m_pageClient->notifyInputContextAboutDiscardedComposition();
     2264    // FIXME: A load going on in one frame shouldn't affect text editing in other frames on the page.
    22652265    m_pageClient->resetSecureInputState();
    22662266    dismissCorrectionPanel(ReasonForDismissingAlternativeTextIgnored);
    … …  
    30703070#if PLATFORM(MAC)
    30713071    bool couldChangeSecureInputState = m_editorState.isInPasswordField != editorState.isInPasswordField || m_editorState.selectionIsNone;
     3072    bool closedComposition = !editorState.shouldIgnoreCompositionSelectionChange && !editorState.hasComposition && (m_editorState.hasComposition || m_temporarilyClosedComposition);
     3073    m_temporarilyClosedComposition = editorState.shouldIgnoreCompositionSelectionChange && (m_temporarilyClosedComposition || m_editorState.hasComposition) && !editorState.hasComposition;
    30723074#endif
    30733075
    … …  
    30753077
    30763078#if PLATFORM(MAC)
    3077     m_pageClient->updateTextInputState(couldChangeSecureInputState);
     3079    // Selection being none is a temporary state when editing. Flipping secure input state too quickly was causing trouble (not fully understood).
     3080    if (couldChangeSecureInputState && !editorState.selectionIsNone)
     3081        m_pageClient->updateSecureInputState();
     3082
     3083    if (editorState.shouldIgnoreCompositionSelectionChange)
     3084        return;
     3085
     3086    if (closedComposition)
     3087        m_pageClient->notifyInputContextAboutDiscardedComposition();
     3088    if (editorState.hasComposition) {
     3089        // Abandon the current inline input session if selection changed for any other reason but an input method changing the composition.
     3090        // FIXME: This logic should be in WebCore, no need to round-trip to UI process to cancel the composition.
     3091        cancelComposition();
     3092        m_pageClient->notifyInputContextAboutDiscardedComposition();
     3093    }
    30783094#elif PLATFORM(QT) || PLATFORM(EFL) || PLATFORM(GTK)
    30793095    m_pageClient->updateTextInputState();
    … …  
    39163932    m_touchEventQueue.clear();
    39173933#endif
     3934
     3935    // FIXME: Reset m_editorState.
     3936    // FIXME: Notify input methods about abandoned composition.
     3937    m_temporarilyClosedComposition = false;
    39183938
    39193939#if PLATFORM(MAC)

  • trunk/Source/WebKit2/UIProcess/WebPageProxy.h

    r150271 r150291  
    11321132
    11331133    EditorState m_editorState;
     1134    bool m_temporarilyClosedComposition; // Editor state changed from hasComposition to !hasComposition, but that was only with shouldIgnoreCompositionSelectionChange yet.
    11341135
    11351136    double m_textZoomFactor;
Note: See TracChangeset for help on using the changeset viewer.