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

Issue 802381 link

Starred by 3 users

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

InputMethodController::DeleteSurroundingText() doesn't fire beforeinput/input properly

Project Member Reported by rlanday@chromium.org, Jan 16 2018

Issue description

InputMethodController::DeleteSurroundingText() currently fires the input event twice (once for the text before the selection to be deleted and once for the text after), but doesn't fire the beforeinput event at all.

We should definitely be firing beforeinput before we fire input. I think we should probably do the whole delete together so we only fire each event once.
 
Ah, I think the reason it's implemented as it is is because if there's some text selected, this method can delete text before the selection and after the selection while leaving the selection itself. So unless we want to special-case the caret selection case (I have not yet seen a compelling reason to do this), I guess we should do:

beforeinput (preceding text)
input (preceding text)
beforeinput (following text)
input (following text)
Project Member

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

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

commit 6d163ae1380a2a69f713bf18203cff25de515c46
Author: Ryan Landay <rlanday@chromium.org>
Date: Wed Jan 17 06:58:06 2018

Add layout test for DeleteSurroundingText() JavaScript events

I was working on adding a missing beforeinput event in
https://chromium-review.googlesource.com/c/chromium/src/+/862521
and observed that InputMethodController::DeleteSurroundingText() is also not
properly firing a beforeinput event. This CL adds a layout test to record the
current behavior. Currently, we do not fire beforeinput at all, but fire input
twice (once for the text before the selection, and once for the text after the
selection; note that if we have a range selection, these pieces of text may not
be contiguous).

Bug: 802381
Change-Id: Ia2194ecc98c1a6767fc8a088fff74efe819153f7
Reviewed-on: https://chromium-review.googlesource.com/868952
Reviewed-by: Kent Tamura <tkent@chromium.org>
Reviewed-by: Xiaocheng Hu <xiaochengh@chromium.org>
Commit-Queue: Ryan Landay <rlanday@chromium.org>
Cr-Commit-Position: refs/heads/master@{#529657}
[modify] https://crrev.com/6d163ae1380a2a69f713bf18203cff25de515c46/content/shell/test_runner/text_input_controller.cc
[modify] https://crrev.com/6d163ae1380a2a69f713bf18203cff25de515c46/content/shell/test_runner/text_input_controller.h
[modify] https://crrev.com/6d163ae1380a2a69f713bf18203cff25de515c46/third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-ime.html

The second input event, for the deletion after the selection, should have inputType=deleteContentForward instead of deleteContentBackward.
Project Member

Comment 4 by bugdroid1@chromium.org, Jan 18 2018

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

commit 0d331b4d9460618ebf29546593e74f940b77621d
Author: Ryan Landay <rlanday@chromium.org>
Date: Thu Jan 18 19:58:31 2018

Add a missing beforeinput event in InputMethodController::SetComposition()

Currently, if an IME calls InputMethodController::SetComposition() with an empty
replacement string when no composition is open but some text is selected (so
the effect is to delete the selected text), we fire an input event, but not a
beforeinput event. This CL adds the beforeinput event.

This CL also fixes the same issue for
InputMethodController::DeleteSurroundingText().

Bug:  796690 ,802381
Change-Id: Icc0226103e515d51aa58ab7c8c4bed1f1180b0a3
Reviewed-on: https://chromium-review.googlesource.com/862521
Commit-Queue: Ryan Landay <rlanday@chromium.org>
Reviewed-by: Yoshifumi Inoue <yosin@chromium.org>
Reviewed-by: Xiaocheng Hu <xiaochengh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#530239}
[modify] https://crrev.com/0d331b4d9460618ebf29546593e74f940b77621d/third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-ime.html
[modify] https://crrev.com/0d331b4d9460618ebf29546593e74f940b77621d/third_party/WebKit/Source/core/editing/ime/InputMethodController.cpp
[modify] https://crrev.com/0d331b4d9460618ebf29546593e74f940b77621d/third_party/WebKit/Source/core/editing/ime/InputMethodController.h

Cc: rlanday@chromium.org
Owner: ----
Status: Available (was: Assigned)
I fixed the main issue here (beforeinput not being fired; we decided it makes sense for input to be fired twice).

We decided the second input event should have inputType=deleteContentForward instead of deleteContentBackward, and have not fixed that yet.
Glad to see the aspiration to address this bug; I am currently adding a keydown handler kludge to register which key initiated the delete and override deleteContentBackward => deleteContentForward for (at least the common, trivially known case).

Sign in to add a comment