Bug 59121 – WebKit2: Text input is largely broken when there are subframes loading

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Alexey Proskuryakov]

We reset text input state on frame load, failing to recognize that there is only one WKView per page, but multiple frames.

Steps to reproduce:
1. Open attach test case.
2. Try to type and edit text using any non-trivial features (inline input, dictionary pop-up, possibly accessibility).

Observe that it's obviously and mightily broken.

[Alexey Proskuryakov]

<rdar://problem/9320468>

[Alexey Proskuryakov]

Created attachment 90614 [details]
test case (open from a local file)

[Alexey Proskuryakov]

*** Bug 67105 has been marked as a duplicate of this bug. ***

[Alexey Proskuryakov]

Created attachment 202008 [details]
proposed fix

[Brady Eidson]

Comment on attachment 202008 [details]
proposed fix

View in context: https://bugs.webkit.org/attachment.cgi?id=202008&action=review

> Source/WebCore/ChangeLog:20
> +        Added a hook taht gets invoked right before transitioning to committed state starts.

s/taht/that/

> LayoutTests/platform/mac-wk2/TestExpectations:321
> +# textInputController.hasMarkedText is implemented, but gives a wrong result for some reason.
> +platform/mac/editing/input/unconfirmed-text-navigation-with-page-cache.html

Kind of troubling that the test doesn't cover the port where the most changes took place.  =/

[Alexey Proskuryakov]

Comment on attachment 202008 [details]
proposed fix

View in context: https://bugs.webkit.org/attachment.cgi?id=202008&action=review

>> LayoutTests/platform/mac-wk2/TestExpectations:321
>> +platform/mac/editing/input/unconfirmed-text-navigation-with-page-cache.html
> 
> Kind of troubling that the test doesn't cover the port where the most changes took place.  =/

Currently, many text input tests are failing or skipped because or WTR deficiencies, and there are many aspects of text input that can't be tested on any platform. It's unfortunate that text input test coverage is so spotty, but I guess it's better than nothing.

In WebKit2, driving tests from WebProcess makes it particularly tricky to test anything that deals with user actions.

[Darin Adler]

Comment on attachment 202008 [details]
proposed fix

View in context: https://bugs.webkit.org/attachment.cgi?id=202008&action=review

review+ as long as you do not add a member function to Frame!

> Source/WebCore/page/Frame.h:110
> +        void willTransitionToCommitted();

We should not add a function to Frame for this. The code should be in FrameLoader. It’s completely reasonable for FrameLoader to make Editor calls. You can put this new function in either FrameLoader or Editor, but not in Frame.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:3070
> +    // This is a temporary state when editing. Flipping secure input state too quickly can expose race conditions.
> +    if (couldChangeSecureInputState && !editorState.selectionIsNone)
> +        m_pageClient->updateSecureInputState();

I don’t understand the comment. It doesn’t say what it’s referring to. What is a temporary state when editing? What does “too quickly” mean? It seems like you moved this comment here, but I don’t understand it in its original location.

[Alexey Proskuryakov]

Committed <http://trac.webkit.org/r150291>.

[Alexey Proskuryakov]

Build fix in <http://trac.webkit.org/r150293>.