Bug 67377 - REGRESSION(r94274): selection-change-closes-typing.html fails
Summary: REGRESSION(r94274): selection-change-closes-typing.html fails
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks: 67569
  Show dependency treegraph
 
Reported: 2011-09-01 01:37 PDT by Ryosuke Niwa
Modified: 2011-09-06 13:36 PDT (History)
9 users (show)

See Also:


Attachments
work in progress (4.50 KB, patch)
2011-09-01 02:24 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
fixes the bug (5.46 KB, patch)
2011-09-01 16:05 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
properly svn copy tests (6.53 KB, patch)
2011-09-01 16:09 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2011-09-01 01:37:40 PDT
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.
Comment 1 Ryosuke Niwa 2011-09-01 02:02:31 PDT
Temporarily skipped the test in http://trac.webkit.org/changeset/94280.
Comment 2 Ryosuke Niwa 2011-09-01 02:23:04 PDT
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
Comment 3 Ryosuke Niwa 2011-09-01 02:24:59 PDT
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.
Comment 4 Ryosuke Niwa 2011-09-01 02:25:30 PDT
I'll think about this bug tomorrow morning when my brain is functioning.
Comment 5 Ryosuke Niwa 2011-09-01 16:05:55 PDT
Created attachment 106053 [details]
fixes the bug
Comment 6 Ryosuke Niwa 2011-09-01 16:09:51 PDT
Created attachment 106056 [details]
properly svn copy tests
Comment 7 Kent Tamura 2011-09-02 02:22:56 PDT
Comment on attachment 106056 [details]
properly svn copy tests

platform/mac/editing/input/selection-change-closes-typing.html  should be unskipped.
Comment 8 Alexey Proskuryakov 2011-09-02 12:57:02 PDT
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?
Comment 9 Ryosuke Niwa 2011-09-02 13:10:00 PDT
(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.
Comment 10 Ryosuke Niwa 2011-09-02 14:37:37 PDT
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
Comment 11 Ryosuke Niwa 2011-09-03 00:27:01 PDT
(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
Comment 12 Ryosuke Niwa 2011-09-03 16:29:03 PDT
Comment on attachment 106056 [details]
properly svn copy tests

Clearing flags on attachment: 106056

Committed r94497: <http://trac.webkit.org/changeset/94497>
Comment 13 Ryosuke Niwa 2011-09-03 16:29:08 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Darin Adler 2011-09-03 18:33:36 PDT
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());
Comment 15 Kenneth Rohde Christiansen 2011-09-06 13:35:23 PDT
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.