Issue 1995333002: Handle newCursorPosition correctly for Android's commitText()

yabinh

Description was changed from ========== Fix commitText when newCursorPosition != 1 BUG=570920 ========== to ========== ...

4 years, 7 months ago (2016-05-20 11:27:41 UTC) #1

yabinh

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

4 years, 7 months ago (2016-05-20 11:27:41 UTC) #2

dcheng

dcheng@chromium.org changed reviewers: + dcheng@chromium.org

4 years, 7 months ago (2016-05-20 20:07:37 UTC) #4

dcheng

Note that we don't allow default args on virtual functions.

4 years, 7 months ago (2016-05-20 20:07:38 UTC) #5

Changwan Ryu

https://codereview.chromium.org/1995333002/diff/1/third_party/WebKit/Source/core/editing/InputMethodController.cpp File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/1995333002/diff/1/third_party/WebKit/Source/core/editing/InputMethodController.cpp#newcode349 third_party/WebKit/Source/core/editing/InputMethodController.cpp:349: const EphemeralRange& startRange = PlainTextRange(0, start).createRange(*rootEditableElement); There is a ...

4 years, 7 months ago (2016-05-24 08:35:51 UTC) #6

yabinh

Description was changed from ========== commitText doesn't work well when newCursorPosition != 1 When |newCursorPosition| ...

4 years, 5 months ago (2016-06-30 07:11:34 UTC) #7

yabinh

Description was changed from ========== commitText doesn't work well when newCursorPosition != 1 When |newCursorPosition| ...

4 years, 5 months ago (2016-06-30 07:11:50 UTC) #8

yabinh

https://codereview.chromium.org/1995333002/diff/1/third_party/WebKit/Source/web/WebViewImpl.cpp File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/1995333002/diff/1/third_party/WebKit/Source/web/WebViewImpl.cpp#newcode2396 third_party/WebKit/Source/web/WebViewImpl.cpp:2396: newCursorPosition = newCursorPosition + textInputInfo().compositionStart; InputMethodController::m_compositionRange doesn't work for ...

4 years, 5 months ago (2016-07-06 11:44:30 UTC) #9

yabinh

The CQ bit was checked by yabinh@chromium.org to run a CQ dry run

4 years, 4 months ago (2016-08-03 05:07:01 UTC) #10

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1995333002/20001

4 years, 4 months ago (2016-08-03 05:07:16 UTC) #11

yabinh

changwan@, can you take a look of this patch? https://codereview.chromium.org/1995333002/diff/20001/third_party/WebKit/Source/core/editing/InputMethodController.cpp File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/1995333002/diff/20001/third_party/WebKit/Source/core/editing/InputMethodController.cpp#newcode250 third_party/WebKit/Source/core/editing/InputMethodController.cpp:250: ...

4 years, 4 months ago (2016-08-03 05:12:09 UTC) #12

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 4 months ago (2016-08-03 06:38:06 UTC) #13

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 4 months ago (2016-08-03 06:38:08 UTC) #14

Changwan Ryu

Please try to minimize API changes. https://codereview.chromium.org/1995333002/diff/20001/components/test_runner/text_input_controller.cc File components/test_runner/text_input_controller.cc (right): https://codereview.chromium.org/1995333002/diff/20001/components/test_runner/text_input_controller.cc#newcode167 components/test_runner/text_input_controller.cc:167: view()->confirmComposition(1); you don't ...

4 years, 4 months ago (2016-08-03 07:37:30 UTC) #15

yabinh

The CQ bit was checked by yabinh@chromium.org to run a CQ dry run

4 years, 4 months ago (2016-08-08 07:32:37 UTC) #16

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1995333002/40001

4 years, 4 months ago (2016-08-08 07:32:54 UTC) #17

yabinh

changwan@, PTAL, thanks. https://codereview.chromium.org/1995333002/diff/20001/components/test_runner/text_input_controller.cc File components/test_runner/text_input_controller.cc (right): https://codereview.chromium.org/1995333002/diff/20001/components/test_runner/text_input_controller.cc#newcode167 components/test_runner/text_input_controller.cc:167: view()->confirmComposition(1); On 2016/08/03 07:37:29, Changwan Ryu ...

4 years, 4 months ago (2016-08-08 07:33:44 UTC) #18

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 4 months ago (2016-08-08 08:40:49 UTC) #19

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 4 months ago (2016-08-08 08:40:50 UTC) #20

yabinh

The CQ bit was checked by yabinh@chromium.org to run a CQ dry run

4 years, 4 months ago (2016-08-24 01:12:56 UTC) #21

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1995333002/60001

4 years, 4 months ago (2016-08-24 01:13:46 UTC) #22

Changwan Ryu

It seems that we don't have WebViewTest to cover the != 1 cases. Could you ...

4 years, 4 months ago (2016-08-24 01:32:04 UTC) #24

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 4 months ago (2016-08-24 01:44:46 UTC) #25

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_asan_rel_ng/builds/214778)

4 years, 4 months ago (2016-08-24 01:44:47 UTC) #26

yabinh

The CQ bit was checked by yabinh@chromium.org to run a CQ dry run

4 years, 4 months ago (2016-08-24 06:03:19 UTC) #27

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1995333002/80001

4 years, 4 months ago (2016-08-24 06:03:42 UTC) #28

yabinh

On 2016/08/24 01:32:04, Changwan Ryu wrote: > It seems that we don't have WebViewTest to ...

4 years, 4 months ago (2016-08-24 06:04:40 UTC) #29

yabinh

https://codereview.chromium.org/1995333002/diff/60001/content/browser/renderer_host/ime_adapter_android.h File content/browser/renderer_host/ime_adapter_android.h (right): https://codereview.chromium.org/1995333002/diff/60001/content/browser/renderer_host/ime_adapter_android.h#newcode53 content/browser/renderer_host/ime_adapter_android.h:53: int new_cursor_pos = 1); On 2016/08/24 01:32:03, Changwan Ryu ...

4 years, 4 months ago (2016-08-24 06:05:01 UTC) #30

Changwan Ryu

On 2016/08/24 06:05:01, yabinh wrote: > https://codereview.chromium.org/1995333002/diff/60001/content/browser/renderer_host/ime_adapter_android.h > File content/browser/renderer_host/ime_adapter_android.h (right): > > https://codereview.chromium.org/1995333002/diff/60001/content/browser/renderer_host/ime_adapter_android.h#newcode53 > ...

4 years, 4 months ago (2016-08-24 06:48:14 UTC) #31

On 2016/08/24 06:05:01, yabinh wrote:
>
https://codereview.chromium.org/1995333002/diff/60001/content/browser/rendere...
> File content/browser/renderer_host/ime_adapter_android.h (right):
> 
>
https://codereview.chromium.org/1995333002/diff/60001/content/browser/rendere...
> content/browser/renderer_host/ime_adapter_android.h:53: int new_cursor_pos =
1);
> On 2016/08/24 01:32:03, Changwan Ryu wrote:
> > this is probably not needed?
> 
> Done.
> 
>
https://codereview.chromium.org/1995333002/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right):
> 
>
https://codereview.chromium.org/1995333002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/editing/InputMethodController.cpp:244: //
> According to Android documentation, the new cursor position is
> On 2016/08/24 01:32:03, Changwan Ryu wrote:
> > This comment is platform-specific. Could you make it more generic?
> 
> Done.

I feel that commit description is somewhat vague to non-Android engineers. How
about the following format?

Handle newCursorPosition correctly for Android's commitText()

Android’s InputConnection API specifies a way to commit (confirm) the current
composition
and move the cursor at the same time:
https://developer.android.com/reference/android/view/inputmethod/
InputConnection.html#commitText(java.lang.CharSequence, int)

Currently, commitText doesn't work correctly when newCursorPosition is not 1.
newCursorPosition is a relative value that depends on the location of the text
being committed.

When |newCursorPosition| < 1, the cursor should move to the left of 
the committing text by |newCursorPosition| characters. If the cursor
exceeds the left boundary, it will clamp to  the left boundary.

When |newCursorPosition| >= 1, the cursor will move to the right of 
the committing text by |newCursorPosition - 1| characters. If the cursor
exceeds the right boundary, it will clamp to the right boundary.

aelias_OOO_until_Jul13

Description was changed from ========== commitText doesn't work well when newCursorPosition != 1 When |newCursorPosition| ...

4 years, 4 months ago (2016-08-24 06:59:34 UTC) #32

aelias_OOO_until_Jul13

Description was changed from ========== commitText doesn't work well when newCursorPosition != 1 When |newCursorPosition| ...

4 years, 4 months ago (2016-08-24 07:00:56 UTC) #33

aelias_OOO_until_Jul13

Description was changed from ========== commitText doesn't work well when newCursorPosition != 1 When |newCursorPosition| ...

4 years, 4 months ago (2016-08-24 07:01:05 UTC) #34

aelias_OOO_until_Jul13

Description was changed from ========== commitText doesn't work well when newCursorPosition != 1 When |newCursorPosition| ...

4 years, 4 months ago (2016-08-24 07:01:34 UTC) #35

aelias_OOO_until_Jul13

Description was changed from ========== Fix commitText when newCursorPosition != 1 When |newCursorPosition| < 1, ...

4 years, 4 months ago (2016-08-24 07:02:24 UTC) #36

aelias_OOO_until_Jul13

Description was changed from ========== Handle newCursorPosition correctly for Android's commitText() Android’s InputConnection API specifies ...

4 years, 4 months ago (2016-08-24 07:03:02 UTC) #37

Description was changed from

==========
Handle newCursorPosition correctly for Android's commitText()

Android’s InputConnection API specifies a way to commit (confirm) the current
composition
and move the cursor at the same time:
https://developer.android.com/reference/android/view/inputmethod/
InputConnection.html#commitText(java.lang.CharSequence, int)

Currently, commitText doesn't work correctly when newCursorPosition is not 1.
newCursorPosition is a relative value that depends on the location of the text
being committed.

When |newCursorPosition| < 1, the cursor should move to the left of 
the committing text by |newCursorPosition| characters. If the cursor
exceeds the left boundary, it will clamp to  the left boundary.

When |newCursorPosition| >= 1, the cursor will move to the right of 
the committing text by |newCursorPosition - 1| characters. If the cursor
exceeds the right boundary, it will clamp to the right boundary.

BUG=570920
==========

to

==========
Handle newCursorPosition correctly for Android's commitText()

Android’s InputConnection API specifies a way to commit (confirm) the current
composition
and move the cursor at the same time:
https://developer.android.com/reference/android/view/inputmethod/
InputConnection.html#commitText(java.lang.CharSequence, int)

Currently, commitText doesn't conform to the documentation nor to
Android TextView's behavior when newCursorPosition is not 1.
newCursorPosition is a relative value that depends on the location of the text
being committed.

When |newCursorPosition| < 1, the cursor should move to the left of 
the committing text by |newCursorPosition| characters. If the cursor
exceeds the left boundary, it will clamp to  the left boundary.

When |newCursorPosition| >= 1, the cursor will move to the right of 
the committing text by |newCursorPosition - 1| characters. If the cursor
exceeds the right boundary, it will clamp to the right boundary.

BUG=570920
==========

yabinh

Description was changed from ========== Handle newCursorPosition correctly for Android's commitText() Android’s InputConnection API specifies ...

4 years, 4 months ago (2016-08-24 07:08:55 UTC) #38

Description was changed from

==========
Handle newCursorPosition correctly for Android's commitText()

Android’s InputConnection API specifies a way to commit (confirm) the current
composition
and move the cursor at the same time:
https://developer.android.com/reference/android/view/inputmethod/
InputConnection.html#commitText(java.lang.CharSequence, int)

Currently, commitText doesn't conform to the documentation nor to
Android TextView's behavior when newCursorPosition is not 1.
newCursorPosition is a relative value that depends on the location of the text
being committed.

When |newCursorPosition| < 1, the cursor should move to the left of 
the committing text by |newCursorPosition| characters. If the cursor
exceeds the left boundary, it will clamp to  the left boundary.

When |newCursorPosition| >= 1, the cursor will move to the right of 
the committing text by |newCursorPosition - 1| characters. If the cursor
exceeds the right boundary, it will clamp to the right boundary.

BUG=570920
==========

to

==========
Handle newCursorPosition correctly for Android's commitText()

Android’s InputConnection API specifies a way to commit (confirm) the current
composition
and move the cursor at the same time:
https://developer.android.com/reference/android/view/inputmethod/
InputConnection.html#commitText(java.lang.CharSequence, int)

Currently, commitText doesn't conform to the documentation nor to
Android TextView's behavior when newCursorPosition is not 1.
newCursorPosition is a relative value that depends on the location of the text
being committed.

When |newCursorPosition| < 1, the cursor should move to the left of 
the committing text by |newCursorPosition| characters. If the cursor
exceeds the left boundary, it will clamp to  the left boundary.

When |newCursorPosition| >= 1, the cursor will move to the right of 
the committing text by |newCursorPosition - 1| characters. If the cursor
exceeds the right boundary, it will clamp to the right boundary.

BUG=570920
==========

yabinh

On 2016/08/24 06:48:14, Changwan Ryu wrote: > On 2016/08/24 06:05:01, yabinh wrote: > > > ...

4 years, 4 months ago (2016-08-24 07:16:47 UTC) #39

On 2016/08/24 06:48:14, Changwan Ryu wrote:
> On 2016/08/24 06:05:01, yabinh wrote:
> >
>
https://codereview.chromium.org/1995333002/diff/60001/content/browser/rendere...
> > File content/browser/renderer_host/ime_adapter_android.h (right):
> > 
> >
>
https://codereview.chromium.org/1995333002/diff/60001/content/browser/rendere...
> > content/browser/renderer_host/ime_adapter_android.h:53: int new_cursor_pos =
> 1);
> > On 2016/08/24 01:32:03, Changwan Ryu wrote:
> > > this is probably not needed?
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/1995333002/diff/60001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/editing/InputMethodController.cpp
(right):
> > 
> >
>
https://codereview.chromium.org/1995333002/diff/60001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/editing/InputMethodController.cpp:244: //
> > According to Android documentation, the new cursor position is
> > On 2016/08/24 01:32:03, Changwan Ryu wrote:
> > > This comment is platform-specific. Could you make it more generic?
> > 
> > Done.
> 
> I feel that commit description is somewhat vague to non-Android engineers. How
> about the following format?
> 
> 
> Handle newCursorPosition correctly for Android's commitText()
> 
> Android’s InputConnection API specifies a way to commit (confirm) the current
> composition
> and move the cursor at the same time:
> https://developer.android.com/reference/android/view/inputmethod/
> InputConnection.html#commitText(java.lang.CharSequence, int)
> 
> Currently, commitText doesn't work correctly when newCursorPosition is not 1.
> newCursorPosition is a relative value that depends on the location of the text
> being committed.
> 
> When |newCursorPosition| < 1, the cursor should move to the left of 
> the committing text by |newCursorPosition| characters. If the cursor
> exceeds the left boundary, it will clamp to  the left boundary.
> 
> When |newCursorPosition| >= 1, the cursor will move to the right of 
> the committing text by |newCursorPosition - 1| characters. If the cursor
> exceeds the right boundary, it will clamp to the right boundary.

Yes. It's much better. Thanks.

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 4 months ago (2016-08-24 07:59:41 UTC) #40

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/279768)

4 years, 4 months ago (2016-08-24 07:59:42 UTC) #41

Changwan Ryu

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

4 years, 4 months ago (2016-08-24 08:00:42 UTC) #42

Changwan Ryu

lgtm, but I don't have all the ownership here. yosin@, could you take a look?

4 years, 4 months ago (2016-08-24 08:00:43 UTC) #43

aelias_OOO_until_Jul13

aelias@chromium.org changed reviewers: - yosin@chromium.org

4 years, 4 months ago (2016-08-24 08:13:05 UTC) #44

aelias_OOO_until_Jul13

https://codereview.chromium.org/1995333002/diff/20001/third_party/WebKit/Source/core/editing/InputMethodController.cpp File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/1995333002/diff/20001/third_party/WebKit/Source/core/editing/InputMethodController.cpp#newcode254 third_party/WebKit/Source/core/editing/InputMethodController.cpp:254: newCursorPosition += compositionRange.end(); On 2016/08/08 at 07:33:44, yabinh wrote: ...

4 years, 4 months ago (2016-08-24 08:13:07 UTC) #45

https://codereview.chromium.org/1995333002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right):

https://codereview.chromium.org/1995333002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:254:
newCursorPosition += compositionRange.end();
On 2016/08/08 at 07:33:44, yabinh wrote:
> On 2016/08/03 07:37:30, Changwan Ryu wrote:
> > Why should this differ from the else line?
> 
> There are 2 cases when hasCompositio() == true:
> 
> 
> Case 1. text.length() != 0
> This happens when we call ImeTest#setComposingText("hello",
newCursorPosition), then call ImeTest#commitText("world", newCursorPosition).
> 
> In this case, we should drop the composing "hello", and insert the committing
"world".
> 
> 
> Case 2.text.length() == 0
> This happens in the test RenderViewImplTest#ImeComposition. 
>
(https://cs.chromium.org/chromium/src/content/renderer/render_view_browsertest...)
> [...]
> Here, we mean to confirm the composing text, not to replace it with empty
text.

Acknowledged, but checking "text.length() == 0" is a nonobvious way to make this
distinction.  On Android, there is no such thing as "confirm with no text", so
it sounds like what is going on here is different platform-specific semantics. 
We should check for this more explicitly instead of relying on implicit signals
like "text == 0".  For example, does the enum value "ConfirmCompositionBehavior"
help?  If not, can we add another enum value, or possibly make the other
platforms not run this code at all?

https://codereview.chromium.org/1995333002/diff/80001/content/common/input_me...
File content/common/input_messages.h (right):

https://codereview.chromium.org/1995333002/diff/80001/content/common/input_me...
content/common/input_messages.h:174: int /* new_cursor_pos */)
I realize that you matched the name used in the Android documentation, but I
think a clearer name would be "relative_cursor_pos".  Could you
search-and-replace in your patch to switch to that name?  I think that would
avoid the mistake of assuming it's an absolute offset from the beginning of the
textbox.

https://codereview.chromium.org/1995333002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right):

https://codereview.chromium.org/1995333002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:239: // If
newCursorPosition > 0, it's relative to the end of the text - 1;
Please add a comment that this is to match Android behavior with a link to
https://developer.android.com/reference/android/view/inputmethod/InputConnect...,
int)

https://codereview.chromium.org/1995333002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:253:
newCursorPosition += compositionRange.end();
I read your explanation above that text.length() == 0 effectively means the
current composition is considered the text.  I think this should be expressed
more explicitly, along the lines of:

bool InputMethodController::confirmCompositionOrInsertText(... , int
relativeCursorPosition)

int textStart, textLength, absoluteCursorPosition;
textStart = compositionRange.start() [or getSelectionOffsets().start() ];
textLength = text.length() ? text.length() : compositionRange.length();
absoluteCursorPosition = relativeCursorPosition + textStart - 1;
if (relativeCursorPosition > 0) absoluteCursorPosition += textLength;

https://codereview.chromium.org/1995333002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:272: } else if
(text.length()) {
The code flow is getting difficult to follow.  The reason is that the behavior
varies too much based on whether we have KeepSelection or not.  This calls for a
small refactoring to split into two methods.  Here's what I propose:

1. Rename existing confirmComposition(text, ConfirmCompositionBehavior) to
replaceComposition() and make this method private (it seems to have no outside
callers).
2. Split "confirmCompositionOrInsertText(text, KeepSelection, 1) ->
"confirmComposition(text)"
3. Split "confirmCompositionOrInsertText(text, DoNotKeepSelection,
relativeCursorPos" -> "confirmCompositionWithCursor(text, relativeCursorPos)"

and WebViewImpl::confirmComposition should call the right one based on the value
of KeepSelection.

aelias_OOO_until_Jul13

https://codereview.chromium.org/1995333002/diff/20001/third_party/WebKit/Source/core/editing/InputMethodController.cpp File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/1995333002/diff/20001/third_party/WebKit/Source/core/editing/InputMethodController.cpp#newcode254 third_party/WebKit/Source/core/editing/InputMethodController.cpp:254: newCursorPosition += compositionRange.end(); On 2016/08/24 at 08:13:07, aelias wrote: ...

4 years, 4 months ago (2016-08-24 08:16:01 UTC) #46

yabinh

The CQ bit was checked by yabinh@chromium.org to run a CQ dry run

4 years, 4 months ago (2016-08-25 02:29:54 UTC) #47

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1995333002/100001

4 years, 4 months ago (2016-08-25 02:30:25 UTC) #48

yabinh

aelias@, can you take another look of this patch? https://codereview.chromium.org/1995333002/diff/80001/content/common/input_messages.h File content/common/input_messages.h (right): https://codereview.chromium.org/1995333002/diff/80001/content/common/input_messages.h#newcode174 content/common/input_messages.h:174: ...

4 years, 4 months ago (2016-08-25 02:32:21 UTC) #49

aelias@, can you take another look of this patch?

https://codereview.chromium.org/1995333002/diff/80001/content/common/input_me...
File content/common/input_messages.h (right):

https://codereview.chromium.org/1995333002/diff/80001/content/common/input_me...
content/common/input_messages.h:174: int /* new_cursor_pos */)
On 2016/08/24 08:13:07, aelias wrote:
> I realize that you matched the name used in the Android documentation, but I
> think a clearer name would be "relative_cursor_pos".  Could you
> search-and-replace in your patch to switch to that name?  I think that would
> avoid the mistake of assuming it's an absolute offset from the beginning of
the
> textbox.

Done.

https://codereview.chromium.org/1995333002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right):

https://codereview.chromium.org/1995333002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:239: // If
newCursorPosition > 0, it's relative to the end of the text - 1;
On 2016/08/24 08:13:07, aelias wrote:
> Please add a comment that this is to match Android behavior with a link to
>
https://developer.android.com/reference/android/view/inputmethod/InputConnect...,
> int)

Done.

https://codereview.chromium.org/1995333002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:253:
newCursorPosition += compositionRange.end();
On 2016/08/24 08:13:07, aelias wrote:
> I read your explanation above that text.length() == 0 effectively means the
> current composition is considered the text.  I think this should be expressed
> more explicitly, along the lines of:
> 
> bool InputMethodController::confirmCompositionOrInsertText(... , int
> relativeCursorPosition)
> 
> int textStart, textLength, absoluteCursorPosition;
> textStart = compositionRange.start() [or getSelectionOffsets().start() ];
> textLength = text.length() ? text.length() : compositionRange.length();
> absoluteCursorPosition = relativeCursorPosition + textStart - 1;
> if (relativeCursorPosition > 0) absoluteCursorPosition += textLength;

Done.

https://codereview.chromium.org/1995333002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:272: } else if
(text.length()) {
On 2016/08/24 08:13:07, aelias wrote:
> The code flow is getting difficult to follow.  The reason is that the behavior
> varies too much based on whether we have KeepSelection or not.  This calls for
a
> small refactoring to split into two methods.  Here's what I propose:
> 
> 1. Rename existing confirmComposition(text, ConfirmCompositionBehavior) to
> replaceComposition() and make this method private (it seems to have no outside
> callers).
> 2. Split "confirmCompositionOrInsertText(text, KeepSelection, 1) ->
> "confirmComposition(text)"
> 3. Split "confirmCompositionOrInsertText(text, DoNotKeepSelection,
> relativeCursorPos" -> "confirmCompositionWithCursor(text, relativeCursorPos)"
> 
> and WebViewImpl::confirmComposition should call the right one based on the
value
> of KeepSelection.

For#1, The old InputMethodController#confirmComposition ( ika the new
inputMethodController#replaceComposition) is called in some places outside
InputMethodController, like InputMethodControllerTest, WebViewImpl,
WebLocalFrameImpl, and WebFrameWidgetImpl. So we'd better keep it public.

aelias_OOO_until_Jul13

Looking pretty good, just a few more details. https://codereview.chromium.org/1995333002/diff/100001/third_party/WebKit/Source/core/editing/InputMethodController.cpp File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/1995333002/diff/100001/third_party/WebKit/Source/core/editing/InputMethodController.cpp#newcode293 third_party/WebKit/Source/core/editing/InputMethodController.cpp:293: if ...

4 years, 4 months ago (2016-08-25 02:50:57 UTC) #50

aelias_OOO_until_Jul13

https://codereview.chromium.org/1995333002/diff/100001/content/renderer/render_frame_impl.h File content/renderer/render_frame_impl.h (right): https://codereview.chromium.org/1995333002/diff/100001/content/renderer/render_frame_impl.h#newcode373 content/renderer/render_frame_impl.h:373: int relative_cursor_pos = 1); Default arguments violate Chromium style ...

4 years, 4 months ago (2016-08-25 03:02:11 UTC) #51

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 4 months ago (2016-08-25 04:08:35 UTC) #53

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 4 months ago (2016-08-25 04:08:36 UTC) #54

yabinh

The CQ bit was checked by yabinh@chromium.org to run a CQ dry run

4 years, 4 months ago (2016-08-25 08:41:53 UTC) #55

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1995333002/120001

4 years, 4 months ago (2016-08-25 08:42:27 UTC) #56

yabinh

https://codereview.chromium.org/1995333002/diff/100001/content/renderer/render_frame_impl.h File content/renderer/render_frame_impl.h (right): https://codereview.chromium.org/1995333002/diff/100001/content/renderer/render_frame_impl.h#newcode373 content/renderer/render_frame_impl.h:373: int relative_cursor_pos = 1); On 2016/08/25 03:02:10, aelias wrote: ...

4 years, 3 months ago (2016-08-25 10:27:08 UTC) #57

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 3 months ago (2016-08-25 10:48:02 UTC) #58

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 3 months ago (2016-08-25 10:48:03 UTC) #59

aelias_OOO_until_Jul13

Almost there, thanks for following up! https://codereview.chromium.org/1995333002/diff/100001/third_party/WebKit/Source/core/editing/InputMethodController.h File third_party/WebKit/Source/core/editing/InputMethodController.h (right): https://codereview.chromium.org/1995333002/diff/100001/third_party/WebKit/Source/core/editing/InputMethodController.h#newcode66 third_party/WebKit/Source/core/editing/InputMethodController.h:66: bool replaceComposition(const String& ...

4 years, 3 months ago (2016-08-25 22:43:16 UTC) #60

aelias_OOO_until_Jul13

lgtm assuming replaceComposition() is made private. Feel free to add yosin@ as reviewer once that ...

4 years, 3 months ago (2016-08-25 22:44:49 UTC) #61

yabinh

https://codereview.chromium.org/1995333002/diff/100001/third_party/WebKit/Source/core/editing/InputMethodController.h File third_party/WebKit/Source/core/editing/InputMethodController.h (right): https://codereview.chromium.org/1995333002/diff/100001/third_party/WebKit/Source/core/editing/InputMethodController.h#newcode66 third_party/WebKit/Source/core/editing/InputMethodController.h:66: bool replaceComposition(const String& text, ConfirmCompositionBehavior confirmBehavior = KeepSelection); On ...

4 years, 3 months ago (2016-08-26 01:23:20 UTC) #62

aelias_OOO_until_Jul13

https://codereview.chromium.org/1995333002/diff/100001/third_party/WebKit/Source/core/editing/InputMethodController.h File third_party/WebKit/Source/core/editing/InputMethodController.h (right): https://codereview.chromium.org/1995333002/diff/100001/third_party/WebKit/Source/core/editing/InputMethodController.h#newcode66 third_party/WebKit/Source/core/editing/InputMethodController.h:66: bool replaceComposition(const String& text, ConfirmCompositionBehavior confirmBehavior = KeepSelection); On ...

4 years, 3 months ago (2016-08-26 02:08:59 UTC) #63

yabinh

https://codereview.chromium.org/1995333002/diff/100001/third_party/WebKit/Source/core/editing/InputMethodController.h File third_party/WebKit/Source/core/editing/InputMethodController.h (right): https://codereview.chromium.org/1995333002/diff/100001/third_party/WebKit/Source/core/editing/InputMethodController.h#newcode66 third_party/WebKit/Source/core/editing/InputMethodController.h:66: bool replaceComposition(const String& text, ConfirmCompositionBehavior confirmBehavior = KeepSelection); On ...

4 years, 3 months ago (2016-08-26 02:46:23 UTC) #64

aelias_OOO_until_Jul13

https://codereview.chromium.org/1995333002/diff/100001/third_party/WebKit/Source/core/editing/InputMethodController.h File third_party/WebKit/Source/core/editing/InputMethodController.h (right): https://codereview.chromium.org/1995333002/diff/100001/third_party/WebKit/Source/core/editing/InputMethodController.h#newcode75 third_party/WebKit/Source/core/editing/InputMethodController.h:75: On 2016/08/26 at 02:46:23, yabinh wrote: > On a ...

4 years, 3 months ago (2016-08-26 05:58:59 UTC) #65

yabinh

The CQ bit was checked by yabinh@chromium.org to run a CQ dry run

4 years, 3 months ago (2016-08-26 07:13:17 UTC) #66

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1995333002/140001

4 years, 3 months ago (2016-08-26 07:13:29 UTC) #67

yabinh

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

4 years, 3 months ago (2016-08-26 07:18:31 UTC) #68

yabinh

yosin@, can you take a look at this patch? Thanks. https://codereview.chromium.org/1995333002/diff/100001/third_party/WebKit/Source/core/editing/InputMethodController.h File third_party/WebKit/Source/core/editing/InputMethodController.h (right): https://codereview.chromium.org/1995333002/diff/100001/third_party/WebKit/Source/core/editing/InputMethodController.h#newcode75 ...

4 years, 3 months ago (2016-08-26 07:18:32 UTC) #69

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 3 months ago (2016-08-26 07:30:49 UTC) #70

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_rel_ng/builds/268218)

4 years, 3 months ago (2016-08-26 07:30:51 UTC) #71

yabinh

The CQ bit was checked by yabinh@chromium.org to run a CQ dry run

4 years, 3 months ago (2016-08-26 08:37:53 UTC) #72

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1995333002/160001

4 years, 3 months ago (2016-08-26 08:38:13 UTC) #73

yosin_UTC9

https://codereview.chromium.org/1995333002/diff/160001/third_party/WebKit/Source/core/editing/InputMethodController.cpp File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/1995333002/diff/160001/third_party/WebKit/Source/core/editing/InputMethodController.cpp#newcode256 third_party/WebKit/Source/core/editing/InputMethodController.cpp:256: int textStart, textLength; Can we introduce other functions to ...

4 years, 3 months ago (2016-08-26 10:06:11 UTC) #74

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 3 months ago (2016-08-26 10:32:48 UTC) #75

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 3 months ago (2016-08-26 10:32:49 UTC) #76

yabinh

The CQ bit was checked by yabinh@chromium.org to run a CQ dry run

4 years, 3 months ago (2016-08-29 05:17:46 UTC) #77

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1995333002/180001

4 years, 3 months ago (2016-08-29 05:18:15 UTC) #78

yabinh

yosin@, can you take another look at this patch? Thanks. https://codereview.chromium.org/1995333002/diff/160001/third_party/WebKit/Source/core/editing/InputMethodController.cpp File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/1995333002/diff/160001/third_party/WebKit/Source/core/editing/InputMethodController.cpp#newcode256 ...

4 years, 3 months ago (2016-08-29 05:49:53 UTC) #79

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 3 months ago (2016-08-29 06:58:04 UTC) #80

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_android_rel_ng/builds/131705)

4 years, 3 months ago (2016-08-29 06:58:05 UTC) #81

yosin_UTC9

https://codereview.chromium.org/1995333002/diff/180001/third_party/WebKit/Source/core/editing/InputMethodController.cpp File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/1995333002/diff/180001/third_party/WebKit/Source/core/editing/InputMethodController.cpp#newcode267 third_party/WebKit/Source/core/editing/InputMethodController.cpp:267: nit: no need to have an extra blank line. ...

4 years, 3 months ago (2016-08-29 07:02:02 UTC) #82

yabinh

The CQ bit was checked by yabinh@chromium.org to run a CQ dry run

4 years, 3 months ago (2016-08-29 08:39:29 UTC) #83

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1995333002/200001

4 years, 3 months ago (2016-08-29 08:39:42 UTC) #84

yabinh

https://codereview.chromium.org/1995333002/diff/180001/third_party/WebKit/Source/core/editing/InputMethodController.cpp File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/1995333002/diff/180001/third_party/WebKit/Source/core/editing/InputMethodController.cpp#newcode267 third_party/WebKit/Source/core/editing/InputMethodController.cpp:267: On 2016/08/29 07:02:01, Yosi_UTC9 wrote: > nit: no need ...

4 years, 3 months ago (2016-08-29 08:44:01 UTC) #85

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 3 months ago (2016-08-29 09:52:47 UTC) #86

commit-bot: I haz the power

Dry run: Exceeded global retry quota

4 years, 3 months ago (2016-08-29 09:52:49 UTC) #87

yabinh

yabinh@chromium.org changed reviewers: + sievers@chromium.org, tkent@chromium.org

4 years, 3 months ago (2016-08-30 07:07:38 UTC) #89

yabinh

tkent@chromium.org: Please review changes in text_input_controller.cc sievers@chromium.org: Please review changes in content/ Thanks~

4 years, 3 months ago (2016-08-30 07:07:39 UTC) #90

tkent

https://codereview.chromium.org/1995333002/diff/200001/third_party/WebKit/Source/core/editing/InputMethodController.cpp File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/1995333002/diff/200001/third_party/WebKit/Source/core/editing/InputMethodController.cpp#newcode239 third_party/WebKit/Source/core/editing/InputMethodController.cpp:239: // https://developer.android.com/reference/android/view/inputmethod/InputConnection.html#commitText(java.lang.CharSequence, int) Even though this follows an Android ...

4 years, 3 months ago (2016-08-30 07:47:12 UTC) #91

yabinh

The CQ bit was checked by yabinh@chromium.org to run a CQ dry run

4 years, 3 months ago (2016-08-30 10:43:11 UTC) #92

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1995333002/220001

4 years, 3 months ago (2016-08-30 10:43:27 UTC) #93

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 3 months ago (2016-08-30 11:41:49 UTC) #94

commit-bot: I haz the power

Dry run: Exceeded global retry quota

4 years, 3 months ago (2016-08-30 11:41:50 UTC) #95

yabinh

https://codereview.chromium.org/1995333002/diff/200001/third_party/WebKit/Source/core/editing/InputMethodController.cpp File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/1995333002/diff/200001/third_party/WebKit/Source/core/editing/InputMethodController.cpp#newcode239 third_party/WebKit/Source/core/editing/InputMethodController.cpp:239: // https://developer.android.com/reference/android/view/inputmethod/InputConnection.html#commitText(java.lang.CharSequence, int) On 2016/08/30 07:47:11, tkent wrote: > ...

4 years, 3 months ago (2016-08-30 12:33:05 UTC) #96

aelias_OOO_until_Jul13

Back to not lgtm because I have concerns about correctness after the latest rounds of ...

4 years, 3 months ago (2016-08-30 17:26:13 UTC) #97

yabinh

The CQ bit was checked by yabinh@chromium.org to run a CQ dry run

4 years, 3 months ago (2016-08-31 10:20:56 UTC) #98

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1995333002/240001

4 years, 3 months ago (2016-08-31 10:21:10 UTC) #99

yabinh

The CQ bit was checked by yabinh@chromium.org to run a CQ dry run

4 years, 3 months ago (2016-08-31 11:22:11 UTC) #100

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1995333002/260001

4 years, 3 months ago (2016-08-31 11:22:30 UTC) #101

yabinh

https://codereview.chromium.org/1995333002/diff/160001/third_party/WebKit/Source/core/editing/InputMethodController.cpp File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/1995333002/diff/160001/third_party/WebKit/Source/core/editing/InputMethodController.cpp#newcode267 third_party/WebKit/Source/core/editing/InputMethodController.cpp:267: textLength = text.length() ? text.length() : compositionRange.length(); On 2016/08/30 ...

4 years, 3 months ago (2016-08-31 11:23:28 UTC) #102

https://codereview.chromium.org/1995333002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right):

https://codereview.chromium.org/1995333002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:267: textLength
= text.length() ? text.length() : compositionRange.length();
On 2016/08/30 17:26:13, aelias wrote:
> I don't see any equivalent to this line of logic starting in patch set #10, so
> how can the latest patches be correct?
> 

That logic is hidden in replaceCompositionAndMoveCaret. When text is empty,
textLength = composingText().length(); otherwise, textLength = text.length();

> Furthermore, I believe tkent@'s desired semantics aren't feasible to implement
> correctly if you bring back the correct behavior.  The up-to-date
> compositionRange length is not known from the browser process.
> 
> If I'm right, please introduce a unit test covering this case to avoid this
> regressing again.

I'm not sure if it matters.
relativeCaretPosition is relative to the end of text, so we only need to be
careful with the case when replaceCompositionAndMoveCaret(composingText(),
relativeCaretPosition) is called with relativeCaretPosition < 0 in
InputMethodController::commitCompositionAndMoveCaret, because |composingText()|
here might be changed, and different from that in ime_adapter_android.

But in ImeTest, it's impossible to go to
replaceCompositionAndMoveCaret(composingText(), relativeCaretPosition). So it's
not a problem at least in ImeTest.

But there is another problem. We are able to set the caret within the composing
text with this patch. (See WebViewTest.cpp in patch set 14.) 
I'm not sure if anyone wants to do that, at least android doesn't want because
of the similar concern. 
(See
https://developer.android.com/reference/android/view/inputmethod/InputConnect...,
int)). 
Luckily, we are still unable to set the caret through ImeAdapter, because we
have some conversion in ime_adapter_android.cc.

https://codereview.chromium.org/1995333002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/WebViewTest.cpp (right):

https://codereview.chromium.org/1995333002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebViewTest.cpp:1135: 
We are able to move caret with composing text.

yabinh

https://codereview.chromium.org/1995333002/diff/260001/third_party/WebKit/Source/web/tests/WebViewTest.cpp File third_party/WebKit/Source/web/tests/WebViewTest.cpp (right): https://codereview.chromium.org/1995333002/diff/260001/third_party/WebKit/Source/web/tests/WebViewTest.cpp#newcode1135 third_party/WebKit/Source/web/tests/WebViewTest.cpp:1135: On 2016/08/31 11:23:27, yabinh wrote: > We are able ...

4 years, 3 months ago (2016-08-31 11:26:21 UTC) #103

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 3 months ago (2016-08-31 12:57:08 UTC) #104

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 3 months ago (2016-08-31 12:57:09 UTC) #105

aelias_OOO_until_Jul13

OK, I've thought about it and convinced myself that the patch is correct. But it ...

4 years, 3 months ago (2016-09-01 05:21:58 UTC) #106

OK, I've thought about it and convinced myself that the patch is correct.  But
it took way too much effort, so let's make the system easier to reason about. 
And I discovered a preexisting minor bug we can't fix with the current approach.

The main problem is the special case of empty "text" meaning we should use the
existing composition -- let's get rid of that.  Reasons:
1. In Android EditText, committing "" clobbers the composition (see
implementation at
https://android.googlesource.com/platform/frameworks/base/+/master/core/java/...
) so the special case is actually a preexisting bug on Android (difference from
reference implementation) that can only be fixed by changing our internal
message API.
2. The combination of negative tkent-style caret offset and empty text is
nonsensical: your patch is only correct because that can never happen based on
platform API call pattern.  But that's confusing: it would be better to
structure our internal methods so that nonsense argument combinations are
impossible.

I propose splitting InputMsg_ImeConfirmComposition into two messages and also
change InputMethodController public methods to match them:

A. InputMsg_ImeFinishComposition(bool keepSelection)
B. InputMsg_ImeCommitText(String text, Range replacementRange, int
tkentStyleOffset)

(Semantic notes: keepSelection = false means caret goes to end of composition. 
ImeCommitText would not have ability to keep selection -- I've verified that's
not a necessary feature, keepSelection is purely for Android finishComposition()
-- and it would not have any special behavior for empty string.)

Every case on Android, Mac and Aura where empty text is currently passed in,
please change to call InputMsg_ImeFinishComposition() instead.  I assume
empty-text special case probably originates from Windows system API so
RenderWidgetHostViewAura should have a if(text.empty() forwarding to
ImeFinishComposition instead -- that's where that branch should live, not deep
in Blink.

https://codereview.chromium.org/1995333002/diff/260001/content/browser/render...
File content/browser/renderer_host/render_widget_host_impl.h (right):

https://codereview.chromium.org/1995333002/diff/260001/content/browser/render...
content/browser/renderer_host/render_widget_host_impl.h:380: int
relative_cursor_pos = 0);
Default argument values are against Chromium style.

https://codereview.chromium.org/1995333002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/WebViewTest.cpp (right):

https://codereview.chromium.org/1995333002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebViewTest.cpp:1135: 
On 2016/08/31 at 11:26:21, yabinh wrote:
> On 2016/08/31 11:23:27, yabinh wrote:
> > We are able to move caret with composing text.
> 
> Typo. Should be "within composing text".

OK, I think we don't need to worry about that, tkent's offset system is fine. 
Android docs are worried about this because Android InputFilter objects can
mutate text at composition commit time.  But we don't have any equivalent on the
web (we don't allow Javascript to do such a thing).

yabinh

The CQ bit was checked by yabinh@chromium.org to run a CQ dry run

4 years, 3 months ago (2016-09-06 09:58:43 UTC) #107

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1995333002/280001

4 years, 3 months ago (2016-09-06 09:58:53 UTC) #108

yabinh

On 2016/09/01 05:21:58, aelias wrote: > I propose splitting InputMsg_ImeConfirmComposition into two messages and also ...

4 years, 3 months ago (2016-09-06 11:25:55 UTC) #109

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 3 months ago (2016-09-06 12:23:04 UTC) #110

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 3 months ago (2016-09-06 12:23:06 UTC) #111

aelias_OOO_until_Jul13

lgtm modulo comments below. tkent@, could you take another look? https://codereview.chromium.org/1995333002/diff/280001/content/browser/renderer_host/render_widget_host_view_aura.cc File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1995333002/diff/280001/content/browser/renderer_host/render_widget_host_view_aura.cc#newcode1384 ...

4 years, 3 months ago (2016-09-07 05:02:40 UTC) #112

yosin_UTC9

Some nits. https://codereview.chromium.org/1995333002/diff/280001/third_party/WebKit/Source/core/editing/InputMethodController.cpp File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/1995333002/diff/280001/third_party/WebKit/Source/core/editing/InputMethodController.cpp#newcode211 third_party/WebKit/Source/core/editing/InputMethodController.cpp:211: return replaceCompositionAndMoveCaret(text, relativeCaretPosition); nit: It is better ...

4 years, 3 months ago (2016-09-07 07:19:22 UTC) #114

yabinh

The CQ bit was checked by yabinh@chromium.org to run a CQ dry run

4 years, 3 months ago (2016-09-07 10:15:58 UTC) #115

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1995333002/300001

4 years, 3 months ago (2016-09-07 10:16:15 UTC) #116

yabinh

Description was changed from ========== Handle newCursorPosition correctly for Android's commitText() Android’s InputConnection API specifies ...

4 years, 3 months ago (2016-09-07 10:22:17 UTC) #117

Description was changed from

==========
Handle newCursorPosition correctly for Android's commitText()

Android’s InputConnection API specifies a way to commit (confirm) the current
composition
and move the cursor at the same time:
https://developer.android.com/reference/android/view/inputmethod/
InputConnection.html#commitText(java.lang.CharSequence, int)

Currently, commitText doesn't conform to the documentation nor to
Android TextView's behavior when newCursorPosition is not 1.
newCursorPosition is a relative value that depends on the location of the text
being committed.

When |newCursorPosition| < 1, the cursor should move to the left of 
the committing text by |newCursorPosition| characters. If the cursor
exceeds the left boundary, it will clamp to  the left boundary.

When |newCursorPosition| >= 1, the cursor will move to the right of 
the committing text by |newCursorPosition - 1| characters. If the cursor
exceeds the right boundary, it will clamp to the right boundary.

BUG=570920
==========

to

==========
Handle newCursorPosition correctly for Android's commitText()

Android’s InputConnection API specifies a way to commit (confirm) the
current composition and move the cursor at the same time:
https://developer.android.com/reference/android/view/inputmethod/InputConnect...,
int)

Currently, commitText doesn't conform to the documentation nor to
Android TextView's behavior when newCursorPosition is not 1.
newCursorPosition
is a relative value that depends on the location of the text being committed.

When |newCursorPosition| < 1, the cursor should move to the left of 
the committing text by |newCursorPosition| characters. If the cursor
exceeds the left boundary, it will clamp to  the left boundary.

When |newCursorPosition| >= 1, the cursor will move to the right of 
the committing text by |newCursorPosition - 1| characters. If the cursor
exceeds the right boundary, it will clamp to the right boundary.

BUG=570920
==========

yabinh

Description was changed from ========== Handle newCursorPosition correctly for Android's commitText() Android’s InputConnection API specifies ...

4 years, 3 months ago (2016-09-07 10:22:55 UTC) #118

Description was changed from

==========
Handle newCursorPosition correctly for Android's commitText()

Android’s InputConnection API specifies a way to commit (confirm) the
current composition and move the cursor at the same time:
https://developer.android.com/reference/android/view/inputmethod/InputConnect...,
int)

Currently, commitText doesn't conform to the documentation nor to
Android TextView's behavior when newCursorPosition is not 1.
newCursorPosition
is a relative value that depends on the location of the text being committed.

When |newCursorPosition| < 1, the cursor should move to the left of 
the committing text by |newCursorPosition| characters. If the cursor
exceeds the left boundary, it will clamp to  the left boundary.

When |newCursorPosition| >= 1, the cursor will move to the right of 
the committing text by |newCursorPosition - 1| characters. If the cursor
exceeds the right boundary, it will clamp to the right boundary.

BUG=570920
==========

to

==========
Handle newCursorPosition correctly for Android's commitText()

Android’s InputConnection API specifies a way to commit (confirm) the
current composition and move the cursor at the same time:
https://developer.android.com/reference/android/view/inputmethod/
InputConnection.html#commitText(java.lang.CharSequence, int)

Currently, commitText doesn't conform to the documentation nor to
Android TextView's behavior when newCursorPosition is not 1.
newCursorPosition
is a relative value that depends on the location of the text being committed.

When |newCursorPosition| < 1, the cursor should move to the left of 
the committing text by |newCursorPosition| characters. If the cursor
exceeds the left boundary, it will clamp to  the left boundary.

When |newCursorPosition| >= 1, the cursor will move to the right of 
the committing text by |newCursorPosition - 1| characters. If the cursor
exceeds the right boundary, it will clamp to the right boundary.

BUG=570920
==========

yabinh

sievers@, can you take a look at changes in content/ ? Thanks. https://codereview.chromium.org/1995333002/diff/280001/content/browser/renderer_host/render_widget_host_view_aura.cc File content/browser/renderer_host/render_widget_host_view_aura.cc ...

4 years, 3 months ago (2016-09-07 10:27:15 UTC) #119

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 3 months ago (2016-09-07 11:37:02 UTC) #120

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 3 months ago (2016-09-07 11:37:04 UTC) #121

no sievers

content lgtm stamp. maybe get somebody to double-check for aura+mac though?

4 years, 3 months ago (2016-09-13 18:33:38 UTC) #122

aelias_OOO_until_Jul13

On 2016/09/13 at 18:33:38, sievers wrote: > maybe get somebody to double-check for aura+mac though? ...

4 years, 3 months ago (2016-09-13 18:42:48 UTC) #123

aelias_OOO_until_Jul13

The CQ bit was checked by aelias@chromium.org to run a CQ dry run

4 years, 3 months ago (2016-09-13 18:43:09 UTC) #124

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1995333002/300001

4 years, 3 months ago (2016-09-13 18:43:38 UTC) #125

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 3 months ago (2016-09-13 18:47:34 UTC) #126

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm64_dbg_recipe/builds/128023) android_clang_dbg_recipe on ...

4 years, 3 months ago (2016-09-13 18:47:36 UTC) #127

aelias_OOO_until_Jul13

One last comment below, aside from that just needs a rebase to land. https://codereview.chromium.org/1995333002/diff/300001/third_party/WebKit/Source/core/editing/InputMethodController.cpp File ...

4 years, 3 months ago (2016-09-13 19:30:08 UTC) #128

aelias_OOO_until_Jul13

4 years, 3 months ago (2016-09-14 03:23:22 UTC) #129

I couldn't upload a rebased patchset here, so I created another issue and landed
this on yabinh@'s behalf in https://codereview.chromium.org/2339793002/ (he's on
vacation for another week and I'd like this to land before it needs further
rebase).  Closing this issue.

Issue 1995333002: Handle newCursorPosition correctly for Android's commitText() (Closed)

Description

Patch Set 1 #

Patch Set 2 : adjust selection in confirmCompositionOrInsertText() #

Patch Set 3 : For changwan@'s review #

Patch Set 4 : Sync. Almost the same to the previous patch. #

Patch Set 5 : Add 3 tests #

Patch Set 6 : change for aelias@'s review #

Patch Set 7 : For aelias@'s 2nd review #

Patch Set 8 : Remove unused function #

Patch Set 9 : For compile error. #

Patch Set 10 : For yosin@'s review #

Patch Set 11 : For yosin@'s 2nd review #

Patch Set 12 : For tkent@'s review #

Patch Set 13 : Move caret even when text is empty, and add a test. #

Patch Set 14 : Move caret within composing text. #

Patch Set 15 : Split confirmComposition into 2 #

Patch Set 16 : For aelias and yosin review #

Messages