[IME] Fire 'compositionend' after 'textInput' event and all other DOM updates

[IME] Fire 'compositionend' after 'textInput' event and all other DOM updates

Before CL Blink/Webkit fires 'textInput' after 'compositionend', and 'textInput's default handler will modify DOM. (violates spec)

This CL cleans up code about events during composition, and only fire
'compositionend' event after 'textInput' event and all other DOM updates.

This CL follows option b) in BUG=41945
Potential interop issue:
Safari and Edge still fires 'textInput' after 'compositionend', so it might affect websites that gets IME input using shadow elements.

Alternative Approach (option c in the bug description):
Preserve the order but don't modify DOM in 'textInput' default handler. (matches Edge, but will affect websites with UA check)

PS: There is another issue about Chinese IBus IME on Linux where the order seems non-fixable, will create a separate issue.

SPEC=https://w3c.github.io/uievents/#compositionend
BUG=41945
Committed: https://crrev.com/afce9d93e76f2ff81baaa088a4ea25f67d1a76b3
Cr-Commit-Position: refs/heads/master@{#395930}

[chongz, 2016-05-19 21:31:39]

Description was changed from

==========
[IME] Fire 'compositionend' after 'textInput' event and all other DOM updates

This CL cleans up code about events during composition, and only fire
'compositionend' event after all DOM updates are finished.

Before CL blink sends 'compositionend' event before last DOM update (as per
old spec for TextEvent), after CL blink will match Firefox and Edge.

Note: There is an issue about Chinese IBus IME on Linux where the order seems
non-fixable.

BUG=41945
==========

to

==========
[IME] Fire 'compositionend' after 'textInput' event and all other DOM updates

This CL cleans up code about events during composition, and only fire
'compositionend' event after 'textInput' event and all other DOM updates are
finished.

This CL follows option b) in crbug.com/41945.

Potential interop issue:
Safari and Edge still fires 'textInput' after 'compositionend', so it might
affect websites that gets IME input using shadow elements. 

Alternative Approach (option c in the bug description):
Preserve the order but don't modify DOM in 'textInput' default handler. (matches
Edge, but will affect websites with UA check)

Note: There is an issue about Chinese IBus IME on Linux where the order seems
non-fixable, will create a separate issue.

SPEC=https://w3c.github.io/uievents/#compositionend
BUG=41945
==========

[chongz, 2016-05-19 21:32:00]

Description was changed from

==========
[IME] Fire 'compositionend' after 'textInput' event and all other DOM updates

This CL cleans up code about events during composition, and only fire
'compositionend' event after 'textInput' event and all other DOM updates are
finished.

This CL follows option b) in crbug.com/41945.

Potential interop issue:
Safari and Edge still fires 'textInput' after 'compositionend', so it might
affect websites that gets IME input using shadow elements. 

Alternative Approach (option c in the bug description):
Preserve the order but don't modify DOM in 'textInput' default handler. (matches
Edge, but will affect websites with UA check)

Note: There is an issue about Chinese IBus IME on Linux where the order seems
non-fixable, will create a separate issue.

SPEC=https://w3c.github.io/uievents/#compositionend
BUG=41945
==========

to

==========
[IME] Fire 'compositionend' after 'textInput' event and all other DOM updates

This CL cleans up code about events during composition, and only fire
'compositionend' event after 'textInput' event and all other DOM updates.

This CL follows option b) in crbug.com/41945.

Potential interop issue:
Safari and Edge still fires 'textInput' after 'compositionend', so it might
affect websites that gets IME input using shadow elements. 

Alternative Approach (option c in the bug description):
Preserve the order but don't modify DOM in 'textInput' default handler. (matches
Edge, but will affect websites with UA check)

Note: There is an issue about Chinese IBus IME on Linux where the order seems
non-fixable, will create a separate issue.

SPEC=https://w3c.github.io/uievents/#compositionend
BUG=41945
==========

[chongz, 2016-05-19 21:32:18]

Description was changed from

==========
[IME] Fire 'compositionend' after 'textInput' event and all other DOM updates

This CL cleans up code about events during composition, and only fire
'compositionend' event after 'textInput' event and all other DOM updates.

This CL follows option b) in crbug.com/41945.

Potential interop issue:
Safari and Edge still fires 'textInput' after 'compositionend', so it might
affect websites that gets IME input using shadow elements. 

Alternative Approach (option c in the bug description):
Preserve the order but don't modify DOM in 'textInput' default handler. (matches
Edge, but will affect websites with UA check)

Note: There is an issue about Chinese IBus IME on Linux where the order seems
non-fixable, will create a separate issue.

SPEC=https://w3c.github.io/uievents/#compositionend
BUG=41945
==========

to

==========
[IME] Fire 'compositionend' after 'textInput' event and all other DOM updates

This CL cleans up code about events during composition, and only fire
'compositionend' event after 'textInput' event and all other DOM updates.

This CL follows option b) in BUG=41945.

Potential interop issue:
Safari and Edge still fires 'textInput' after 'compositionend', so it might
affect websites that gets IME input using shadow elements. 

Alternative Approach (option c in the bug description):
Preserve the order but don't modify DOM in 'textInput' default handler. (matches
Edge, but will affect websites with UA check)

Note: There is an issue about Chinese IBus IME on Linux where the order seems
non-fixable, will create a separate issue.

SPEC=https://w3c.github.io/uievents/#compositionend
BUG=41945
==========

[chongz, 2016-05-19 21:36:21]

Description was changed from

==========
[IME] Fire 'compositionend' after 'textInput' event and all other DOM updates

This CL cleans up code about events during composition, and only fire
'compositionend' event after 'textInput' event and all other DOM updates.

This CL follows option b) in BUG=41945.

Potential interop issue:
Safari and Edge still fires 'textInput' after 'compositionend', so it might
affect websites that gets IME input using shadow elements. 

Alternative Approach (option c in the bug description):
Preserve the order but don't modify DOM in 'textInput' default handler. (matches
Edge, but will affect websites with UA check)

Note: There is an issue about Chinese IBus IME on Linux where the order seems
non-fixable, will create a separate issue.

SPEC=https://w3c.github.io/uievents/#compositionend
BUG=41945
==========

to

==========
[IME] Fire 'compositionend' after 'textInput' event and all other DOM updates

Before CL Blink/Webkit fires 'textInput' after 'compositionend', and
'textInput's default handler will modify DOM. (violates spec)

This CL cleans up code about events during composition, and only fire
'compositionend' event after 'textInput' event and all other DOM updates.

This CL follows option b) in BUG=41945.

Potential interop issue:
Safari and Edge still fires 'textInput' after 'compositionend', so it might
affect websites that gets IME input using shadow elements. 

Alternative Approach (option c in the bug description):
Preserve the order but don't modify DOM in 'textInput' default handler. (matches
Edge, but will affect websites with UA check)

Note: There is an issue about Chinese IBus IME on Linux where the order seems
non-fixable, will create a separate issue.

SPEC=https://w3c.github.io/uievents/#compositionend
BUG=41945
==========

[chongz, 2016-05-19 21:38:01]

Description was changed from

==========
[IME] Fire 'compositionend' after 'textInput' event and all other DOM updates

Before CL Blink/Webkit fires 'textInput' after 'compositionend', and
'textInput's default handler will modify DOM. (violates spec)

This CL cleans up code about events during composition, and only fire
'compositionend' event after 'textInput' event and all other DOM updates.

This CL follows option b) in BUG=41945.

Potential interop issue:
Safari and Edge still fires 'textInput' after 'compositionend', so it might
affect websites that gets IME input using shadow elements. 

Alternative Approach (option c in the bug description):
Preserve the order but don't modify DOM in 'textInput' default handler. (matches
Edge, but will affect websites with UA check)

Note: There is an issue about Chinese IBus IME on Linux where the order seems
non-fixable, will create a separate issue.

SPEC=https://w3c.github.io/uievents/#compositionend
BUG=41945
==========

to

==========
[IME] Fire 'compositionend' after 'textInput' event and all other DOM updates

Before CL Blink/Webkit fires 'textInput' after 'compositionend', and
'textInput's default handler will modify DOM. (violates spec)

This CL cleans up code about events during composition, and only fire
'compositionend' event after 'textInput' event and all other DOM updates.

This CL follows option b) in BUG=41945

Potential interop issue:
Safari and Edge still fires 'textInput' after 'compositionend', so it might
affect websites that gets IME input using shadow elements. 

Alternative Approach (option c in the bug description):
Preserve the order but don't modify DOM in 'textInput' default handler. (matches
Edge, but will affect websites with UA check)

PS: There is another issue about Chinese IBus IME on Linux where the order seems
non-fixable, will create a separate issue.

SPEC=https://w3c.github.io/uievents/#compositionend
BUG=41945
==========

[chongz, 2016-05-19 21:44:18]

chongz@chromium.org changed reviewers:
+ yosin@chromium.org

[chongz, 2016-05-19 21:44:19]

yosin@ can you take a look at this CL please? Thanks!

Basically what I did is moving |dispatchCompositionEnd()| down and surrounding
last edit action with 'beforeinput' and 'compositionupdate'.

https://codereview.chromium.org/1998783002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right):

https://codereview.chromium.org/1998783002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:96: return;
When would event handler destroy document...? I tested using Javascript but it
says I can only close window I created.

[yosin_UTC9, 2016-05-20 07:58:12]

https://codereview.chromium.org/1998783002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right):

https://codereview.chromium.org/1998783002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:77:
CHECK(compositionType ==
TypingCommand::TextCompositionType::TextCompositionUpdate || compositionType ==
TypingCommand::TextCompositionType::TextCompositionConfirm);
Please add "<< compositionType" for ease of debugging.

https://codereview.chromium.org/1998783002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:357:
CHECK(!text.isEmpty());
Do we really need to use |CHECK()|?

https://codereview.chromium.org/1998783002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:358:
CHECK(frame().document());
This CHECK() makes L364 redundant. Which one should we leave?

[chongz, 2016-05-20 17:44:48]

yosin@ I've updated CL based on your comments, PTAL, thanks!

Also I'm not sure how large the interop issue it will cause, do we need to send
an intent-to-ship for this change?

And which option in the issue page (https://crbug.com/41945) would you prefer?
(Maybe b or c? I'm matching b).

https://codereview.chromium.org/1998783002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right):

https://codereview.chromium.org/1998783002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:77:
CHECK(compositionType ==
TypingCommand::TextCompositionType::TextCompositionUpdate || compositionType ==
TypingCommand::TextCompositionType::TextCompositionConfirm);
On 2016/05/20 07:58:12, Yosi_UTC9 wrote:
> Please add "<< compositionType" for ease of debugging.

Done. Also changed to DCHECK().

https://codereview.chromium.org/1998783002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:357:
CHECK(!text.isEmpty());
On 2016/05/20 07:58:12, Yosi_UTC9 wrote:
> Do we really need to use |CHECK()|?

Probably overkill since I already filtered it out. I was trying to make it clear
that empty text means something else and shouldn't go into here.

https://codereview.chromium.org/1998783002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:358:
CHECK(frame().document());
On 2016/05/20 07:58:12, Yosi_UTC9 wrote:
> This CHECK() makes L364 redundant. Which one should we leave?

L364 was trying to handle the case you mentioned before, where document might
got destroyed by event handler. However I cannot reproduce it using
Javascript... (I'm using "myWindow.close()")

Can I have more information on how to reproduce that? Thanks! (Also see L96)

[yosin_UTC9, 2016-05-23 04:57:28]

lgtm

https://codereview.chromium.org/1998783002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right):

https://codereview.chromium.org/1998783002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:357:
CHECK(!text.isEmpty());
On 2016/05/20 at 17:44:48, chongz wrote:
> On 2016/05/20 07:58:12, Yosi_UTC9 wrote:
> > Do we really need to use |CHECK()|?
> 
> Probably overkill since I already filtered it out. I was trying to make it
clear that empty text means something else and shouldn't go into here.
I supposed to use |DCHECK()| instead of |CHECK()|. Having |DCHECK()| makes debug
code slow, but it provide some information.

https://codereview.chromium.org/1998783002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:358:
CHECK(frame().document());
On 2016/05/20 at 17:44:48, chongz wrote:
> On 2016/05/20 07:58:12, Yosi_UTC9 wrote:
> > This CHECK() makes L364 redundant. Which one should we leave?
> 
> L364 was trying to handle the case you mentioned before, where document might
got destroyed by event handler. However I cannot reproduce it using
Javascript... (I'm using "myWindow.close()")
> 
> Can I have more information on how to reproduce that? Thanks! (Also see L96)

ClusterFuzz will find test case. ;-)

[chongz, 2016-05-25 02:47:39]

Patchset #3 (id:40001) has been deleted

[chongz, 2016-05-25 05:40:08]

chongz@chromium.org changed reviewers:
+ changwan@chromium.org

[chongz, 2016-05-25 05:41:30]

changwan@ can you take a look at ImeTest.java please? Thanks!

[Changwan Ryu, 2016-05-25 05:55:27]

Imetest.java lgtm

[chongz, 2016-05-25 13:59:11]

The CQ bit was checked by chongz@chromium.org

[chongz, 2016-05-25 13:59:12]

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/1998783002/#ps60001
(title: "Add DCHECK for empty text, fix ImeTest.java")

[commit-bot: I haz the power, 2016-05-25 13:59:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1998783002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1998783002/60001

[commit-bot: I haz the power, 2016-05-25 18:03:07]

Description was changed from

==========
[IME] Fire 'compositionend' after 'textInput' event and all other DOM updates

Before CL Blink/Webkit fires 'textInput' after 'compositionend', and
'textInput's default handler will modify DOM. (violates spec)

This CL cleans up code about events during composition, and only fire
'compositionend' event after 'textInput' event and all other DOM updates.

This CL follows option b) in BUG=41945

Potential interop issue:
Safari and Edge still fires 'textInput' after 'compositionend', so it might
affect websites that gets IME input using shadow elements. 

Alternative Approach (option c in the bug description):
Preserve the order but don't modify DOM in 'textInput' default handler. (matches
Edge, but will affect websites with UA check)

PS: There is another issue about Chinese IBus IME on Linux where the order seems
non-fixable, will create a separate issue.

SPEC=https://w3c.github.io/uievents/#compositionend
BUG=41945
==========

to

==========
[IME] Fire 'compositionend' after 'textInput' event and all other DOM updates

Before CL Blink/Webkit fires 'textInput' after 'compositionend', and
'textInput's default handler will modify DOM. (violates spec)

This CL cleans up code about events during composition, and only fire
'compositionend' event after 'textInput' event and all other DOM updates.

This CL follows option b) in BUG=41945

Potential interop issue:
Safari and Edge still fires 'textInput' after 'compositionend', so it might
affect websites that gets IME input using shadow elements. 

Alternative Approach (option c in the bug description):
Preserve the order but don't modify DOM in 'textInput' default handler. (matches
Edge, but will affect websites with UA check)

PS: There is another issue about Chinese IBus IME on Linux where the order seems
non-fixable, will create a separate issue.

SPEC=https://w3c.github.io/uievents/#compositionend
BUG=41945
==========

[commit-bot: I haz the power, 2016-05-25 18:03:08]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-05-25 18:04:53]

Description was changed from

==========
[IME] Fire 'compositionend' after 'textInput' event and all other DOM updates

Before CL Blink/Webkit fires 'textInput' after 'compositionend', and
'textInput's default handler will modify DOM. (violates spec)

This CL cleans up code about events during composition, and only fire
'compositionend' event after 'textInput' event and all other DOM updates.

This CL follows option b) in BUG=41945

Potential interop issue:
Safari and Edge still fires 'textInput' after 'compositionend', so it might
affect websites that gets IME input using shadow elements. 

Alternative Approach (option c in the bug description):
Preserve the order but don't modify DOM in 'textInput' default handler. (matches
Edge, but will affect websites with UA check)

PS: There is another issue about Chinese IBus IME on Linux where the order seems
non-fixable, will create a separate issue.

SPEC=https://w3c.github.io/uievents/#compositionend
BUG=41945
==========

to

==========
[IME] Fire 'compositionend' after 'textInput' event and all other DOM updates

Before CL Blink/Webkit fires 'textInput' after 'compositionend', and
'textInput's default handler will modify DOM. (violates spec)

This CL cleans up code about events during composition, and only fire
'compositionend' event after 'textInput' event and all other DOM updates.

This CL follows option b) in BUG=41945

Potential interop issue:
Safari and Edge still fires 'textInput' after 'compositionend', so it might
affect websites that gets IME input using shadow elements.

Alternative Approach (option c in the bug description):
Preserve the order but don't modify DOM in 'textInput' default handler. (matches
Edge, but will affect websites with UA check)

PS: There is another issue about Chinese IBus IME on Linux where the order seems
non-fixable, will create a separate issue.

SPEC=https://w3c.github.io/uievents/#compositionend
BUG=41945

Committed: https://crrev.com/afce9d93e76f2ff81baaa088a4ea25f67d1a76b3
Cr-Commit-Position: refs/heads/master@{#395930}
==========

[commit-bot: I haz the power, 2016-05-25 18:04:54]

Patchset 3 (id:??) landed as
https://crrev.com/afce9d93e76f2ff81baaa088a4ea25f67d1a76b3
Cr-Commit-Position: refs/heads/master@{#395930}