Bug 107737 – Editor::m_compositionNode not updated on HTMLInputElement::setValue()

• Component: HTML Editing
• Status: RESOLVED FIXED

[Aurimas Liutikas]

Created attachment 184314 [details]
Screenshot of www.google.com

m_compositionNode is not being updated on JavaScript changes to the focused input text field. It has to be explicitly updated using Editor::confirmCompositon(). In Chrome most of the platforms send confirmComposition on mouse clicks which does not really a good idea and it should probably get called automatically on HTMLInputElement::setValue() calls.

On Chrome for Android we were not calling confirmComposition and then cause the Editor::setComposition() do behave weirdly as it was not able to change the node value anymore returning false. It can be checked using Chrome Beta M25 on mobile version of google.com when clicking on the autocomplete suggestion with a little arrow (see screenshot) when there still is a non-empty composition.

[Aurimas Liutikas]

The user case:
1. User goes to google.com on their mobile phone.
2. User starts typing a word in an input field, for example "Hello"
3. User selects a suggestion from the page (for example "Hello Kitty") by tapping on a little arrow that uses JavaScript to replace the input field value.
4. User tries to type some more words (composing).

Expectation:
User can continue typing at step 4.

What happens:
User cannot input any new text at step 4.

[Aurimas Liutikas]

What I observed is that if Editor::confirmComposition() is called before the JavaScript value change, then the user can continue typing after the value has changed. I am guessing confirmComposition resets the Editor state correctly.

Should the WebWidget implementor be responsible for calling confirmComposition before JavaScript changes to the input field values?

[Aurimas Liutikas]

I found a way to reproduce this bug (I think the root cause is the same that the editor does not get updated on javascript changes) on Chrome GTK and ChromeOS (might be affecting other platforms too but I do not have a device to test).

User steps:
1. Navigate to http://jsfiddle.net/eEpQz/2/
2. Within 10 seconds type in something in the input field using IME (I used ibus for Linux and built-in IME on ChromeOS). You must have a composition (underline in Chrome). 
3. Wait for the JavaScript timer to expire.

Observation:
The composition (the underline) will be set to the number of characters the you typed in the field.
For example, if I typed '啊' before the replacement, it will become 'Nonononono' with the first letter 'N' as the composition.

Expectation:
The composition should get reset when JS changes the value of the focused field.

[Ryosuke Niwa]

I don't observe the said behavior on Mac port. I'm pretty certain this is a problem in Chromium's IME code.

[Ryosuke Niwa]

I can reproduce this problem on Chrome (Mac).

[Aurimas Liutikas]

Does Mac port call commitComposition or cancelComposition before JavaScript changes?

[Levi Weintraub]

Here's a backtrace where the JS change blows away the composition on Mac.

#0  WebCore::Editor::cancelComposition (this=0x115096770) at /Volumes/Workspace/webkit/Source/WebCore/editing/Editor.cpp:1333
#1  0x0000000101926834 in -[WebHTMLView(WebNSTextInputSupport) _updateSelectionForInputManager] (self=0x120ebc0a0, _cmd=0x7fff897b3d8c) at /Volumes/Workspace/webkit/Source/WebKit/mac/WebView/WebHTMLView.mm:6051
#2  0x000000010192192b in -[WebHTMLView(WebInternal) _selectionChanged] (self=0x120ebc0a0, _cmd=0x7fff897ad14a) at /Volumes/Workspace/webkit/Source/WebKit/mac/WebView/WebHTMLView.mm:5050
#3  0x00000001018b7aa7 in WebEditorClient::respondToChangedSelection (this=0x109f7f990, frame=0x115096200) at /Volumes/Workspace/webkit/Source/WebKit/mac/WebCoreSupport/WebEditorClient.mm:295
#4  0x000000010395c247 in WebCore::Editor::notifyComponentsOnChangedSelection (this=0x115096770, oldSelection=@0x7fff5fbfd388, options=6) at /Volumes/Workspace/webkit/Source/WebCore/editing/Editor.cpp:485
#5  0x0000000103969a05 in WebCore::Editor::respondToChangedSelection (this=0x115096770, oldSelection=@0x7fff5fbfd388, options=6) at /Volumes/Workspace/webkit/Source/WebCore/editing/Editor.cpp:2879
#6  0x0000000103ab0ffd in WebCore::FrameSelection::setSelection (this=0x115096838, newSelection=@0x7fff5fbfd5a0, options=6, align=WebCore::FrameSelection::AlignCursorOnScrollIfNeeded, granularity=WebCore::CharacterGranularity) at /Volumes/Workspace/webkit/Source/WebCore/editing/FrameSelection.cpp:313
#7  0x0000000103d0dcd0 in WebCore::HTMLTextFormControlElement::setSelectionRange (this=0x120ef0580, start=14, end=14, direction=WebCore::SelectionHasNoDirection) at /Volumes/Workspace/webkit/Source/WebCore/html/HTMLTextFormControlElement.cpp:344
#8  0x0000000104d4e08d in WebCore::TextFieldInputType::setValue (this=0x11d0009f0, sanitizedValue=@0x7fff5fbfd850, valueChanged=true, eventBehavior=WebCore::DispatchNoEvent) at /Volumes/Workspace/webkit/Source/WebCore/html/TextFieldInputType.cpp:111
#9  0x0000000103ca1d5d in WebCore::HTMLInputElement::setValue (this=0x120ef0580, value=@0x7fff5fbfd8e8, eventBehavior=WebCore::DispatchNoEvent) at /Volumes/Workspace/webkit/Source/WebCore/html/HTMLInputElement.cpp:1014
#10 0x0000000103ca2604 in WebCore::HTMLInputElement::setValue (this=0x120ef0580, value=@0x7fff5fbfd8e8, ec=@0x7fff5fbfd8f4, eventBehavior=WebCore::DispatchNoEvent) at /Volumes/Workspace/webkit/Source/WebCore/html/HTMLInputElement.cpp:998
#11 0x0000000104138722 in WebCore::setJSHTMLInputElementValue (exec=0x1167fc058, thisObject=0x118a8fd40, value={u = {asInt64 = 4636448704, ptr = 0x1145a97c0, asBits = {payload = 341481408, tag = 1}}}) at /Volumes/Workspace/WebKit/WebKitBuild/Debug/DerivedSources/WebCore/JSHTMLInputElement.cpp:1034
#12 0x000000010413b079 in JSC::lookupPut<WebCore::JSHTMLInputElement> (exec=0x1167fc058, propertyName={m_impl = 0x10e14d310, static NotAnIndex = 4294967295}, value={u = {asInt64 = 4636448704, ptr = 0x1145a97c0, asBits = {payload = 341481408, tag = 1}}}, table=0x105d6d840, thisObj=0x118a8fd40, shouldThrow=false) at Lookup.h:373
#13 0x000000010413aab8 in JSC::lookupPut<WebCore::JSHTMLInputElement, WebCore::JSHTMLElement> (exec=0x1167fc058, propertyName={m_impl = 0x10e14d310, static NotAnIndex = 4294967295}, value={u = {asInt64 = 4636448704, ptr = 0x1145a97c0, asBits = {payload = 341481408, tag = 1}}}, table=0x105d6d840, thisObj=0x118a8fd40, slot=@0x7fff5fbfdbe0) at Lookup.h:389
#14 0x0000000104135e37 in WebCore::JSHTMLInputElement::put (cell=0x118a8fd40, exec=0x1167fc058, propertyName={m_impl = 0x10e14d310, static NotAnIndex = 4294967295}, value={u = {asInt64 = 4636448704, ptr = 0x1145a97c0, asBits = {payload = 341481408, tag = 1}}}, slot=@0x7fff5fbfdbe0) at /Volumes/Workspace/WebKit/WebKitBuild/Debug/DerivedSources/WebCore/JSHTMLInputElement.cpp:751
#15 0x00000001009d50f9 in JSC::JSValue::put (this=0x7fff5fbfdbf8, exec=0x1167fc058, propertyName={m_impl = 0x10e14d310, static NotAnIndex = 4294967295}, value={u = {asInt64 = 4636448704, ptr = 0x1145a97c0, asBits = {payload = 341481408, tag = 1}}}, slot=@0x7fff5fbfdbe0) at JSValueInlines.h:678

[Aurimas Liutikas]

Created attachment 184829 [details]
Patch

[Aurimas Liutikas]

rniwa, do this this is a reasonable way of solving this?

[Levi Weintraub]

Comment on attachment 184829 [details]
Patch

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

> Source/WebCore/editing/Editor.cpp:2884
> +    // Cancel the composition if the composition selection cannot be retrieved.
> +    unsigned start;
> +    unsigned end;
> +    if (hasComposition() && !ignoreCompositionSelectionChange() && !getCompositionSelection(start, end))
> +        cancelComposition();
> +

The Mac port has embedding code that seems to handle this case. Is there a reason your fix goes in Editor instead? If this is always desired, is the Mac platform code no longer needed?

[Levi Weintraub]

For the record, I talked with Aurimas about needing a test, which was why I suggested he drop the R?. Niwa-san, it'd be good if you could verify this is a reasonable place to make this check, as I'm unfamiliar with our IME design.

[Aurimas Liutikas]

Comment on attachment 184829 [details]
Patch

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

>> Source/WebCore/editing/Editor.cpp:2884
>> +
> 
> The Mac port has embedding code that seems to handle this case. Is there a reason your fix goes in Editor instead? If this is always desired, is the Mac platform code no longer needed?

I think this code will make the Mac port code unnecessary. The only thing that I would need to add is a new notification call didCancelComposition to Editor client to be able to completely replace Mac port code.

I also need to add a test for this.

[Ryosuke Niwa]

Comment on attachment 184829 [details]
Patch

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

> Source/WebCore/editing/Editor.cpp:2883
> +    // Cancel the composition if the composition selection cannot be retrieved.
> +    unsigned start;
> +    unsigned end;
> +    if (hasComposition() && !ignoreCompositionSelectionChange() && !getCompositionSelection(start, end))
> +        cancelComposition();

I'd like to know why this bug only reproduces in Chromium first.

[Aurimas Liutikas]

Comment on attachment 184829 [details]
Patch

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

>> Source/WebCore/editing/Editor.cpp:2883
>> +        cancelComposition();
> 
> I'd like to know why this bug only reproduces in Chromium first.

This bug is fixed in Mac port because on WebEditorClient::respondToChangedSelection there is a call to WebHTMLView::_selectionChanged which in turn calls WebHTMLView::_updateSelectionForInputManager. In _updateSelectionForInputManager there is this exact check as in this patch that calls editor()->cancelComposition().

Chromium does not have a similar call path.

[Ryosuke Niwa]

Comment on attachment 184829 [details]
Patch

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

>>> Source/WebCore/editing/Editor.cpp:2883
>>> +        cancelComposition();
>> 
>> I'd like to know why this bug only reproduces in Chromium first.
> 
> This bug is fixed in Mac port because on WebEditorClient::respondToChangedSelection there is a call to WebHTMLView::_selectionChanged which in turn calls WebHTMLView::_updateSelectionForInputManager. In _updateSelectionForInputManager there is this exact check as in this patch that calls editor()->cancelComposition().
> 
> Chromium does not have a similar call path.

Why don't you make the same call in WebEditorClient then? Alternatively, we can move that code in the mac port to WebCore but then we should be deleting the code in WebEditorClient instead.

[Aurimas Liutikas]

Comment on attachment 184829 [details]
Patch

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

>>>> Source/WebCore/editing/Editor.cpp:2883
>>>> +        cancelComposition();
>>> 
>>> I'd like to know why this bug only reproduces in Chromium first.
>> 
>> This bug is fixed in Mac port because on WebEditorClient::respondToChangedSelection there is a call to WebHTMLView::_selectionChanged which in turn calls WebHTMLView::_updateSelectionForInputManager. In _updateSelectionForInputManager there is this exact check as in this patch that calls editor()->cancelComposition().
>> 
>> Chromium does not have a similar call path.
> 
> Why don't you make the same call in WebEditorClient then? Alternatively, we can move that code in the mac port to WebCore but then we should be deleting the code in WebEditorClient instead.

I think it would be good to have shared code as this would benefit all the WebKit clients. Do yo think this is a good place to put it though?

Also, do you think we should pass a new argument to EditorClient::respondToChangedSelection() that would tell the client if the composition was cancelled? That way I could remove most of the updateSelectionForInputManager code.

[Ryosuke Niwa]

Comment on attachment 184829 [details]
Patch

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

>>>>> Source/WebCore/editing/Editor.cpp:2883
>>>>> +        cancelComposition();
>>>> 
>>>> I'd like to know why this bug only reproduces in Chromium first.
>>> 
>>> This bug is fixed in Mac port because on WebEditorClient::respondToChangedSelection there is a call to WebHTMLView::_selectionChanged which in turn calls WebHTMLView::_updateSelectionForInputManager. In _updateSelectionForInputManager there is this exact check as in this patch that calls editor()->cancelComposition().
>>> 
>>> Chromium does not have a similar call path.
>> 
>> Why don't you make the same call in WebEditorClient then? Alternatively, we can move that code in the mac port to WebCore but then we should be deleting the code in WebEditorClient instead.
> 
> I think it would be good to have shared code as this would benefit all the WebKit clients. Do yo think this is a good place to put it though?
> 
> Also, do you think we should pass a new argument to EditorClient::respondToChangedSelection() that would tell the client if the composition was cancelled? That way I could remove most of the updateSelectionForInputManager code.

We could do that. Alternatively, the client can keep track of when the composition was canceled since I think we already have a callback for when a composition is canceled.

[Aurimas Liutikas]

I just found that there was another WebKit bug open that is pretty similar to this one https://bugs.webkit.org/show_bug.cgi?id=55560 . The previous Chrome developer claimed that this should not be done in Chrome. Do you think his concerns are still valid? I tried to look him up, but he does not seem to be working at Google anymore.

This bug has a test editing/input/setting-input-value-cancel-ime-composition.html that now passes with my patch.

[Ryosuke Niwa]

(In reply to comment #18)
> I just found that there was another WebKit bug open that is pretty similar to this one https://bugs.webkit.org/show_bug.cgi?id=55560 . The previous Chrome developer claimed that this should not be done in Chrome. Do you think his concerns are still valid? I tried to look him up, but he does not seem to be working at Google anymore.

I don't know.

[Aurimas Liutikas]

Created attachment 185648 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Aurimas Liutikas]

Created attachment 185663 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 185663 [details]
Patch

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

> Source/WebKit/chromium/src/WebViewImpl.cpp:1365
> +        unsigned start, end;
> +        Editor* editor = frame->editor();
> +        if (editor->hasComposition() && !editor->ignoreCompositionSelectionChange() && !editor->getCompositionSelection(start, end)) {

Can we extract this as a member function of Editor? It seems like we're duplicating code in WebView.m here.

[Aurimas Liutikas]

Created attachment 185674 [details]
Patch

[Aurimas Liutikas]

rniwa: added the refactored call to Editor. Does this work better?

[WebKit Review Bot]

Comment on attachment 185674 [details]
Patch

Attachment 185674 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16217992

[Build Bot]

Comment on attachment 185674 [details]
Patch

Attachment 185674 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16252179

[Ryosuke Niwa]

(In reply to comment #25)
> rniwa: added the refactored call to Editor. Does this work better?

It seems like it doesn't build?

[Aurimas Liutikas]

rniwa: I was rushing to upload the patch before leaving the office. Will take a look at it first thing in the morning.

[Peter Beverloo (cr-android ews)]

Comment on attachment 185674 [details]
Patch

Attachment 185674 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/16251200

[Build Bot]

Comment on attachment 185674 [details]
Patch

Attachment 185674 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16282067

[Build Bot]

Comment on attachment 185674 [details]
Patch

Attachment 185674 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16251269

[WebKit Review Bot]

Comment on attachment 185674 [details]
Patch

Attachment 185674 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16270130

[Ryosuke Niwa]

Comment on attachment 185674 [details]
Patch

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

> Source/WebCore/editing/Editor.cpp:2403
> +bool Editor::cancelCompositionIfRequired()

IfRequired doesn't really tell us what that requirement is, and when we should be calling this function.
How about something like cancelCompositionIfSelectionIsInvalid, cancelCompositionIfSelectionIsOutdated, or cancelCompositionIfSelectionIsOrphaned?
(orphaned is a adjective we use to describe positions in a detached DOM node. Look for "orphaned" in FrameSelection/VisibleSelection/VisiblePosition).

> Source/WebCore/editing/Editor.cpp:2407
> +    if (hasComposition() && !ignoreCompositionSelectionChange() && !getCompositionSelection(start, end)) {

I would have negated this condition so that you can immediately return false instead.

[Aurimas Liutikas]

Created attachment 185795 [details]
Patch

[Alexey Proskuryakov]

This patch affects cross-platform code, so there should be no [chromium] in the title. The purpose of these prefixes is to signal that people working on other platforms can safely ignore the bug.

[Build Bot]

Comment on attachment 185795 [details]
Patch

Attachment 185795 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16282293

[Alexey Proskuryakov]

Comment on attachment 185795 [details]
Patch

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

Looks reasonable to me. I'm deferring to Ryosuke for final review, since he has been working with you on this for a while already.

> Source/WebCore/editing/Editor.cpp:1340
> +    unsigned start, end;

WebKit style is to define each variable on its own line.

[Ryosuke Niwa]

Comment on attachment 185795 [details]
Patch

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

> Source/WebCore/ChangeLog:7
> +

Please describe the nature of your change.

> Source/WebCore/ChangeLog:8
> +        No new tests (OOPS!).

Please explain that an existing test, editing/input/setting-input-value-cancel-ime-composition.html, covers this change.

> Source/WebCore/ChangeLog:22
> +
> +        * WebCore.exp.in:
> +        * editing/Editor.cpp:
> +        (WebCore::Editor::cancelCompositionIfSelectionIsInvalid):
> +        (WebCore):
> +        * editing/Editor.h:
> +        (Editor):

Why is this repeated here?

> Source/WebKit/chromium/ChangeLog:12
> +        Adding a check whether composition is valid after the selection change.
> +        If there is no valid composition, then the composition should get cancelled
> +        and the WebViewClient should get notified about this cancellation.
> +
> +        This bug already had a test that had expectation set to fail, but now it will be passing.

This explanation should probably be done in Source/WebCore/ChangeLog instead.

> Source/WebKit/mac/WebView/WebHTMLView.mm:6048
>      if (coreFrame->editor()->getCompositionSelection(start, end))
>          [[NSInputManager currentInputManager] markedTextSelectionChanged:NSMakeRange(start, end - start) client:self];

Now this code is executed even if hasComposition() and coreFrame->editor()->ignoreCompositionSelectionChange() are both false.
r- because of this.

> LayoutTests/ChangeLog:10
> +        Adding a check whether composition is valid after the selection change.
> +        If there is no valid composition, then the composition should get cancelled
> +        and the WebViewClient should get notified about this cancellation.

This comment applies to Source/WebCore or Source/WebKit, not LayoutTests. Please remove.

[Aurimas Liutikas]

Created attachment 185821 [details]
Patch

[Aurimas Liutikas]

Comment on attachment 185795 [details]
Patch

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

>> Source/WebKit/chromium/ChangeLog:12
>> +        This bug already had a test that had expectation set to fail, but now it will be passing.
> 
> This explanation should probably be done in Source/WebCore/ChangeLog instead.

Done.

>> Source/WebCore/editing/Editor.cpp:1340
>> +    unsigned start, end;
> 
> WebKit style is to define each variable on its own line.

Done.

>> Source/WebKit/mac/WebView/WebHTMLView.mm:6048
>>          [[NSInputManager currentInputManager] markedTextSelectionChanged:NSMakeRange(start, end - start) client:self];
> 
> Now this code is executed even if hasComposition() and coreFrame->editor()->ignoreCompositionSelectionChange() are both false.
> r- because of this.

My refactoring does not really work for the Mac port (it does for all other ports) so I am leaving the code in this file as it was.

>> LayoutTests/ChangeLog:10
>> +        and the WebViewClient should get notified about this cancellation.
> 
> This comment applies to Source/WebCore or Source/WebKit, not LayoutTests. Please remove.

Done.

[Eric Seidel (no email)]

Your latest patch builds fine in Release on my Mac using build-webkit.  I assume that builds both wk1 and wk2.

[Ryosuke Niwa]

Comment on attachment 185821 [details]
Patch

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

> Source/WebKit/win/WebView.cpp:5486
> +    if (!targetFrame)
>          return;

I don't think focusedOrMainFrame can ever be null and it's obnoxious that it even checks this condition but that's a separate issue.

[WebKit Review Bot]

Comment on attachment 185821 [details]
Patch

Clearing flags on attachment: 185821

Committed r141479: <http://trac.webkit.org/changeset/141479>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Florin Malita]

(In reply to comment #44)
> (From update of attachment 185821 [details])
> Clearing flags on attachment: 185821
> 
> Committed r141479: <http://trac.webkit.org/changeset/141479>

This is causing CR webkit_unit_tests failures:

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=@ToT - chromium.org&testType=webkit_unit_tests&tests=WebViewTest.SetCompositionFromExistingText

http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux/builds/5060/steps/webkit_unit_tests/logs/SetCompositionFromExistingText

[Hajime Morrita]

It seems this also breaks content_browsertests 
http://build.chromium.org/p/chromium.webkit/builders/Win7%20%28dbg%29/builds/9181

Could you take a look or can I roll this out?

[WebKit Review Bot]

Re-opened since this is blocked by bug 108564

[Aurimas Liutikas]

It turns out that this patch exposed a bug in WebViewImpl::setCompositionFromExistingText(). Waiting for the fix to land and then I will re-land this patch.

[Aurimas Liutikas]

Comment on attachment 185821 [details]
Patch

The fix for WebViewImpl::SetCompositionFromExistingText() has landed. That fixes the issue that caused a revert for this patch. Can someone r+ and cq+ this again?

Thanks!

[WebKit Review Bot]

Comment on attachment 185821 [details]
Patch

Clearing flags on attachment: 185821

Committed r141545: <http://trac.webkit.org/changeset/141545>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.