Confirm the compostion text on TAP gesture.

Confirm the compostion text on TAP gesture.

The problem is that the editor is not activated when
tapping on the the second input box if the compostion
exists in first input box.

The patch is to confirm the compostion text on TAP gesture,
to make sure the input caret won't be moved with an ongoing
composition text.

BUG=467365
R=jdduke@chromium.org, rbyers@chromium.org, donnd@chromium.org

[jdduke (slow), 2015-03-16 15:11:31]

https://codereview.chromium.org/1011523002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/1011523002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_android.cc:1750:
FinishImeCompositionSession();
+aurimas.

Why is this behavior Android-specific? Also, is there anything special about a
tap here vs, say, a click? Could this be done at a lower level (e.g., if we see
a tap event in the render process, can we do the right thing there?).

[Miyoung Shin, 2015-03-17 00:40:35]

https://codereview.chromium.org/1011523002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/1011523002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_android.cc:1750:
FinishImeCompositionSession();
On 2015/03/16 15:11:31, jdduke wrote:
> +aurimas.
> 
> Why is this behavior Android-specific? Also, is there anything special about a
> tap here vs, say, a click? Could this be done at a lower level (e.g., if we
see
> a tap event in the render process, can we do the right thing there?).

I tried to have the consistent behavior with aura port.

render_widget_host_view_aura.cc
void RenderWidgetHostViewAura::OnMouseEvent(ui::MouseEvent* event) {
...
    if (event->type() == ui::ET_MOUSE_PRESSED)
      FinishImeCompositionSession();

void RenderWidgetHostViewAura::OnGestureEvent(ui::GestureEvent* event) {
...
  if (event->type() == ui::ET_GESTURE_TAP)
    FinishImeCompositionSession();

If we do it in the render process, we would remove the above code. WDYT ?

[jdduke (slow), 2015-03-17 18:47:21]

jdduke@chromium.org changed reviewers:
+ aurimas@chromium.org

[jdduke (slow), 2015-03-17 18:47:21]

+aurimas for IME-specific comments.

Ideally this would be in the renderer, yes, but I can't really speak to what
that should look like. If we can't move it there (sad face), we should at least
be consistent with our behavior here (i.e., do the same thing on click/tap).

https://codereview.chromium.org/1011523002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/1011523002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_android.cc:1937: if
(host_) {
So, the different with RWHVAura is that hey actually track
|has_composition_text_|, avoiding the IPC if they don't need it. Perhaps there's
an ImeAdapterAndroid method we can query? We might as well add this for mouse
click as well I suppose?

[Miyoung Shin, 2015-03-18 16:44:27]

https://codereview.chromium.org/1011523002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/1011523002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_android.cc:1937: if
(host_) {
On 2015/03/17 18:47:21, jdduke wrote:
> So, the different with RWHVAura is that hey actually track
> |has_composition_text_|, avoiding the IPC if they don't need it. Perhaps
there's
> an ImeAdapterAndroid method we can query? We might as well add this for mouse
> click as well I suppose?

I've uploaded the patch to add hasComposition method in ImeAdapterAndroid as
|has_composition_text_|.

[jdduke (slow), 2015-03-18 19:50:08]

https://codereview.chromium.org/1011523002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/ime_adapter_android.cc (right):

https://codereview.chromium.org/1011523002/diff/20001/content/browser/rendere...
content/browser/renderer_host/ime_adapter_android.cc:230: bool
ImeAdapterAndroid::HasComposition() {
Would it be worth simply moving all the FinishComposition logic here? There's
already a CancelComposition and FinishComposingText here so that should fit in
nicely.

[aurimas (slooooooooow), 2015-03-18 23:21:11]

Let's first have a discussion on the bug (crbug.com/467365) to make sure what
the actual problem is as I cannot even reproduce it.

[aurimas (slooooooooow), 2015-03-19 05:40:30]

https://chromiumcodereview.appspot.com/1011523002/diff/20001/content/browser/...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://chromiumcodereview.appspot.com/1011523002/diff/20001/content/browser/...
content/browser/renderer_host/render_widget_host_view_android.cc:1940:
host_->ImeConfirmComposition(base::string16(), gfx::Range::InvalidRange(),
This will get called even if the user taps inside the same input field, so
composition will be cancelled even when user tries to move the carrot within the
same word. That is not a correct behavior compared to Android's EditText, it
keeps the composition unless cursor moves outside of a single word.

Since we do not know where tap is going to land or if the tap is going to be
preventDefaulted, I don't think we can do it in the browser. This should really
be handled somewhere in the renderer.

[Miyoung Shin, 2015-03-19 13:42:29]

On 2015/03/19 05:40:30, aurimas wrote:
>
https://chromiumcodereview.appspot.com/1011523002/diff/20001/content/browser/...
> File content/browser/renderer_host/render_widget_host_view_android.cc (right):
> 
>
https://chromiumcodereview.appspot.com/1011523002/diff/20001/content/browser/...
> content/browser/renderer_host/render_widget_host_view_android.cc:1940:
> host_->ImeConfirmComposition(base::string16(), gfx::Range::InvalidRange(),
> This will get called even if the user taps inside the same input field, so
> composition will be cancelled even when user tries to move the carrot within
the
> same word. That is not a correct behavior compared to Android's EditText, it
> keeps the composition unless cursor moves outside of a single word.
> 
> Since we do not know where tap is going to land or if the tap is going to be
> preventDefaulted, I don't think we can do it in the browser. This should
really
> be handled somewhere in the renderer.

@aurimas, @jdduke,
Thank you for your kind review. I will investigate how to handle it in the
renderer.

[jdduke (slow), 2015-03-19 15:05:55]

On 2015/03/19 13:42:29, Miyoung Shin wrote:
> @aurimas, @jdduke,
> Thank you for your kind review. I will investigate how to handle it in the
> renderer.

Great, thanks, this may even simplify (or fix) the Aura implementation.