Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2493703002:
Make "compositionend" event fired after setting caret position (Closed)
DescriptionMake "compositionend" event fired after setting caret position
JS may want to listen to 'compositionend' and mutate DOM & move focus after IME finishes.
Besides, according to the spec (https://www.w3.org/TR/uievents/#event-type-compositionend),
we should fire 'compositionend' after all IME controls such as inserting text and moving
caret. But we are resetting caret position after 'compositionend' event listeners now.
This CL will make the order correct.
BUG=663944
TEST=run_webkit_layout_test --gtest_filter=InputMethodControllerTest.*
Committed: https://crrev.com/df699ce3ffd28396d0447fbc8072ebe7199857ca
Cr-Commit-Position: refs/heads/master@{#433308}
Patch Set 1 : Not able to listen to selectionchange event #
Total comments: 1
Patch Set 2 : Ignore selectionchange event #Patch Set 3 #Patch Set 4 : Address yosin@'s review #
Total comments: 3
Patch Set 5 : Address yosin@'s 2nd review #Patch Set 6 : simulate "Potential issue" #
Total comments: 8
Patch Set 7 #
Total comments: 4
Patch Set 8 : Add more test. #
Total comments: 24
Patch Set 9 : Refactor #
Total comments: 9
Patch Set 10 : A little change #Patch Set 11 : Rebase #Patch Set 12 : format #
Total comments: 1
Messages
Total messages: 88 (62 generated)
The CQ bit was checked by yabinh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by yabinh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Set caret positon before "compositionend" event when confirming/canceling composition BUG=663944 ========== to ========== Make "compositionend" event fired AFTER setting caret positon when confirming/canceling composition BUG=663944 ==========
yabinh@chromium.org changed reviewers: + aelias@chromium.org, changwan@chromium.org
Description was changed from ========== Make "compositionend" event fired AFTER setting caret positon when confirming/canceling composition BUG=663944 ========== to ========== Make "compositionend" event fired AFTER setting caret positon BUG=663944 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL~ https://codereview.chromium.org/2493703002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp (right): https://codereview.chromium.org/2493703002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:860: "});" It seems that selectionchange doesn't work here. (I also tried <input> and <textarea>). Note that selectionchange event is queued and races with other events, so even if we are able to listen to it, we are still not able to ensure the correct order.
aelias@chromium.org changed reviewers: + yosin@chromium.org - aelias@chromium.org, changwan@chromium.org
aelias@chromium.org changed reviewers: + aelias@chromium.org
I don't have much opinion on this one way or another, replacing myself with yosin@ as reviewer.
Code changes are fine. Could you add a test case checks selection is changed before "compositionend" event?
The CQ bit was checked by yabinh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by yabinh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/11/11 01:58:09, Yosi_UTC9 wrote: > Code changes are fine. > Could you add a test case checks selection is changed before "compositionend" > event? Done. PTAL~
https://codereview.chromium.org/2493703002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp (right): https://codereview.chromium.org/2493703002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:858: " if (getSelection().anchorOffset != event.data.length)" nit: We should have {} for then-clause. https://codereview.chromium.org/2493703002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:859: " document.title += 'Wrong selection! Expected ' " Can we have a test case for this? BTW, I don't think it is wrong selection. Since, caller can specify caret position other than end.
The CQ bit was checked by yabinh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL~ https://codereview.chromium.org/2493703002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp (right): https://codereview.chromium.org/2493703002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:859: " document.title += 'Wrong selection! Expected ' " On 2016/11/11 04:17:46, Yosi_UTC9 wrote: > Can we have a test case for this? > > BTW, I don't think it is wrong selection. Since, caller can specify caret > position other than end. How about this: document.title += `selectionOffset:${getSelection().anchorOffset};`; Thus, we don't need additional test case.
On 2016/11/11 at 05:23:52, yabinh wrote: > PTAL~ > > https://codereview.chromium.org/2493703002/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp (right): > > https://codereview.chromium.org/2493703002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:859: " document.title += 'Wrong selection! Expected ' " > On 2016/11/11 04:17:46, Yosi_UTC9 wrote: > > Can we have a test case for this? > > > > BTW, I don't think it is wrong selection. Since, caller can specify caret > > position other than end. > > How about this: > document.title += `selectionOffset:${getSelection().anchorOffset};`; > Thus, we don't need additional test case. Please update description why we do this, e.g. summary of "Potential Issue" in a bug. It is better have a test case to simulate "Potential issue" and leave event data test is original since selection isn't related event data; it is too complex to read.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Make "compositionend" event fired AFTER setting caret positon BUG=663944 ========== to ========== JS may want to listen to 'compositionend' and mutate DOM & move focus after IME finishes. Besides, according to the spec (https://www.w3.org/TR/uievents/#event-type-compositionend), we should fire 'compositionend' after all IME controls such as inserting text and moving caret. But we are resetting caret position after 'compositionend' event listeners incorrectly now. This CL will make the order correct. BUG=663944 ==========
Description was changed from ========== JS may want to listen to 'compositionend' and mutate DOM & move focus after IME finishes. Besides, according to the spec (https://www.w3.org/TR/uievents/#event-type-compositionend), we should fire 'compositionend' after all IME controls such as inserting text and moving caret. But we are resetting caret position after 'compositionend' event listeners incorrectly now. This CL will make the order correct. BUG=663944 ========== to ========== JS may want to listen to 'compositionend' and mutate DOM & move focus after IME finishes. Besides, according to the spec (https://www.w3.org/TR/uievents/#event-type-compositionend), we should fire 'compositionend' after all IME controls such as inserting text and moving caret. But we are resetting caret position after 'compositionend' event listeners incorrectly now. This CL will make the order correct. BUG=663944 ==========
Description was changed from ========== JS may want to listen to 'compositionend' and mutate DOM & move focus after IME finishes. Besides, according to the spec (https://www.w3.org/TR/uievents/#event-type-compositionend), we should fire 'compositionend' after all IME controls such as inserting text and moving caret. But we are resetting caret position after 'compositionend' event listeners incorrectly now. This CL will make the order correct. BUG=663944 ========== to ========== JS may want to listen to 'compositionend' and mutate DOM & move focus after IME finishes. Besides, according to the spec (https://www.w3.org/TR/uievents/#event-type-compositionend), we should fire 'compositionend' after all IME controls such as inserting text and moving caret. But we are resetting caret position after 'compositionend' event listeners incorrectly now. This CL will make the order correct. BUG=663944 ==========
Description was changed from ========== JS may want to listen to 'compositionend' and mutate DOM & move focus after IME finishes. Besides, according to the spec (https://www.w3.org/TR/uievents/#event-type-compositionend), we should fire 'compositionend' after all IME controls such as inserting text and moving caret. But we are resetting caret position after 'compositionend' event listeners incorrectly now. This CL will make the order correct. BUG=663944 ========== to ========== JS may want to listen to 'compositionend' and mutate DOM & move focus after IME finishes. Besides, according to the spec (https://www.w3.org/TR/uievents/#event-type-compositionend), we should fire 'compositionend' after all IME controls such as inserting text and moving caret. But we are resetting caret position after 'compositionend' event listeners now. This CL will make the order correct. BUG=663944 ==========
The CQ bit was checked by yabinh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== JS may want to listen to 'compositionend' and mutate DOM & move focus after IME finishes. Besides, according to the spec (https://www.w3.org/TR/uievents/#event-type-compositionend), we should fire 'compositionend' after all IME controls such as inserting text and moving caret. But we are resetting caret position after 'compositionend' event listeners now. This CL will make the order correct. BUG=663944 ========== to ========== JS may want to listen to 'compositionend' and mutate DOM & move focus after IME finishes. Besides, according to the spec (https://www.w3.org/TR/uievents/#event-type-compositionend), we should fire 'compositionend' after all IME controls such as inserting text and moving caret. But we are resetting caret position after 'compositionend' event listeners now. This CL will make the order correct. BUG=663944 ==========
> Please update description why we do this, e.g. summary of "Potential Issue" in a > bug. > It is better have a test case to simulate "Potential issue" and leave event data > test is original since selection isn't related event data; it is too complex to > read. Done. PTAL~
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Please write description as:
Title\n
\n
Description
\n
BUG=XXXX
TEST=run_webkit_layout_test --gtest_filter=XXX
The test is still unclear to me.
It seems replaceCompositionAndMoveCaret() is good tester.
// Inserts the given text string in the place of the existing composition
// and moves caret. Returns true if did replace and moved caret successfully.
bool replaceCompositionAndMoveCaret(const String&, int relativeCaretPosition);
.. set caret at end of composition text
replaceCompositionAndMoveCaret("abcdefg", -3);
EXPECT_EQ(caret position, 4)
Also, using 0 as expected value isn't robust, since 0 sometimes means fallback
value.
https://codereview.chromium.org/2493703002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right):
https://codereview.chromium.org/2493703002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:218: bool
result = replaceComposition(composing);
nit: s/bool/const bool/
https://codereview.chromium.org/2493703002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp
(right):
https://codereview.chromium.org/2493703002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:857:
"function(event) {"
nit: s/function(event)/event =>/
More ES6-ish ;-)
https://codereview.chromium.org/2493703002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:860: "
var node = document.getElementById('sample').firstChild;"
nit: s/var/const/
https://codereview.chromium.org/2493703002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:861: "
var range = document.createRange();"
Just write as below:
getSeleciton().collapse(node, 0);
Description was changed from ========== JS may want to listen to 'compositionend' and mutate DOM & move focus after IME finishes. Besides, according to the spec (https://www.w3.org/TR/uievents/#event-type-compositionend), we should fire 'compositionend' after all IME controls such as inserting text and moving caret. But we are resetting caret position after 'compositionend' event listeners now. This CL will make the order correct. BUG=663944 ========== to ========== Make "compositionend" event fired after setting caret positon JS may want to listen to 'compositionend' and mutate DOM & move focus after IME finishes. Besides, according to the spec (https://www.w3.org/TR/uievents/#event-type-compositionend), we should fire 'compositionend' after all IME controls such as inserting text and moving caret. But we are resetting caret position after 'compositionend' event listeners now. This CL will make the order correct. BUG=663944 ==========
The CQ bit was checked by yabinh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
> It seems replaceCompositionAndMoveCaret() is good tester. commitText calls replaceCompositionAndMoveCaret most of the time. Also, finishComposingText(DoNotKeepSelection) also calls replaceCompositionAndMoveCaret. So I think it's OK to use commitText or finishComposingText(DoNotKeepSelection). Besides, replaceCompositionAndMoveCaret is a private function. I think we'd better not change that. To make it more clear, I'll add some comment. > Also, using 0 as expected value isn't robust, since 0 sometimes means fallback value. Done. Changed it to another value. https://codereview.chromium.org/2493703002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/2493703002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:218: bool result = replaceComposition(composing); On 2016/11/15 02:27:04, Yosi_UTC9 wrote: > nit: s/bool/const bool/ Done. https://codereview.chromium.org/2493703002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp (right): https://codereview.chromium.org/2493703002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:857: "function(event) {" On 2016/11/15 02:27:05, Yosi_UTC9 wrote: > nit: s/function(event)/event =>/ > More ES6-ish ;-) Done. Also applied the same style in other place. https://codereview.chromium.org/2493703002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:860: " var node = document.getElementById('sample').firstChild;" On 2016/11/15 02:27:04, Yosi_UTC9 wrote: > nit: s/var/const/ Done. https://codereview.chromium.org/2493703002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:861: " var range = document.createRange();" On 2016/11/15 02:27:05, Yosi_UTC9 wrote: > Just write as below: > getSeleciton().collapse(node, 0); Done.
Description was changed from ========== Make "compositionend" event fired after setting caret positon JS may want to listen to 'compositionend' and mutate DOM & move focus after IME finishes. Besides, according to the spec (https://www.w3.org/TR/uievents/#event-type-compositionend), we should fire 'compositionend' after all IME controls such as inserting text and moving caret. But we are resetting caret position after 'compositionend' event listeners now. This CL will make the order correct. BUG=663944 ========== to ========== Make "compositionend" event fired after setting caret positon JS may want to listen to 'compositionend' and mutate DOM & move focus after IME finishes. Besides, according to the spec (https://www.w3.org/TR/uievents/#event-type-compositionend), we should fire 'compositionend' after all IME controls such as inserting text and moving caret. But we are resetting caret position after 'compositionend' event listeners now. This CL will make the order correct. BUG=663944 TEST=run_webkit_layout_test --gtest_filter=InputMethodControllerTest.CompositionInputEventIsComposing ==========
Description was changed from ========== Make "compositionend" event fired after setting caret positon JS may want to listen to 'compositionend' and mutate DOM & move focus after IME finishes. Besides, according to the spec (https://www.w3.org/TR/uievents/#event-type-compositionend), we should fire 'compositionend' after all IME controls such as inserting text and moving caret. But we are resetting caret position after 'compositionend' event listeners now. This CL will make the order correct. BUG=663944 TEST=run_webkit_layout_test --gtest_filter=InputMethodControllerTest.CompositionInputEventIsComposing ========== to ========== Make "compositionend" event fired after setting caret positon JS may want to listen to 'compositionend' and mutate DOM & move focus after IME finishes. Besides, according to the spec (https://www.w3.org/TR/uievents/#event-type-compositionend), we should fire 'compositionend' after all IME controls such as inserting text and moving caret. But we are resetting caret position after 'compositionend' event listeners now. This CL will make the order correct. BUG=663944 TEST=run_webkit_layout_test --gtest_filter=InputMethodControllerTest.CompositionInputEventData ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
Description was changed from ========== Make "compositionend" event fired after setting caret positon JS may want to listen to 'compositionend' and mutate DOM & move focus after IME finishes. Besides, according to the spec (https://www.w3.org/TR/uievents/#event-type-compositionend), we should fire 'compositionend' after all IME controls such as inserting text and moving caret. But we are resetting caret position after 'compositionend' event listeners now. This CL will make the order correct. BUG=663944 TEST=run_webkit_layout_test --gtest_filter=InputMethodControllerTest.CompositionInputEventData ========== to ========== Make "compositionend" event fired after setting caret position JS may want to listen to 'compositionend' and mutate DOM & move focus after IME finishes. Besides, according to the spec (https://www.w3.org/TR/uievents/#event-type-compositionend), we should fire 'compositionend' after all IME controls such as inserting text and moving caret. But we are resetting caret position after 'compositionend' event listeners now. This CL will make the order correct. BUG=663944 TEST=run_webkit_layout_test --gtest_filter=InputMethodControllerTest.CompositionInputEventData ==========
https://codereview.chromium.org/2493703002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp (right): https://codereview.chromium.org/2493703002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:842: TEST_F(InputMethodControllerTest, CompositionInputEventData) { This test case is too long and it is hard to use for debugging since each step depend on previous step. We should break out them into individual tests for ease of narrowing failure cause, e.g. TEST_F(IMCTest, CompositionEndForDelete) TEST_F(IMCTest, CompositionEndForInsert) TEST_F(IMCTest, CompositionEndForReplace) TEST_F(IMCTest, BeforeInputForDelete) TEST_F(IMCTest, BeforeInputForInsert) TEST_F(IMCTest, BeforeInputForReplace) https://codereview.chromium.org/2493703002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:861: " getSelection().collapse(node, 3);" Could you add two more tests which set selection - range, e.g. 2,5 - none, getSelection().removeAllRange()
The CQ bit was checked by yabinh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Make "compositionend" event fired after setting caret position JS may want to listen to 'compositionend' and mutate DOM & move focus after IME finishes. Besides, according to the spec (https://www.w3.org/TR/uievents/#event-type-compositionend), we should fire 'compositionend' after all IME controls such as inserting text and moving caret. But we are resetting caret position after 'compositionend' event listeners now. This CL will make the order correct. BUG=663944 TEST=run_webkit_layout_test --gtest_filter=InputMethodControllerTest.CompositionInputEventData ========== to ========== Make "compositionend" event fired after setting caret position JS may want to listen to 'compositionend' and mutate DOM & move focus after IME finishes. Besides, according to the spec (https://www.w3.org/TR/uievents/#event-type-compositionend), we should fire 'compositionend' after all IME controls such as inserting text and moving caret. But we are resetting caret position after 'compositionend' event listeners now. This CL will make the order correct. BUG=663944 TEST=run_webkit_layout_test --gtest_filter=InputMethodControllerTest.* ==========
https://codereview.chromium.org/2493703002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp (right): https://codereview.chromium.org/2493703002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:842: TEST_F(InputMethodControllerTest, CompositionInputEventData) { On 2016/11/16 02:02:10, Yosi_UTC9 wrote: > This test case is too long and it is hard to use for debugging since each step > depend on previous step. > We should break out them into individual tests for ease of narrowing failure > cause, e.g. > > TEST_F(IMCTest, CompositionEndForDelete) > TEST_F(IMCTest, CompositionEndForInsert) > TEST_F(IMCTest, CompositionEndForReplace) > TEST_F(IMCTest, BeforeInputForDelete) > TEST_F(IMCTest, BeforeInputForInsert) > TEST_F(IMCTest, BeforeInputForReplace) Done. https://codereview.chromium.org/2493703002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:861: " getSelection().collapse(node, 3);" On 2016/11/16 02:02:10, Yosi_UTC9 wrote: > Could you add two more tests which set selection > - range, e.g. 2,5 > - none, getSelection().removeAllRange() Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2493703002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp (right): https://codereview.chromium.org/2493703002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:842: TEST_F(InputMethodControllerTest, CompositionInputEventForReplace) { Could you make another CL for re-factoring IMCTest.cpp? https://codereview.chromium.org/2493703002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:845: "<div id='sample' contentEditable='true'></div>", "sample"); nit: s/contentEditable='true'/contenteditable/ https://codereview.chromium.org/2493703002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:849: "event => {" nit: You can write this in one line: event => document.tile += `beforeinput.data:${event.data};`); This can make code compact. https://codereview.chromium.org/2493703002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:886: Element* script = document().createElement("script", ASSERT_NO_EXCEPTION); nit: No need to have |ASSERT_NO_EXCEPTION| https://codereview.chromium.org/2493703002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:900: ASSERT_NO_EXCEPTION); Note: I'm going to make |ASSERT_NO_EXCPETION| in |setInnerHTML()| to be default. See http://crrev.com/2505883002 https://codereview.chromium.org/2493703002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:902: document().view()->updateAllLifecyclePhases(); nit: No need to have |ASSERT_NO_EXCEPTION| https://codereview.chromium.org/2493703002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:926: Element* script = document().createElement("script", ASSERT_NO_EXCEPTION); nit: No need to have |ASSERT_NO_EXCEPTION| https://codereview.chromium.org/2493703002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:941: document().body()->appendChild(script, ASSERT_NO_EXCEPTION); nit: No need to have |ASSERT_NO_EXCEPTION| https://codereview.chromium.org/2493703002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:965: Element* script = document().createElement("script", ASSERT_NO_EXCEPTION); nit: No need to have |ASSERT_NO_EXCEPTION| https://codereview.chromium.org/2493703002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:1048: Element* script = document().createElement("script", ASSERT_NO_EXCEPTION); nit: No need to have |ASSERT_NO_EXCEPTION| https://codereview.chromium.org/2493703002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:1081: Element* script = document().createElement("script", ASSERT_NO_EXCEPTION); nit: No need to have |ASSERT_NO_EXCEPTION| https://codereview.chromium.org/2493703002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:1121: Element* script = document().createElement("script", ASSERT_NO_EXCEPTION); nit: No need to have |ASSERT_NO_EXCEPTION|
The CQ bit was checked by yabinh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2493703002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp (right): https://codereview.chromium.org/2493703002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:842: TEST_F(InputMethodControllerTest, CompositionInputEventForReplace) { On 2016/11/17 01:39:55, Yosi_UTC9 wrote: > Could you make another CL for re-factoring IMCTest.cpp? Done. https://codereview.chromium.org/2493703002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:845: "<div id='sample' contentEditable='true'></div>", "sample"); On 2016/11/17 01:39:55, Yosi_UTC9 wrote: > nit: s/contentEditable='true'/contenteditable/ Done. Also applied to other places. https://codereview.chromium.org/2493703002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:849: "event => {" On 2016/11/17 01:39:55, Yosi_UTC9 wrote: > nit: You can write this in one line: > > event => document.tile += `beforeinput.data:${event.data};`); > > This can make code compact. Done. https://codereview.chromium.org/2493703002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:886: Element* script = document().createElement("script", ASSERT_NO_EXCEPTION); On 2016/11/17 01:39:55, Yosi_UTC9 wrote: > nit: No need to have |ASSERT_NO_EXCEPTION| Done. https://codereview.chromium.org/2493703002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:900: ASSERT_NO_EXCEPTION); On 2016/11/17 01:39:55, Yosi_UTC9 wrote: > Note: I'm going to make |ASSERT_NO_EXCPETION| in |setInnerHTML()| to be default. > See http://crrev.com/2505883002 Done. https://codereview.chromium.org/2493703002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:902: document().view()->updateAllLifecyclePhases(); On 2016/11/17 01:39:55, Yosi_UTC9 wrote: > nit: No need to have |ASSERT_NO_EXCEPTION| Done. https://codereview.chromium.org/2493703002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:926: Element* script = document().createElement("script", ASSERT_NO_EXCEPTION); On 2016/11/17 01:39:55, Yosi_UTC9 wrote: > nit: No need to have |ASSERT_NO_EXCEPTION| Done. https://codereview.chromium.org/2493703002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:941: document().body()->appendChild(script, ASSERT_NO_EXCEPTION); On 2016/11/17 01:39:55, Yosi_UTC9 wrote: > nit: No need to have |ASSERT_NO_EXCEPTION| Done. https://codereview.chromium.org/2493703002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:965: Element* script = document().createElement("script", ASSERT_NO_EXCEPTION); On 2016/11/17 01:39:55, Yosi_UTC9 wrote: > nit: No need to have |ASSERT_NO_EXCEPTION| Done. https://codereview.chromium.org/2493703002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:1048: Element* script = document().createElement("script", ASSERT_NO_EXCEPTION); On 2016/11/17 01:39:55, Yosi_UTC9 wrote: > nit: No need to have |ASSERT_NO_EXCEPTION| Done. https://codereview.chromium.org/2493703002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:1081: Element* script = document().createElement("script", ASSERT_NO_EXCEPTION); On 2016/11/17 01:39:55, Yosi_UTC9 wrote: > nit: No need to have |ASSERT_NO_EXCEPTION| Done. https://codereview.chromium.org/2493703002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:1121: Element* script = document().createElement("script", ASSERT_NO_EXCEPTION); On 2016/11/17 01:39:55, Yosi_UTC9 wrote: > nit: No need to have |ASSERT_NO_EXCEPTION| Done.
lgtm w/ nits https://codereview.chromium.org/2493703002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp (left): https://codereview.chromium.org/2493703002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:821: document().body()->appendChild(script, ASSERT_NO_EXCEPTION); http://crrev.com/2506903005 is being committed. Please rebase once it is committed. https://codereview.chromium.org/2493703002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp (right): https://codereview.chromium.org/2493703002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:63: "event => document.title = `beforeinput.data:${event.data};`);" nit: s/event =>/ event ->/ https://codereview.chromium.org/2493703002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:65: "event => document.title += `input.data:${event.data};`);" nit: s/event =>/ event ->/ https://codereview.chromium.org/2493703002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:67: "event => document.title += `compositionend.data:${event.data};`);"); nit: s/event =>/ event ->/ https://codereview.chromium.org/2493703002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:105: " var range = document.createRange();" nit: s/var/const/ https://codereview.chromium.org/2493703002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:110: " selection.addRange(range);" We don't need to use |Rang| object. Just write as below: seleciton.collapse(node, 2); selection.extend(node, 4);
The CQ bit was checked by yabinh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2493703002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp (right): https://codereview.chromium.org/2493703002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:63: "event => document.title = `beforeinput.data:${event.data};`);" On 2016/11/17 05:38:06, Yosi_UTC9 wrote: > nit: s/event =>/ event ->/ It seems that "event ->" doesn't work. https://codereview.chromium.org/2493703002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:110: " selection.addRange(range);" On 2016/11/17 05:38:06, Yosi_UTC9 wrote: > We don't need to use |Rang| object. Just write as below: > > seleciton.collapse(node, 2); > selection.extend(node, 4); Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by yabinh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2493703002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp (right): https://codereview.chromium.org/2493703002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:63: "event => document.title = `beforeinput.data:${event.data};`);" On 2016/11/17 06:58:15, yabinh wrote: > On 2016/11/17 05:38:06, Yosi_UTC9 wrote: > > nit: s/event =>/ event ->/ > > It seems that "event ->" doesn't work. According to ES6, there is only fat arrow function(=>), no thin arrow function(->). See http://softwareengineering.stackexchange.com/questions/306082/why-doesnt-es6-... and the document of arrow function: http://exploringjs.com/es6/ch_arrow-functions.html
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by yabinh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yosin@chromium.org Link to the patchset: https://codereview.chromium.org/2493703002/#ps200001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Internal error: failed to checkout. Please try again.
The CQ bit was checked by yabinh@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/11/17 at 18:18:45, yabinh wrote: > https://codereview.chromium.org/2493703002/diff/160001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp (right): > > https://codereview.chromium.org/2493703002/diff/160001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:63: "event => document.title = `beforeinput.data:${event.data};`);" > On 2016/11/17 06:58:15, yabinh wrote: > > On 2016/11/17 05:38:06, Yosi_UTC9 wrote: > > > nit: s/event =>/ event ->/ > > > > It seems that "event ->" doesn't work. > > According to ES6, there is only fat arrow function(=>), no thin arrow function(->). > > See http://softwareengineering.stackexchange.com/questions/306082/why-doesnt-es6-... > and the document of arrow function: > http://exploringjs.com/es6/ch_arrow-functions.html Oops, sorrty it is typo. I said add an indentation or adding two spaces before "event =>". So, I could say nit: s/event =>/ event =>/
The CQ bit was unchecked by yosin@chromium.org
The CQ bit was checked by yabinh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yosin@chromium.org Link to the patchset: https://codereview.chromium.org/2493703002/#ps220001 (title: "format")
> Oops, sorrty it is typo. I said add an indentation or adding two spaces before > "event =>". > So, I could say nit: s/event =>/ event =>/ Done.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Make "compositionend" event fired after setting caret position JS may want to listen to 'compositionend' and mutate DOM & move focus after IME finishes. Besides, according to the spec (https://www.w3.org/TR/uievents/#event-type-compositionend), we should fire 'compositionend' after all IME controls such as inserting text and moving caret. But we are resetting caret position after 'compositionend' event listeners now. This CL will make the order correct. BUG=663944 TEST=run_webkit_layout_test --gtest_filter=InputMethodControllerTest.* ========== to ========== Make "compositionend" event fired after setting caret position JS may want to listen to 'compositionend' and mutate DOM & move focus after IME finishes. Besides, according to the spec (https://www.w3.org/TR/uievents/#event-type-compositionend), we should fire 'compositionend' after all IME controls such as inserting text and moving caret. But we are resetting caret position after 'compositionend' event listeners now. This CL will make the order correct. BUG=663944 TEST=run_webkit_layout_test --gtest_filter=InputMethodControllerTest.* ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Make "compositionend" event fired after setting caret position JS may want to listen to 'compositionend' and mutate DOM & move focus after IME finishes. Besides, according to the spec (https://www.w3.org/TR/uievents/#event-type-compositionend), we should fire 'compositionend' after all IME controls such as inserting text and moving caret. But we are resetting caret position after 'compositionend' event listeners now. This CL will make the order correct. BUG=663944 TEST=run_webkit_layout_test --gtest_filter=InputMethodControllerTest.* ========== to ========== Make "compositionend" event fired after setting caret position JS may want to listen to 'compositionend' and mutate DOM & move focus after IME finishes. Besides, according to the spec (https://www.w3.org/TR/uievents/#event-type-compositionend), we should fire 'compositionend' after all IME controls such as inserting text and moving caret. But we are resetting caret position after 'compositionend' event listeners now. This CL will make the order correct. BUG=663944 TEST=run_webkit_layout_test --gtest_filter=InputMethodControllerTest.* Committed: https://crrev.com/df699ce3ffd28396d0447fbc8072ebe7199857ca Cr-Commit-Position: refs/heads/master@{#433308} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/df699ce3ffd28396d0447fbc8072ebe7199857ca Cr-Commit-Position: refs/heads/master@{#433308}
Message was sent while issue was closed.
https://codereview.chromium.org/2493703002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/2493703002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/InputMethodController.cpp:640: return dispatchCompositionEndEvent(frame(), text); This was reverted due to http://crbug.com/674295. Not sure we want to reland (rather we might change the spec to match the reality that implementations rely on current ordering), but if we ever do, note that this change here is also incorrect. It should be in "if (hasComposition())" block, because otherwise it's sending a CompositionEnd without a corresponding CompositionBegin. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
