Note: in (3), we need to select the characters first.

I took an initial look and it seems that the current logic of InputMethodController may need to change: we select the composition, delete it, and insert new text. But in order to keep the style, we may need to calculate the diff and apply it separately.

The problem is that the keyboard app is asking us to replace the entire word when you type a character at the end, which naturally makes us clobber the multi-node substructure.

Yes, I think most of these cases could be fixed by, in InputMethodController::confirmComposition(), running a TextIterator forward character by character over both the selected text and the new text, and shrinking both as long as the characters match.

This is another variant of http://crbug.com/488628 which I fixed so I'll see if I can find the cycles to fix this one too.

 Issue 644586  has been merged into this issue.

Context from the other bug: "On the internal bug, we discussed changing Google Keyboard to commit one letter at a time to avoid this, but yukawa@ opposes this because the Android APIs force a whole-word composition in order to support nonstandard underline colors (because that's a span object starting at the beginning of the word)."

So we'll go with the Blink-side diff calculation (yabinh@ is currently working on this).

By the way, there is another bug similar to this one:

Steps to reproduce:
(1): Same to the above.
(2): Type 2 words separated by space, e.g: "hello world"
(3): Same to the above.
(4): Go to the start of "world", and wait until the whole text gets underlined.
(5): Now press backspace key to delete the space, and observe that bold style has been removed.

The reason is that, when we press the backspace key, we will:
1. set "world" to be composing text.
2. confirm the composing text. (We don't lose style at this step.)
3. select "world", and call extendSelectionAndDelete(1,0) to delete "world" and the space. We lose style at this step
4. commitText("world") to get "world" back.

Clarify: at step(3), we should boldify the character(s) in "world", like "or".

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

commit 993074d17afee685ced99283d7ffbe809fdb6f99
Author: yabinh <yabinh@chromium.org>
Date: Wed Oct 12 11:33:10 2016

Workaround for setComposition styling clobber

In order to keep the previous text formatting when continuing typing on
the word, this CL detects when the beginning of the IME commit is
the same at what was there already, and restricts the write to only
the character(s) at the end that actually changed. (This CL is based on
aelias@'s CL: https://codereview.chromium.org/2073863003)

BUG= 620549 

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

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

Your change meets the bar and is auto-approved for M55 (branch: 2883)

[Bulk edit]

Please process this M55 merge by landing the appropriate CLs on branch 2883 by Monday at 5 PM PT, or it will miss our beta candidate build.  Ping me if you have questions / concerns.

This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

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

commit 518012b2b00f0296b957476c797528a9ea03391c
Author: Changwan Ryu <changwan@google.com>
Date: Mon Oct 17 01:23:09 2016

Workaround for setComposition styling clobber

In order to keep the previous text formatting when continuing typing on
the word, this CL detects when the beginning of the IME commit is
the same at what was there already, and restricts the write to only
the character(s) at the end that actually changed. (This CL is based on
aelias@'s CL: https://codereview.chromium.org/2073863003)

BUG= 620549 

Review-Url: https://codereview.chromium.org/2372493002
Cr-Commit-Position: refs/heads/master@{#424716}
(cherry picked from commit 993074d17afee685ced99283d7ffbe809fdb6f99)

Review URL: https://codereview.chromium.org/2427433002 .

Cr-Commit-Position: refs/branch-heads/2883@{#132}
Cr-Branched-From: 614d31daee2f61b0180df403a8ad43f20b9f6dd7-refs/heads/master@{#423768}

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

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

commit 518012b2b00f0296b957476c797528a9ea03391c
Author: Changwan Ryu <changwan@google.com>
Date: Mon Oct 17 01:23:09 2016

Workaround for setComposition styling clobber

In order to keep the previous text formatting when continuing typing on
the word, this CL detects when the beginning of the IME commit is
the same at what was there already, and restricts the write to only
the character(s) at the end that actually changed. (This CL is based on
aelias@'s CL: https://codereview.chromium.org/2073863003)

BUG= 620549 

Review-Url: https://codereview.chromium.org/2372493002
Cr-Commit-Position: refs/heads/master@{#424716}
(cherry picked from commit 993074d17afee685ced99283d7ffbe809fdb6f99)

Review URL: https://codereview.chromium.org/2427433002 .

Cr-Commit-Position: refs/branch-heads/2883@{#132}
Cr-Branched-From: 614d31daee2f61b0180df403a8ad43f20b9f6dd7-refs/heads/master@{#423768}

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

According to my tests (Android Canary 56.0.2902.0) the patch only fixed issue for |setComposition()|, e.g. during composition updates. But didn't fix confirmComposition, e.g. |InputMethodController::commitText()| and |InputMethodController::finishComposingText()|.

Which means the existing style will be preserved during composition updates, but will still get lost on hitting Enter or tapping blank space. Apologize if I missed something.

That was also reported in https://buganizer.corp.google.com/issues/27889099#comment36 .  But the reason is that the IME likes to perform whole-word-spellcheck-replacement on enter or space.  There's no sane way to preserve style in that case.  I think this is only a bug if you observe style is lost in the case where IME chose not to make any correction.

I'm not so familiar with IME side spellcheck but it feels to me that if we can do the diff on full-word-composition-update we will also be able to handle whole-word-spellcheck-replacement?

On the other hand, if that's the case I cannot find any behavior difference before/after patch (except during composition), is that expected?

e.g. For test page https://jsbin.com/luxaqinicu (Google Keyboard Engligh US)
  1. Hitting IME suggested word preserves style both before/after patch (when the caret is at the right side of the word)
  2. Appending characters to the end of the word and hit Enter/Space doesn't preserve style both before/after patch

> I'm not so familiar with IME side spellcheck but it feels to me that if we can do the diff on full-word-composition-update we will also be able to handle whole-word-spellcheck-replacement?

Correct, the diffing code should work just the same -- but only if the spellcheck-replacement applies to suffix characters only.  If there is a diff midway through the word the algorithm either gives up or treats it as a very long suffix (and the entire suffix is always converted to a single style).

> e.g. For test page https://jsbin.com/luxaqinicu (Google Keyboard Engligh US)

This report is too vague because I don't know exactly what you chose to type and what the IME did as a result.  A video would help (it is quite easy to take a video -- use the command "adb shell screenrecord /sdcard/demo.mp4 && adb pull /sdcard/demo.mp4").

Here is the video of 3 test cases, (original word "k<b>i</b>d"):
  1. Hit IME suggested word "kids" -> preserves style before&after patch
  2. Append 's' and hit Enter -> doesn't preserve after hitting Enter
    * e.g. "k<b>i</b>d" => "k<b>i</b>ds" => "kids\n"
  3. Append 's' and hit Space -> similar to 2.

Hope that helps.

Deleted: demo.mp4 3.8 MB

	demo.mp4 3.8 MB View Download

Hmm, yeah, that's pretty bad.  I can easily repro locally and it seems univerally broken.  I seem to remember this worked in an earlier patchset I tested but it might've got broken by one of the changes recommended during code review.

Also according to https://www.w3.org/TR/uievents/#event-type-compositionupdate
```
CompositionEvent.data : the string comprising the current results of the composition session, which MAY be the empty string if the content has been deleted
```

I think it means |'compositionupdate'.data| should be the entire marked text (which matches FF and Safari), but the patch changed it to the diff of the marked text (see attached SS).

Deleted: Before.png 333 KB

	Before.png 333 KB View Download

Deleted: After.png 374 KB

	After.png 374 KB View Download

Yeah, the event of setComposition() is problematic.

Another problem: In order to keep the style, we shouldn't replace the composing text in finishComposingText(). That means no "beforeinput" or "input" event gets fired, which seem to be wrong. See InputMethodControllerTest#CompositionInputEventData for more details.

> In order to keep the style, we shouldn't replace the composing text in finishComposingText().

Yes that's one problem I'm trying to solve. My plan was to delete and reinsert the styled text by
  1. Calling |DeleteSelectionCommand| to delete composition string
  2. And calling |ensureComposition()->unapply()| to recover the delete string.

Does that make sense to you?

Yeah, that sounds reasonable to me for finishComposingText().
Besides, I think we also need delete and reinsert for setComposition() if we want to get the correct events.

Reply to comment#24:

I tried, and found that deleting and canceling will fire 2 "input.data:null" event. So that method may not work. See the patch https://codereview.chromium.org/2469983003/#ps1

I have another idea.  How about adding a new TypingCommand::insertText option flag to make TypingCommand do nothing (except clear the selection) if the plain-text versions of selected text and new text are equal?  Then you can delete m_isDirty and the early return block in replaceComposition() is unneeded.  The codepaths would become more unified and the events will be naturally correct in all cases.

(In fact, perhaps the better way to fix incremental event bug discovered in #22 is to implement the diffing algorithm inside TypingCommand instead of at a higher level?)

yosin@, what do you think of this idea?

I love higher level approach or implement separate function, e.g. TypeCommandForInputMethod or InputMethodController::TypingCommand.

Since TypeingCommand is already super complex, so I don't want to make more complex due by yet another flag.

That makes sense.  The problem is that the higher-level approach hasn't been working that well for this because it has caused branched/early-return codepaths that have trouble performing correct side effects like sending events.  So I'd like the style preservation logic to be moved a bit deeper, inside the switch statement of insertTextDuringCompositionWithEvents().

But it can be a custom method or custom CompositeEditCommand subclass instead of adding more options to TypingCommand, sure.

How about this:
  1. Introduce |InsertIncrementalTextCommand : CompositeEditCommand|, which holds all the diff logic;
  2. In |insertTextDuringCompositionWithEvents()|, pass an option to |TypingCommand::insertText()|;
  3. Then in |TypingCommand::doApply()|, map the option to new method |TypingCommand::insertIncrementalText()| (instead of another |TypingCommand::insertText()|) and utilize |InsertIncrementalTextCommand|.

Does that sound like a workable solution?

That sounds workable, except that yosin@ said he doesn't want to add any more option to TypingCommand for code complexity reasons, so is there a way we could write the diffing code somewhere outside TypingCommand (while still having it run in the same timing inside insertTextDuringCompositionWithEvents() rather than where it runs today)?

It is not mandatory to use TypingCommand::insertText() to insert text.  It might be OK to duplicate a bit of the code in TypingCommand::insertText() into a new class if needed.  Composition text insertion has different needs from normal web-exposed TypingCommand and perhaps it's not appropriate after all for them to both use the same function call.

[Automated comment] removing mislabelled merge-merged-2840

>It is not mandatory to use TypingCommand::insertText() to insert text.  It might be OK to duplicate a bit of the code in TypingCommand::insertText() into a new class if needed.

Do you mean introducing TypingCommand::insertIncrementalText() and calling it instead of TypingCommand::insertText() in insertTextDuringCompositionWithEvents()? Let's assume the whole text is "hello" and the incremental text is "llo". We need dispatch "input:hello" event and insert "llo" actually. But in TypingCommand::insertText(), we will update the inserting text after dispatching.(newText = dispatchBeforeTextInsertedEvent()). If we also update in TypingCommand::insertIncrementalText(), the inserting text will become "hello". How should we handle that?

Another problem. In insertTextDuringCompositionWithEvents(), setComposition() goes to insertText(), and finishComposingText() goes to frame.eventHandler().handleTextInputEvent(). That means the above method only deals with setComposition(). Should we change finishComposingText() to call TypingCommand::insertIncrementalText()?  

> If we also update in TypingCommand::insertIncrementalText(), the inserting text will become "hello". How should we handle that?

The insertIncrementalText() should take as parameter/state both the entire Range for the old text as well as the entire new text, to perform the appropriate diffing.

> Should we change finishComposingText() to call TypingCommand::insertIncrementalText()?  

Yes.

[Bulk edit]

URGENT - PTAL ASAP.

This issue is marked as a stable release blocker for Android M55.  We will be cutting our stable candidate from branch 2883 on Tuesday, 11/29, so *all* blockers must be fixed on trunk by Monday, 11/28 to allow time to merge back to the branch.  Please review this issue ASAP.

Know that this issue shouldn't block the release?  Remove the ReleaseBlock-Stable label.

Unsure if the issue should block the release, or think that the issue should block the release but you know you won't be able to fix it in time?  Please CC me to the bug so that we can discuss options.

Thanks!

"
The textInput event, originally proposed as a replacement for keypress, was removed in favor of the current beforeinput and input events.
"
(https://www.w3.org/TR/uievents/#event-flow)

But we are still listening to textInput event in "ime-composition-events-001-expected.txt" for now. I suggest we replace it with beforeinput and input events in that file. What do you think?

Removing "ReleaseBlock-Stable" because I don't think this can go to M55 in time.

I have a CL on this and it's still under review now. See:
https://codereview.chromium.org/2530843003/

Thanks for the CL! However I think we cannot remove 'textInput' event due to it's high usage (0.0626%), see:
https://www.chromestatus.com/metrics/feature/timeline/popularity/831

Will comment on the CL.

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

commit dc5b3fb4ceeeb88b2b5cbdfc8065fae2c73b0d20
Author: yabinh <yabinh@chromium.org>
Date: Fri Dec 02 10:03:01 2016

Introduce 'TextCompositionCancel' for cancelComposition

cancelComposition and other functions (e.g finishComposingText) share the same
tag "TextCompositionConfirm". In order to keep the former's behavior and change
the others', this CL introduces "TextCompositionCancel" for cancelComposition,
and assigns the old tag to other functions. This is a pre-CL for
https://codereview.chromium.org/2530843003/

BUG= 620549 

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

[modify] https://crrev.com/dc5b3fb4ceeeb88b2b5cbdfc8065fae2c73b0d20/third_party/WebKit/Source/core/editing/InputMethodController.cpp
[modify] https://crrev.com/dc5b3fb4ceeeb88b2b5cbdfc8065fae2c73b0d20/third_party/WebKit/Source/core/editing/commands/TypingCommand.h

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

commit 74eab98911047f5d59f89dddaeb412ea6f77718c
Author: yabinh <yabinh@chromium.org>
Date: Fri Dec 09 09:15:59 2016

Fix calculateCharacterSubrange() when length == 0

When length == 0, it should return a collasped range instead of a range
with distance == 1.
This is pre-CL of https://codereview.chromium.org/2530843003/

BUG= 620549 

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

[modify] https://crrev.com/74eab98911047f5d59f89dddaeb412ea6f77718c/third_party/WebKit/Source/core/editing/iterators/CharacterIterator.cpp
[modify] https://crrev.com/74eab98911047f5d59f89dddaeb412ea6f77718c/third_party/WebKit/Source/core/editing/iterators/CharacterIteratorTest.cpp

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

commit ef5c846a3e5d3c994d962ddca7d40f4bc1476044
Author: yabinh <yabinh@chromium.org>
Date: Wed Dec 14 12:38:45 2016

Introduce InsertIncrementalTextCommand to respect existing style for composition

In order to keep the previous text formatting when continuing typing on the word,
confirming the ongoing composition, or inserting new text, this CL introduces
InsertIncrementalTextCommand for insertion. The command detects  what was
there already, and restricts the write to the character(s) that actually changed.
Note that this CL doesn't change the behavior of events, i.e., it still sends
'beforeinput', 'input', and 'textInput' events with the entire text.

BUG= 620549 

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

[modify] https://crrev.com/ef5c846a3e5d3c994d962ddca7d40f4bc1476044/content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java
[modify] https://crrev.com/ef5c846a3e5d3c994d962ddca7d40f4bc1476044/third_party/WebKit/Source/core/editing/BUILD.gn
[modify] https://crrev.com/ef5c846a3e5d3c994d962ddca7d40f4bc1476044/third_party/WebKit/Source/core/editing/Editor.cpp
[modify] https://crrev.com/ef5c846a3e5d3c994d962ddca7d40f4bc1476044/third_party/WebKit/Source/core/editing/InputMethodController.cpp
[modify] https://crrev.com/ef5c846a3e5d3c994d962ddca7d40f4bc1476044/third_party/WebKit/Source/core/editing/InputMethodController.h
[modify] https://crrev.com/ef5c846a3e5d3c994d962ddca7d40f4bc1476044/third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp
[add] https://crrev.com/ef5c846a3e5d3c994d962ddca7d40f4bc1476044/third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommand.cpp
[add] https://crrev.com/ef5c846a3e5d3c994d962ddca7d40f4bc1476044/third_party/WebKit/Source/core/editing/commands/InsertIncrementalTextCommand.h
[modify] https://crrev.com/ef5c846a3e5d3c994d962ddca7d40f4bc1476044/third_party/WebKit/Source/core/editing/commands/InsertTextCommand.h
[modify] https://crrev.com/ef5c846a3e5d3c994d962ddca7d40f4bc1476044/third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp
[modify] https://crrev.com/ef5c846a3e5d3c994d962ddca7d40f4bc1476044/third_party/WebKit/Source/core/editing/commands/TypingCommand.h
[modify] https://crrev.com/ef5c846a3e5d3c994d962ddca7d40f4bc1476044/third_party/WebKit/Source/core/events/TextEvent.h
[modify] https://crrev.com/ef5c846a3e5d3c994d962ddca7d40f4bc1476044/third_party/WebKit/Source/core/events/TextEventInputType.h
[modify] https://crrev.com/ef5c846a3e5d3c994d962ddca7d40f4bc1476044/third_party/WebKit/Source/web/tests/WebViewTest.cpp

Locally confirmed fixed on r433351 on https://jsfiddle.net/9n17does.  Seems a bit too large to pull to release branch, so let's let it roll out as part of M57.

 Issue 672703  has been merged into this issue.

 Issue 677050  has been merged into this issue.