| Summary: | Editor::m_compositionNode not updated on HTMLInputElement::setValue() | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Aurimas Liutikas <aurimas> | ||||||||||||||||
| Component: | HTML Editing | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||
| Severity: | Normal | CC: | abarth, ap, buildbot, dglazkov, enrica, eric, fishd, fmalita, gyuyoung.kim, hbono, jamesr, leviw, mifenton, morrita, peter+ews, rakuco, rniwa, tkent+wkapi, tony, webkit.review.bot | ||||||||||||||||
| Priority: | P2 | ||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||
| Hardware: | All | ||||||||||||||||||
| OS: | All | ||||||||||||||||||
| Bug Depends on: | 108543, 108564 | ||||||||||||||||||
| Bug Blocks: | 128323 | ||||||||||||||||||
| Attachments: |
|
||||||||||||||||||
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. 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? 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. I don't observe the said behavior on Mac port. I'm pretty certain this is a problem in Chromium's IME code. I can reproduce this problem on Chrome (Mac). Does Mac port call commitComposition or cancelComposition before JavaScript changes? 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
Created attachment 184829 [details]
Patch
rniwa, do this this is a reasonable way of solving this? 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? 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. 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. 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. 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. 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. 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. 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. 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. (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. Created attachment 185648 [details]
Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI. Created attachment 185663 [details]
Patch
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. Created attachment 185674 [details]
Patch
rniwa: added the refactored call to Editor. Does this work better? Comment on attachment 185674 [details] Patch Attachment 185674 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16217992 Comment on attachment 185674 [details] Patch Attachment 185674 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16252179 (In reply to comment #25) > rniwa: added the refactored call to Editor. Does this work better? It seems like it doesn't build? rniwa: I was rushing to upload the patch before leaving the office. Will take a look at it first thing in the morning. Comment on attachment 185674 [details] Patch Attachment 185674 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/16251200 Comment on attachment 185674 [details] Patch Attachment 185674 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16282067 Comment on attachment 185674 [details] Patch Attachment 185674 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16251269 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 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. Created attachment 185795 [details]
Patch
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. Comment on attachment 185795 [details] Patch Attachment 185795 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16282293 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. 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. Created attachment 185821 [details]
Patch
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. Your latest patch builds fine in Release on my Mac using build-webkit. I assume that builds both wk1 and wk2. 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. Comment on attachment 185821 [details] Patch Clearing flags on attachment: 185821 Committed r141479: <http://trac.webkit.org/changeset/141479> All reviewed patches have been landed. Closing bug. (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 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? Re-opened since this is blocked by bug 108564 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. 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!
Comment on attachment 185821 [details] Patch Clearing flags on attachment: 185821 Committed r141545: <http://trac.webkit.org/changeset/141545> All reviewed patches have been landed. Closing bug. |
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.