Caret position changed abnormal when modify inputtext on InputEvent |
||||||||||||||||
Issue descriptionChrome 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
,
Jun 28 2017
,
Jun 28 2017
,
Jun 29 2017
,
Jun 30 2017
M58(stable) and M61(canary) work as expected. Please do bisect for M58 to M59. It seems problem happened in M59.
,
Jun 30 2017
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.
,
Jul 6 2017
dknandiraju@, please bisect. I can repro in M58 and M61 too.
,
Jul 7 2017
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.
,
Jul 12 2017
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.
,
Jul 13 2017
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
,
Oct 17 2017
zhengyuli@ are you still encountering the bug in latest chrome stable?
,
Oct 17 2017
,
Oct 18 2017
I can reproduce in version 61.0.3163.96.
,
Oct 24 2017
,
Oct 28 2017
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.
,
Oct 28 2017
This is likely the cause of crbug.com/674139 , as well as Google-internal b/37755889.
,
Oct 28 2017
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).
,
Oct 30 2017
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.
,
Nov 6 2017
in review http://crrev.com/c/754037
,
Dec 13 2017
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/
,
Dec 13 2017
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.
,
Dec 13 2017
> 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.
,
Dec 13 2017
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).
,
Dec 14 2017
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.
,
Jan 3 2018
Issue 796025 has been merged into this issue.
,
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
,
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
,
Jan 17 2018
This will be fixed in Chrome 65, targeted for release around March 6.
,
Jan 22 2018
|
||||||||||||||||
►
Sign in to add a comment |
||||||||||||||||
Comment 1 by wangxianzhu@chromium.org
, Jun 28 2017Components: Blink>Editing
Labels: Needs-Bisect OS-Android