Trigger compositionstart/end on commitText

Trigger compositionstart/end on commitText

On Android, OSK may not accompany keycode when composing text.
Currently, when OSK calls InputConnection#commitText(), web developers can
only see key events for COMPOSITION_KEY_CODE without the delta change.

By replacing commitText() by setComposingText() and finishComposingText(),
we can notify developers of this delta change.

BUG=577967

[Changwan Ryu, 2016-01-15 09:10:07]

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

[Changwan Ryu, 2016-01-15 09:10:07]

Alex, could you take a look? I don't think we need to bring back the best guess
algorithm.

[aelias_OOO_until_Jul13, 2016-01-16 01:48:50]

I think we should bring back guess algorithm (uploaded in
https://codereview.chromium.org/1583283008, I explained there why), and not
necessarily aim to cherry-pick this one to 48, which should allow us to be more
comfortable with making changes that affect all platforms here.

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

https://codereview.chromium.org/1593533002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:85: static void
dispatchCompositionEvent(LocalFrame& frame, const AtomicString& type, const
String& text)
nit: Could you position this method where the previous one was in the file, to
make the diff more readable?

https://codereview.chromium.org/1593533002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:94:
CompositionEvent::create(EventTypeNames::compositionend, frame.domWindow(),
text);
Looks like you intended to use "type" here, instead of still hardcoding
EventTypeNames::compositionend.

https://codereview.chromium.org/1593533002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:140: #if
OS(ANDROID)
Let's avoid introducing any OS(ANDROID) in Blink.

https://codereview.chromium.org/1593533002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:192:
editor().insertText(text, 0);
Hmm, wouldn't it work just as well to add the compositionstart and
compositionend events around this, and not make any of the code-flow changes?

Currently, the method confirmCompositionOrInsertText() is defined to either
confirm an ongoing composition or insert text, whereas confirmComposition() only
confirms ongoing compositions and does nothing otherwise.  This patch muddles
their roles, and I'm afraid it may have some unintended consequences such as
spamming unnecessary compositionStart/compositionEnd when the text didn't
change.

[Changwan Ryu, 2016-01-19 07:57:59]

On 2016/01/16 01:48:50, aelias wrote:
> I think we should bring back guess algorithm (uploaded in
> https://codereview.chromium.org/1583283008, I explained there why), and not
> necessarily aim to cherry-pick this one to 48, which should allow us to be
more
> comfortable with making changes that affect all platforms here.
> 
>
https://codereview.chromium.org/1593533002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right):
> 
>
https://codereview.chromium.org/1593533002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/editing/InputMethodController.cpp:85: static
void
> dispatchCompositionEvent(LocalFrame& frame, const AtomicString& type, const
> String& text)
> nit: Could you position this method where the previous one was in the file, to
> make the diff more readable?
> 
>
https://codereview.chromium.org/1593533002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/editing/InputMethodController.cpp:94:
> CompositionEvent::create(EventTypeNames::compositionend, frame.domWindow(),
> text);
> Looks like you intended to use "type" here, instead of still hardcoding
> EventTypeNames::compositionend.
> 
>
https://codereview.chromium.org/1593533002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/editing/InputMethodController.cpp:140: #if
> OS(ANDROID)
> Let's avoid introducing any OS(ANDROID) in Blink.
> 
>
https://codereview.chromium.org/1593533002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/editing/InputMethodController.cpp:192:
> editor().insertText(text, 0);
> Hmm, wouldn't it work just as well to add the compositionstart and
> compositionend events around this, and not make any of the code-flow changes?
> 
> Currently, the method confirmCompositionOrInsertText() is defined to either
> confirm an ongoing composition or insert text, whereas confirmComposition()
only
> confirms ongoing compositions and does nothing otherwise.  This patch muddles
> their roles, and I'm afraid it may have some unintended consequences such as
> spamming unnecessary compositionStart/compositionEnd when the text didn't
> change.

Another thing I can try is to completely replace commitText as

beginBatchEdit();
boolean result = setComposingText() && finishComposingText();
endBatchEdit();
return result;

This way I don't have to muddle blink at all. What do you think?

[Changwan Ryu, 2016-01-19 09:28:06]

Description was changed from

==========
Trigger compositionstart/end on commitText

On Android, OSK may not accompany keycode when composing text.
Currently, when OSK calls InputConnection#commitText(), web developers can
only see key events for COMPOSITION_KEY_CODE without the delta change.

By changing confirmComposition to send compositionstart / -end, we can
notify developers of this delta change.

Note that the change is intentionally minimized since it's targeting M48.

BUG=577967
==========

to

==========
Trigger compositionstart/end on commitText

On Android, OSK may not accompany keycode when composing text.
Currently, when OSK calls InputConnection#commitText(), web developers can
only see key events for COMPOSITION_KEY_CODE without the delta change.

By replacing commitText() by setComposingText() and finishComposingText(),
we can notify developers of this delta change.

BUG=577967
==========

[Changwan Ryu, 2016-01-19 09:29:49]

On 2016/01/19 07:57:59, Changwan Ryu wrote:
> On 2016/01/16 01:48:50, aelias wrote:
> > I think we should bring back guess algorithm (uploaded in
> > https://codereview.chromium.org/1583283008, I explained there why), and not
> > necessarily aim to cherry-pick this one to 48, which should allow us to be
> more
> > comfortable with making changes that affect all platforms here.
> > 
> >
>
https://codereview.chromium.org/1593533002/diff/1/third_party/WebKit/Source/c...
> > File third_party/WebKit/Source/core/editing/InputMethodController.cpp
(right):
> > 
> >
>
https://codereview.chromium.org/1593533002/diff/1/third_party/WebKit/Source/c...
> > third_party/WebKit/Source/core/editing/InputMethodController.cpp:85: static
> void
> > dispatchCompositionEvent(LocalFrame& frame, const AtomicString& type, const
> > String& text)
> > nit: Could you position this method where the previous one was in the file,
to
> > make the diff more readable?
> > 
> >
>
https://codereview.chromium.org/1593533002/diff/1/third_party/WebKit/Source/c...
> > third_party/WebKit/Source/core/editing/InputMethodController.cpp:94:
> > CompositionEvent::create(EventTypeNames::compositionend, frame.domWindow(),
> > text);
> > Looks like you intended to use "type" here, instead of still hardcoding
> > EventTypeNames::compositionend.
> > 
> >
>
https://codereview.chromium.org/1593533002/diff/1/third_party/WebKit/Source/c...
> > third_party/WebKit/Source/core/editing/InputMethodController.cpp:140: #if
> > OS(ANDROID)
> > Let's avoid introducing any OS(ANDROID) in Blink.
> > 
> >
>
https://codereview.chromium.org/1593533002/diff/1/third_party/WebKit/Source/c...
> > third_party/WebKit/Source/core/editing/InputMethodController.cpp:192:
> > editor().insertText(text, 0);
> > Hmm, wouldn't it work just as well to add the compositionstart and
> > compositionend events around this, and not make any of the code-flow
changes?
> > 
> > Currently, the method confirmCompositionOrInsertText() is defined to either
> > confirm an ongoing composition or insert text, whereas confirmComposition()
> only
> > confirms ongoing compositions and does nothing otherwise.  This patch
muddles
> > their roles, and I'm afraid it may have some unintended consequences such as
> > spamming unnecessary compositionStart/compositionEnd when the text didn't
> > change.
> 
> Another thing I can try is to completely replace commitText as
> 
> beginBatchEdit();
> boolean result = setComposingText() && finishComposingText();
> endBatchEdit();
> return result;
> 
> This way I don't have to muddle blink at all. What do you think?

I just tried it and uploaded a new patch...

By the way, I also tried the original approach here:
https://codereview.chromium.org/1600253003/
I got some layouttests failures as expected.

Please advise.

[aelias_OOO_until_Jul13, 2016-01-19 21:32:56]

lgtm

[aelias_OOO_until_Jul13, 2016-01-19 22:14:07]

On second thought, not lgtm.

I don't like how this makes us send 2 IPCs for a single action.  It's bad for
performance and there could be bugs like a flicker of underline.

I also remembered there are new editing events "beforeedit" and "edit" defined
in https://www.w3.org/TR/DOM-Level-3-Events/.  They have a field "EditEvent
data" containing the updated text.  After that spec is implemented, the
recommended best practice will become to listen to those events, not keyPress
and compositionUpdate.  So we don't need to go out of our way to supply
compositionUpdate events after all.

[Changwan Ryu, 2016-01-20 00:28:04]

On 2016/01/19 22:14:07, aelias wrote:
> On second thought, not lgtm.
> 
> I don't like how this makes us send 2 IPCs for a single action.  It's bad for
> performance and there could be bugs like a flicker of underline.
> 
> I also remembered there are new editing events "beforeedit" and "edit" defined
> in https://www.w3.org/TR/DOM-Level-3-Events/.  They have a field "EditEvent
> data" containing the updated text.  After that spec is implemented, the
> recommended best practice will become to listen to those events, not keyPress
> and compositionUpdate.  So we don't need to go out of our way to supply
> compositionUpdate events after all.

Makes sense. We don't need to trigger compositionupdate in that case.