New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 633840 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Aug 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Bug

Blocking:
issue 551193



Sign in to add a comment

ImeTest#testCommitEnterKeyWhileComposingText fails when ImeThread is enabled

Project Member Reported by changwan@chromium.org, Aug 3 2016

Issue description

Version: development version
OS: Android

This seems to have been caused by https://codereview.chromium.org/2020973002/ ,
assigning to yabinh@.

Setting pri=1 since this can be a user-facing bug.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Aug 9 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/786070a33bcfebaff78b304401a4137e8dadccd8

commit 786070a33bcfebaff78b304401a4137e8dadccd8
Author: yabinh <yabinh@chromium.org>
Date: Tue Aug 09 04:59:03 2016

Change InputMethodController#setSelectionOffsets() to use
NotUserTriggered parameter

InputMethodController#setSelectionOffsets() should have no side
effect other than changing the selection. But
FrameSelection::CloseTyping will close typing, causing the failure of
canceling the composing text when inserting line break. We should
use NotUserTriggered parameter because it has no side effect.

BUG= 633840 

Review-Url: https://codereview.chromium.org/2206923002
Cr-Commit-Position: refs/heads/master@{#410577}

[modify] https://crrev.com/786070a33bcfebaff78b304401a4137e8dadccd8/third_party/WebKit/Source/core/editing/InputMethodController.cpp
[modify] https://crrev.com/786070a33bcfebaff78b304401a4137e8dadccd8/third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp

Now there is another failure when ImeThread is turned on:
https://codereview.chromium.org/2202853004/

The failure is
org.chromium.content.browser.input.ImeTest#testFinishComposingText

Could this be related to the new CL?
Cc: yosin@chromium.org yabinh@chromium.org
Yes, it's related to that CL.:(

It seems that we should close typing after confirming text. However, in InputMethodController::confirmCompositionOrInsertText, we call InputMethodController::setSelectionOffsets (in the destructor of SelectionOffsetsScope), which uses NotUserTriggered instead of FrameSelection::CloseTyping.

We can fix that by passing a SetSelectionOptions parameter to InputMethodController::setSelectionOffsets.

yosin@, what do you think?
Project Member

Comment 4 by bugdroid1@chromium.org, Aug 10 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/dd122a409bd085efd4a566ea950755bdd60d2866

commit dd122a409bd085efd4a566ea950755bdd60d2866
Author: yabinh <yabinh@chromium.org>
Date: Wed Aug 10 00:52:25 2016

Revert of Change InputMethodController#setSelectionOffsets() to use  NotUserTriggered parameter (patchset #3 id:40001 of https://codereview.chromium.org/2206923002/ )

Reason for revert:
ImeTest#testFinishComposingText failed.
We need to fix the test before relanding it.

Original issue's description:
> Change InputMethodController#setSelectionOffsets() to use
> NotUserTriggered parameter
>
> InputMethodController#setSelectionOffsets() should have no side
> effect other than changing the selection. But
> FrameSelection::CloseTyping will close typing, causing the failure of
> canceling the composing text when inserting line break. We should
> use NotUserTriggered parameter because it has no side effect.
>
> BUG= 633840 
>
> Committed: https://crrev.com/786070a33bcfebaff78b304401a4137e8dadccd8
> Cr-Commit-Position: refs/heads/master@{#410577}

TBR=yosin@chromium.org,aelias@chromium.org,changwan@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 633840 

Review-Url: https://codereview.chromium.org/2227333002
Cr-Commit-Position: refs/heads/master@{#410905}

[modify] https://crrev.com/dd122a409bd085efd4a566ea950755bdd60d2866/third_party/WebKit/Source/core/editing/InputMethodController.cpp
[modify] https://crrev.com/dd122a409bd085efd4a566ea950755bdd60d2866/third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp

Status: Fixed (was: Assigned)
closing as the regression CL got reverted.
Project Member

Comment 6 by bugdroid1@chromium.org, Aug 23 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/dd6d1b62de7b449d997d159d63207abe87d3cb46

commit dd6d1b62de7b449d997d159d63207abe87d3cb46
Author: yabinh <yabinh@chromium.org>
Date: Tue Aug 23 07:09:02 2016

Reland: Fix setComposingText with empty text when newCursorPosition != 1

setComposingText("", newCursorPosition) should cancel the
composition (if any), and move the cursor according to
|newCursorPosition|.

Note that we shouldn't close typing when moving the cursor in this
case. In other cases, like in
InputMethodController::confirmCompositionOrInsertText, we should
close typing when moving the cursor. Thus, we need an extra parameter.

This is a reland CL. The previous CL caused a bug
 crbug.com/633840 .

BUG= 570920 ,  633840 

Review-Url: https://codereview.chromium.org/2020973002
Cr-Commit-Position: refs/heads/master@{#413683}

[modify] https://crrev.com/dd6d1b62de7b449d997d159d63207abe87d3cb46/components/test_runner/text_input_controller.cc
[modify] https://crrev.com/dd6d1b62de7b449d997d159d63207abe87d3cb46/content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java
[modify] https://crrev.com/dd6d1b62de7b449d997d159d63207abe87d3cb46/third_party/WebKit/Source/core/editing/InputMethodController.cpp
[modify] https://crrev.com/dd6d1b62de7b449d997d159d63207abe87d3cb46/third_party/WebKit/Source/core/editing/InputMethodController.h
[modify] https://crrev.com/dd6d1b62de7b449d997d159d63207abe87d3cb46/third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp

Sign in to add a comment