Bug 67377 – REGRESSION(r94274): selection-change-closes-typing.html fails

• Component: HTML Editing
• Status: RESOLVED FIXED

[Ryosuke Niwa]

After http://trac.webkit.org/changeset/94274, platform/mac/editing/input/selection-change-closes-typing.html started failing:
--- /Volumes/Data/WebKit-BuildSlave/snowleopard-intel-release-tests/build/layout-test-results/platform/mac/editing/input/selection-change-closes-typing-expected.txt 
+++ /Volumes/Data/WebKit-BuildSlave/snowleopard-intel-release-tests/build/layout-test-results/platform/mac/editing/input/selection-change-closes-typing-actual.txt 
@@ -1,5 +1,5 @@
 Test for bug 32905: With Pinyin Simplified IM, a wrong character is deleted from google.com suggestion.
 
-Should say PASS: PASS
+Should say PASS: PSAS
 

There's some really bizarre thing going on where changing selection caused by setInnerTextValue is causing a composition to confirm and then overriding m_valueIfDirty with an incorrect value.

[Ryosuke Niwa]

Temporarily skipped the test in http://trac.webkit.org/changeset/94280.

[Ryosuke Niwa]

Please the following comments backwards in the natural order of stack growth:

#0	0x1018f2b14 in WebCore::HTMLInputElement::setValueFromRenderer at HTMLInputElement.cpp:1192

 < we're doomed at this point as we've reached subtreeHasChanged; we think the user has edited text >
#1	0x1018f3760 in WebCore::HTMLInputElement::subtreeHasChanged at HTMLInputElement.cpp:659
#2	0x10192d9bc in WebCore::HTMLTextFormControlElement::defaultEventHandler at HTMLTextFormControlElement.cpp:95
#3	0x1018f6c99 in WebCore::HTMLInputElement::defaultEventHandler at HTMLInputElement.cpp:1315
#4	0x10226284b in WebCore::TextControlInnerTextElement::defaultEventHandler at TextControlInnerElements.cpp:90
#5	0x101786e00 in WebCore::EventDispatcher::dispatchEvent at EventDispatcher.cpp:349
#6	0x1017855f7 in WebCore::EventDispatchMediator::dispatchEvent at EventDispatchMediator.cpp:51
#7	0x101785edf in WebCore::EventDispatcher::dispatchEvent at EventDispatcher.cpp:55
#8	0x101e458ca in WebCore::Node::dispatchEvent at Node.cpp:2702
#9	0x1017a18f3 in WebCore::EventTarget::dispatchEvent at EventTarget.cpp:326
#10	0x10176daf7 in WebCore::dispatchEditableContentChangedEvents at Editor.cpp:961
#11	0x10176dd78 in WebCore::Editor::appliedEditing at Editor.cpp:970
#12	0x102290241 in WebCore::TypingCommand::typingAddedToOpenCommand at TypingCommand.cpp:357
#13	0x102291853 in WebCore::TypingCommand::insertTextRunWithoutNewlines at TypingCommand.cpp:399
#14	0x102291af6 in WebCore::TypingCommand::insertText at TypingCommand.cpp:384
#15	0x1022929a8 in WebCore::TypingCommand::insertText at TypingCommand.cpp:199
#16	0x10176ca55 in WebCore::Editor::insertTextWithoutSendingTextEvent at Editor.cpp:1099
#17	0x10176cc28 in WebCore::Editor::handleTextEvent at Editor.cpp:176
#18	0x101789698 in WebCore::EventHandler::defaultTextInputEventHandler at EventHandler.cpp:2964
#19	0x101e4ba49 in WebCore::Node::defaultEventHandler at Node.cpp:2818
#20	0x10192d9cb in WebCore::HTMLTextFormControlElement::defaultEventHandler at HTMLTextFormControlElement.cpp:99
#21	0x1018f6c99 in WebCore::HTMLInputElement::defaultEventHandler at HTMLInputElement.cpp:1315
#22	0x101786e00 in WebCore::EventDispatcher::dispatchEvent at EventDispatcher.cpp:349
#23	0x1017855f7 in WebCore::EventDispatchMediator::dispatchEvent at EventDispatchMediator.cpp:51
#24	0x101785edf in WebCore::EventDispatcher::dispatchEvent at EventDispatcher.cpp:55
#25	0x101e458ca in WebCore::Node::dispatchEvent at Node.cpp:2702
#26	0x1017a18f3 in WebCore::EventTarget::dispatchEvent at EventTarget.cpp:326
#27	0x10178ec66 in WebCore::EventHandler::handleTextInputEvent at EventHandler.cpp:2929
#28	0x101768d3c in WebCore::Editor::insertTextForConfirmedComposition at Editor.cpp:1057
#29	0x10176d8c3 in WebCore::Editor::confirmComposition at Editor.cpp:1520

 < oops, we've somehow decided to confirm the composition >
#30	0x10176d9ed in WebCore::Editor::confirmCompositionWithoutDisturbingSelection at Editor.cpp:1479
#31	0x100bbdf81 in -[WebHTMLView(WebNSTextInputSupport) _updateSelectionForInputManager] at WebHTMLView.mm:5943
#32	0x100bb7975 in -[WebHTMLView(WebInternal) _selectionChanged] at WebHTMLView.mm:4949
#33	0x100c23a4c in -[WebView(WebViewInternal) _selectionChanged] at WebView.mm:6086
#34	0x100b7a81f in WebEditorClient::respondToChangedSelection at WebEditorClient.mm:295
#35	0x10176ad39 in WebCore::Editor::respondToChangedSelection at Editor.cpp:492
#36	0x10176b424 in WebCore::Editor::respondToChangedSelection at Editor.cpp:3160
#37	0x10181f398 in WebCore::FrameSelection::setSelection at FrameSelection.cpp:233
#38	0x101820a1c in WebCore::FrameSelection::textWillBeReplaced at FrameSelection.cpp:380

 < here, we're about to replace text >
#39	0x10148451a in WebCore::CharacterData::setDataAndUpdate at CharacterData.cpp:176
#40	0x1014849f4 in WebCore::CharacterData::setData at CharacterData.cpp:46
#41	0x1018cde86 in WebCore::replaceChildrenWithText at HTMLElement.cpp:343
#42	0x1018cebfd in WebCore::HTMLElement::setInnerText at HTMLElement.cpp:479
#43	0x10192ba59 in WebCore::HTMLTextFormControlElement::setInnerTextValue at HTMLTextFormControlElement.cpp:465
#44	0x1018f3962 in WebCore::HTMLInputElement::updateInnerTextValue at HTMLInputElement.cpp:639

 < this is where we start >
#45	0x1018f3b29 in WebCore::HTMLInputElement::setValue at HTMLInputElement.cpp:1109
#46	0x101b8e05f in WebCore::setJSHTMLInputElementValue at JSHTMLInputElement.cpp:1009

[Ryosuke Niwa]

Created attachment 105929 [details]
work in progress

I'll attach a work-in-progress patch here but this approach is likely to be wrong. I don't think it makes much sense to ignore changes that happened in setValue.

[Ryosuke Niwa]

I'll think about this bug tomorrow morning when my brain is functioning.

[Ryosuke Niwa]

Created attachment 106053 [details]
fixes the bug

[Ryosuke Niwa]

Created attachment 106056 [details]
properly svn copy tests

[Kent Tamura]

Comment on attachment 106056 [details]
properly svn copy tests

platform/mac/editing/input/selection-change-closes-typing.html  should be unskipped.

[Alexey Proskuryakov]

This change seems risky, but let's see how it goes.

Please check that this test passes in manual testing in both single process and multiple process modes in Safari. Also, please check that this patch doesn't regress editing/input (and platform/mac/editing/input) regression tests in WebKit2 mode.

+// FIXME: This function is poorly named. Callers of this function uses this function to cancel composition,
+// and don't expect it to insert the current composition as the name suggests.

Can this be addressed soon?

[Ryosuke Niwa]

(In reply to comment #8)
> This change seems risky, but let's see how it goes.

I agree.

> +// FIXME: This function is poorly named. Callers of this function uses this function to cancel composition,
> +// and don't expect it to insert the current composition as the name suggests.
> 
> Can this be addressed soon?

Yes, I'm hoping to get it done once we know this patch is safe :)  I just didn't want to mix this risky patch with a big refactoring.

[Ryosuke Niwa]

Okay I've done the test ap suggested (run all layout tests + manually test some cases) on both WebKit 1 and WebKit 2 of mac port.  I also submitted the patch to Chromium try server and got green runs:

http://build.chromium.org/p/tryserver.chromium/builders/linux/builds/47099
http://build.chromium.org/p/tryserver.chromium/builders/win/builds/52984
http://build.chromium.org/p/tryserver.chromium/builders/mac/builds/47087

[Ryosuke Niwa]

(In reply to comment #10)
> Okay I've done the test ap suggested (run all layout tests + manually test some cases) on both WebKit 1 and WebKit 2 of mac port.  I also submitted the patch to Chromium try server and got green runs:
> 
> http://build.chromium.org/p/tryserver.chromium/builders/linux/builds/47099
> http://build.chromium.org/p/tryserver.chromium/builders/win/builds/52984
> http://build.chromium.org/p/tryserver.chromium/builders/mac/builds/47087

Apparently these try runs didn't apply patch properly :(  Here are new runs:
http://build.chromium.org/p/tryserver.chromium/builders/linux/builds/47291
http://build.chromium.org/p/tryserver.chromium/builders/mac/builds/47271
http://build.chromium.org/p/tryserver.chromium/builders/win/builds/53196

[Ryosuke Niwa]

Comment on attachment 106056 [details]
properly svn copy tests

Clearing flags on attachment: 106056

Committed r94497: <http://trac.webkit.org/changeset/94497>

[Ryosuke Niwa]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 106056 [details]
properly svn copy tests

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

> Source/WebCore/editing/Editor.cpp:1496
> +        ASSERT(text == emptyString());

The normal way to write this is:

    ASSERT(text.isEmpty());

[Kenneth Rohde Christiansen]

This patch modified confrmSelectionWithoutDisturbingSelection which we are using in the Qt port, though that part is unfortunately not upstreamed yet.

Example:

"a dog jumpedˆ over a bag" with jumped being the composition. When I touch bag with my finger it should become "a dog jumped over the bagˆ" and not "a dog over the bagˆ", thus I need to confirm in that case and not modify the selection.