Bug 59121 - WebKit2: Text input is largely broken when there are subframes loading
Summary: WebKit2: Text input is largely broken when there are subframes loading
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac (Intel) OS X 10.6
: P2 Normal
Assignee: Alexey Proskuryakov
URL:
Keywords: InRadar
: 67105 (view as bug list)
Depends on:
Blocks:
 
Reported: 2011-04-21 11:18 PDT by Alexey Proskuryakov
Modified: 2013-05-17 13:17 PDT (History)
6 users (show)

See Also:


Attachments
test case (open from a local file) (302 bytes, text/html)
2011-04-21 15:24 PDT, Alexey Proskuryakov
no flags Details
proposed fix (23.17 KB, patch)
2013-05-16 18:16 PDT, Alexey Proskuryakov
darin: review+
darin: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Proskuryakov 2011-04-21 11:18:07 PDT
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.
Comment 1 Alexey Proskuryakov 2011-04-21 15:22:32 PDT
<rdar://problem/9320468>
Comment 2 Alexey Proskuryakov 2011-04-21 15:24:42 PDT
Created attachment 90614 [details]
test case (open from a local file)
Comment 3 Alexey Proskuryakov 2011-08-29 00:36:32 PDT
*** Bug 67105 has been marked as a duplicate of this bug. ***
Comment 4 Alexey Proskuryakov 2013-05-16 18:16:57 PDT
Created attachment 202008 [details]
proposed fix
Comment 5 Brady Eidson 2013-05-17 09:20:45 PDT
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.  =/
Comment 6 Alexey Proskuryakov 2013-05-17 09:45:35 PDT
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.
Comment 7 Darin Adler 2013-05-17 10:16:11 PDT
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.
Comment 8 Alexey Proskuryakov 2013-05-17 13:04:32 PDT
Committed <http://trac.webkit.org/r150291>.
Comment 9 Alexey Proskuryakov 2013-05-17 13:17:46 PDT
Build fix in <http://trac.webkit.org/r150293>.