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

Issue 737395 link

Starred by 6 users

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Jan 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Bug



Sign in to add a comment

Caret position changed abnormal when modify inputtext on InputEvent

Project Member Reported by zhengy...@tencent.com, Jun 28 2017

Issue description

Chrome Version: (59.0.3071.92)
OS: Android

What steps will reproduce the problem?
Refer to the attachment(caret.htm)
(1)input chars more than three(input value will modify by web page in this case)
(2)check caret position
(3)

What is the expected result?
Caret displayed at end

What happens instead?
Caret displayed in middle of the value
 
caret.htm
735 bytes View Download
Cc: yosin@chromium.org wangxianzhu@chromium.org
Components: Blink>Editing
Labels: Needs-Bisect OS-Android

Comment 2 by rbyers@chromium.org, Jun 28 2017

Cc: aelias@chromium.org

Comment 3 by aelias@chromium.org, Jun 28 2017

Cc: changwan@chromium.org

Comment 4 by chongz@chromium.org, Jun 29 2017

Cc: chongz@chromium.org
Not tested but probably related to  issue 663944 ?

Comment 5 by yosin@chromium.org, Jun 30 2017

Status: Unconfirmed (was: Untriaged)
Summary: NEEDS_BIDSECT Caret position changed abnormal when modify inputtext on InputEvent (was: Caret position changed abnormal when modify inputtext on InputEvent)
M58(stable) and M61(canary) work as expected.
Please do bisect for M58 to M59.
It seems problem happened in M59.
I just test this issue in M58 and bisect M59.
1) It also happened in 58.0.3029.83, 59.0.3071.117, 59.0.3071.125 versions;
2) I can't find M61(canary) version to test.
Cc: dknandiraju@chromium.org
Labels: triage-te
dknandiraju@, please bisect.

I can repro in M58 and M61 too.
Note:
1) Good Build: 55.0.2883.17
   Bad Build:  55.0.2883.18
2) Bisect range: https://chromium.googlesource.com/chromium/src/+log/55.0.2883.17..55.0.2883.18?pretty=fuller&n=10000
3) Please find logs and video taken on Moto g4 plus(athene_f)/7.0.0 @ http://go/chrome-androidlogs1/6/737395
4) There are not enough commit positions to bisect in this regression range.

Comment 9 by aelias@chromium.org, Jul 12 2017

Owner: changwan@chromium.org
Status: Assigned (was: Unconfirmed)
changwan@'s "Workaround for setComposition styling clobber" is in the range and might be culprit.  Also, general problem of JS interacting with IME is in changwan@'s area of interest.
Cc: -changwan@chromium.org yabinh@chromium.org
Please find details on M56

1) Good Build: 56.0.2888.3
   Bad Build:  56.0.2889.0
   Good Commit: 424715 Bad Commit: 424716
2) Bisect range: https://chromium.googlesource.com/chromium/src/+log/56.0.2888.3..56.0.2889.0?pretty=fuller&n=10000
3) Culprit: https://chromium.googlesource.com/chromium/src/+/993074d17afee685ced99283d7ffbe809fdb6f99
Labels: Needs-Feedback
zhengyuli@ are you still encountering the bug in latest chrome stable?
Cc: changwan@chromium.org
Owner: rlanday@chromium.org
I can reproduce in version 61.0.3163.96.
Labels: -Needs-Bisect hasbisect-per-revision
Labels: -Pri-3 Pri-2
I think I've figured out what's going on here. The good news is that it's not the IME clobbering the JavaScript change like I think we were worried about...we're clobbering it all by ourselves in InputMethodController.

When you type the fourth character into the text box in the test case, Gboard updates the composition using InputConnection#setComposingText(). This is implemented in InputMethodController::SetComposition(). This method performs the following steps:

- Select the composition range (so we can replace it)
- Compute the character offsets for the range the IME is requesting to be selected after the operation (stored as a PlainTextRange).
- Call InsertTextDuringCompositionWithEvents() to replace the text and call the JavaScript event handlers. This updates the selection once to do the replacement, and again after the JavaScript handler overwrites the entire text box's text (this operation moves the insertion caret to the end).
- Now, we have to set the selection to what the IME expects, so we use the character offsets we computed earlier to update the selection.

The problem is that the last step doesn't know JavaScript has messed with the text in the meantime. I think whatever the IME is trying to do with the selection should be overridden by what the JavaScript is asking for.

yosin@, what's a good way to fix this? I think we need to keep track of if the JavaScript event handlers change the text or the selection, and not apply the IME selection range in this case.
This is likely the cause of  crbug.com/674139 , as well as Google-internal b/37755889.
The reason we didn't have this bug earlier is that before the culprit change, we were originally resetting the IME after every single character (since the JavaScript is overwriting the input value after every character). There have been several changes since then (the culprit change added incremental insertions, but disabling that's not enough to work around the issue) that make the IME reset less often. If we fix this properly, we can make the IME only reset when the JavaScript actually changes the input value (so this way, the composition underline won't blink after each character).
I think there are a number of related situations where it's not immediately clear what the correct behavior should be. I'm going to try to write up a doc this week to explore some possibilities.
Components: -Blink>Editing Blink>Editing>Selection
Status: Started (was: Assigned)
Summary: Caret position changed abnormal when modify inputtext on InputEvent (was: NEEDS_BIDSECT Caret position changed abnormal when modify inputtext on InputEvent)
in review http://crrev.com/c/754037

Comment 20 by yosin@chromium.org, Dec 13 2017

Components: -Blink>Editing>Selection Blink>Editing>IME
It seems we may want to set selection before dispatching "input" event rather than
after dispatching "input" event. This is also mentioned in InsertTextDuringCompositionWithEvents() as TODO.

We may also want to composition range, IME marker and so on. Because, "input" event handler
can make DOM tree in unexpected shape.

In other words, if we think selection, marker and composition range as part of "DOM update",
we don't need to deal with unexpected state.

In this way, we are away from state changes made by "input" event handler. And "input"
event handler can see state change made by UA.

The spec[1] doesn't mentioned about when UA should change selection, before or after
dispatching "input" event. Note: "selectionchange" is async event, it is dispatched
after "input" event.

Please try to ask editing-tf for timing of "selection change", before or after "input"
event.

[1] https://www.w3.org/TR/input-events-2/
Cc: tedc...@chromium.org
Thanks for the insight, yosin@.

That's also what I suggested at
https://chromium-review.googlesource.com/c/chromium/src/+/754037#message-b0fa56f981f6a469500358279e7d09a0bc9cd204

rlanday@, what was the reason you didn't consider this approach?
I think it would eliminate this type of conflict to begin with.

> It seems we may want to set selection before dispatching "input" event rather than after dispatching "input" event. This is also mentioned in InsertTextDuringCompositionWithEvents() as TODO.

This is the "TODO(chongz): Fire 'input' event." TODO?
https://chromium.googlesource.com/chromium/src/+/1d58bc4000135f40585ea9aea8716a46aab05381/third_party/WebKit/Source/core/editing/ime/InputMethodController.cpp#177

So to implement this, I think we would make TypingCommand::InsertText() *not* fire the input event, and then fire it ourselves after moving the selection in InputMethodController::SetComposition()/CommitText().

We'd perform these steps in this order:

1. Do insert
2. Move selection
3. Call input handler
4. Call selectionchange handler

Based on my comment, I was concerned that web developers might be assuming that when an input event is called, they could use the current position of the selection to determine where the new text was inserted. I don't know if this is something developers actually do or not. I therefore thought the other approach Changwan suggested, of just not doing the IME cursor update if the IME wanted to leave the cursor at the end of the text (at least, that's how I interpreted what he wrote) seemed safer, and more obviously allowable by the spec.


Reviewing the relevant specs:

input event:

https://w3c.github.io/uievents/#event-type-input (UI Events, W3C Working Draft)
"A user agent MUST dispatch this event immediately after the DOM has been updated."

input is listed as a synchronous event. Synchronous events are defined here:
https://w3c.github.io/uievents/#sync-async

"Events which are synchronous (sync events) are treated as if they are in a virtual queue in a first-in-first-out model, ordered by sequence of temporal occurrence with respect to other events, to changes in the DOM, and to user interaction." 

selectionchange event:

http://w3c.github.io/selection-api/#selectionchange-event

"When the selection is dissociated with its range, associated with a new range or the associated range's boundary point is mutated either by the user or the content script, the user agent must queue a task to fire an event with the name selectionchange, which does not bubble and is not cancelable, at the document associated with the selection."

So it seems we may have some flexibility in how we dispatch selectionchange events; however, the requirement to dispatch the input event *immediately* after updating the DOM seems to mean we're not allowed to stick a selection update after the DOM update but before we call the input event handler (unless we consider the selection update to be part of the DOM update; I would argue that, the way we would implement it, the IME cursor move would happen *after* the DOM update, in a separate step, but this is perhaps debatable).

I'll start a thread on the public-editing-tf mailing list (https://lists.w3.org/Archives/Public/public-editing-tf/) asking if we're allowed to consider the selection update as part of the DOM update. This approach does have some nice properties (such as letting us put the DocumentMarkers in the correct place before any handlers run), if it turns out we can do it without breaking anything. I'm not really *too* concerned by the intervening selection update before the input event handler breaking things, since as we've observed, the IME almost always just wants to leave the selection at the end of the newly-inserted text, which is equivalent to not moving the selection.
I emailed that mailing list. I think it might take a few days for someone to manually approve my post (since I haven't posted before).
My message got posted and I got a response:
https://lists.w3.org/Archives/Public/public-editing-tf/2017Dec/0001.html

Sounds like we're OK to proceed with this approach, so I'll try to implement it and verify that it works.
Cc: rlanday@chromium.org ppolise...@chromium.org sandeepkumars@chromium.org
 Issue 796025  has been merged into this issue.
Project Member

Comment 26 by bugdroid1@chromium.org, Jan 11 2018

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

commit 7b5dbd51de14e44f0d5596e056e26707ff353cdf
Author: Ryan Landay <rlanday@chromium.org>
Date: Thu Jan 11 19:05:59 2018

Add layout test for IME JavaScript events

I'm planning to refactor some of the code that fires JavaScript input events in
response to IME commands in order to fix  crbug.com/737395 . In this CL, I'm
adding a layout test that will allow us to verify that we're not unintentionally
changing the behavior or ordering of any of these events.

Bug:  737395 
Change-Id: I0667195aa19bee596b42b699d98f2a22a2298e1d
Reviewed-on: https://chromium-review.googlesource.com/834885
Reviewed-by: Yoshifumi Inoue <yosin@chromium.org>
Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org>
Commit-Queue: Ryan Landay <rlanday@chromium.org>
Cr-Commit-Position: refs/heads/master@{#528696}
[modify] https://crrev.com/7b5dbd51de14e44f0d5596e056e26707ff353cdf/content/shell/test_runner/text_input_controller.cc
[modify] https://crrev.com/7b5dbd51de14e44f0d5596e056e26707ff353cdf/content/shell/test_runner/text_input_controller.h
[add] https://crrev.com/7b5dbd51de14e44f0d5596e056e26707ff353cdf/third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-ime.html

Project Member

Comment 27 by bugdroid1@chromium.org, Jan 17 2018

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

commit 500c8d1d245355c5b982b00d13d6747965f51cd8
Author: Ryan Landay <rlanday@chromium.org>
Date: Wed Jan 17 06:51:04 2018

Perform IME selection update before firing input and compositionend events

We've received numerous bug reports from web developers trying to use
JavaScript input event handlers to change the caret position after text is
entered that this does not work properly on Chrome for Android. The problem is
that the IME supplies a selection offset to move the cursor, which we currently
apply *after* running the input event handler. This CL fixes this problem by
using ScopedEventQueue to suppress input events from being fired until after we
apply the IME selection offset(s).

This CL also makes the same change for compositionend events (I haven't seen
the issue come up in practice for these events, since they're used much less
frequently, but it currently has the exact same issue).

Design doc for this CL with more information: https://goo.gl/xdy83L

Bug:  737395 ,  796695 
Change-Id: I0b5c3402b41ac5e11dbf1e74462045e99ba1d9c2
Reviewed-on: https://chromium-review.googlesource.com/838280
Reviewed-by: Yoshifumi Inoue <yosin@chromium.org>
Reviewed-by: Xiaocheng Hu <xiaochengh@chromium.org>
Commit-Queue: Ryan Landay <rlanday@chromium.org>
Cr-Commit-Position: refs/heads/master@{#529654}
[modify] https://crrev.com/500c8d1d245355c5b982b00d13d6747965f51cd8/third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-ime.html
[modify] https://crrev.com/500c8d1d245355c5b982b00d13d6747965f51cd8/third_party/WebKit/Source/core/editing/ime/InputMethodController.cpp
[modify] https://crrev.com/500c8d1d245355c5b982b00d13d6747965f51cd8/third_party/WebKit/Source/core/editing/ime/InputMethodControllerTest.cpp

Labels: M-65
Status: Fixed (was: Started)
This will be fixed in Chrome 65, targeted for release around March 6.
Cc: yukawa@chromium.org
 Issue 740213  has been merged into this issue.

Sign in to add a comment