Support parsing BackgroundSpans and UnderlineSpans in Android IME's commitText()

Support parsing BackgroundSpans and UnderlineSpans in Android IME's commitText()

Android InputConnection has two related methods

- setComposingText() replaces the current compose region with a given piece of
  text and sets a composing span around the new text

- commitText() replaces the text the same way but then closes out the compose
  region instead of putting it around the new text.

Support was added for parsing BackgroundSpan and UnderlineSpan annotations in
setComposingText() to support the Google Japanese Input IME (newly entered text
in the compose region is underlined and highlighted in a blue background).

The Android Voice IME instead uses commitText() because it wants the highlighted
regions to persist after the compose region is closed out. Closing out the
compose region after calling setComposingText() would clear the highlight
regions. So to support this, we need to parse the spans for commitText() in
ImeAdapterAndroid just as we do for setComposingText(), and also thread this
information all the way back to InputMethodController as we do for
setComposingText().

I tested this by forwarding calls of setComposingText() to commitText() and
verifying that highlighting works for the Google Japanese IME. It's highlighting
the character immediately ahead of the insertion point instead of the newly
typed character when I do this; I suspect this may be because the IME is
expecting something about the setComposingText() behavior (e.g. the compose
range) that's different in commitText(), but it's also possible I have a bug in
the new logic.

Once I write some code to start parsing out SuggestionSpans from the Voice IME,
I'll be able to test this better (I might be able to get this done tomorrow).

BUG=673491
Review-Url: https://codereview.chromium.org/2568093003
Cr-Commit-Position: refs/heads/master@{#442738}
Committed: https://chromium.googlesource.com/chromium/src/+/7efe230106176ba759388532a0993f0f0bef67d8

[rlanday, 2016-12-13 02:32:01]

rlanday@chromium.org changed reviewers:
+ aelias@chromium.org, dcheng@chromium.org, pfeldman@chromium.org

[rlanday, 2016-12-13 02:32:01]

rlanday@chromium.org changed required reviewers:
+ aelias@chromium.org, dcheng@chromium.org, pfeldman@chromium.org

[rlanday, 2016-12-13 02:32:01]

pfeldman, I'm adding you as a reviewer since you own these files:
  components/test_runner/text_input_controller.cc
  content/browser/browser_plugin/browser_plugin_guest.cc
  content/browser/browser_plugin/browser_plugin_guest.h
  third_party/WebKit/Source/core/editing/InputMethodController.cpp
  third_party/WebKit/Source/core/editing/InputMethodController.h

dcheng, you own these files:
  content/common/browser_plugin/browser_plugin_messages.h
  content/common/input_messages.h

aelias, you own the remaining files

[rlanday, 2016-12-13 18:33:38]

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

[commit-bot: I haz the power, 2016-12-13 18:34:23]

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

[commit-bot: I haz the power, 2016-12-13 18:50:46]

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

[commit-bot: I haz the power, 2016-12-13 18:50:47]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[rlanday, 2016-12-13 19:42:20]

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

[commit-bot: I haz the power, 2016-12-13 19:43:07]

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

[commit-bot: I haz the power, 2016-12-13 20:23:18]

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

[commit-bot: I haz the power, 2016-12-13 20:23:20]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[rlanday, 2016-12-14 00:04:53]

https://codereview.chromium.org/2568093003/diff/20001/content/browser/rendere...
File content/browser/renderer_host/ime_adapter_android.cc (right):

https://codereview.chromium.org/2568093003/diff/20001/content/browser/rendere...
content/browser/renderer_host/ime_adapter_android.cc:356:
underlines.push_back(blink::WebCompositionUnderline(
Actually I think we probably don't want this for commitText()...

[aelias_OOO_until_Jul13, 2016-12-14 00:48:43]

aelias@chromium.org changed reviewers:
+ yutak@chromium.org

[aelias_OOO_until_Jul13, 2016-12-14 00:50:39]

Adding yutak@ for Source/core/editing OWNERS (and cc yosin@ who is on vacation
for the month).  This makes the underlines persist outside composition which is
different semantics from before, but I think that's appropriate (Android edit
box behavior is to make them persist, and I think it's better to keep using the
marking mechanism rather than try to inject stuff into DOM).

[rlanday, 2016-12-14 03:01:30]

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

[commit-bot: I haz the power, 2016-12-14 03:02:21]

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

[commit-bot: I haz the power, 2016-12-14 04:40:51]

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

[commit-bot: I haz the power, 2016-12-14 04:40:52]

Dry run: This issue passed the CQ dry run.

[rlanday, 2016-12-14 18:55:14]

On 2016/12/14 at 00:50:39, aelias wrote:
> Adding yutak@ for Source/core/editing OWNERS (and cc yosin@ who is on vacation
for the month).  This makes the underlines persist outside composition which is
different semantics from before, but I think that's appropriate (Android edit
box behavior is to make them persist, and I think it's better to keep using the
marking mechanism rather than try to inject stuff into DOM).

We decided we need to add a flag to WebCompositionUnderline to specify which
underlines should be cleared on finishComposingText() to match the Android
behavior. On Android, spans passed through setComposingText() are cleared on
finishComposingText(), but not spans passed through commitText(). I think
there's also a way to set a flag manually on spans passed through commitText().
I think this can go in a separate CL that this one can be written on top of.

[rlanday, 2016-12-15 02:12:17]

I suspect the code for adding the underlines in commitText() is buggy, currently
investigating

[Yuta Kitamura, 2016-12-15 09:29:53]

Blink/core LGTM

Changes in Blink look reasonable to me, but ccing tkent@ for
extra eyes.

[dcheng, 2016-12-15 10:13:17]

ipc lgtm

https://codereview.chromium.org/2568093003/diff/40001/content/renderer/render...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/2568093003/diff/40001/content/renderer/render...
content/renderer/render_widget.cc:1555: controller->commitText(text,
WebVector<WebCompositionUnderline>(underlines),
I think the explicit call to WebVector is usually omitted, just for readability.

[tkent, 2016-12-16 00:52:40]

On 2016/12/15 at 09:29:53, yutak wrote:
> Blink/core LGTM
> 
> Changes in Blink look reasonable to me, but ccing tkent@ for
> extra eyes.

lgtm too

[rlanday, 2016-12-16 20:46:33]

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

[commit-bot: I haz the power, 2016-12-16 20:47:52]

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

[rlanday, 2016-12-16 20:55:33]

rlanday@chromium.org changed reviewers:
+ tkent@chromium.org

[rlanday, 2016-12-16 20:55:34]

I fixed the logic enough that when I draw the SuggestionSpans from the Voice IME
with a red background, they seem to get drawn in the right places. I have a
couple questions about things I think are are still not quite right though.

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

https://codereview.chromium.org/2568093003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:368:
addCompositionUnderlines(underlines, rootEditableElement, textStart);
I don't think the Voice IME actually ever replaces a composition span with text
containing underlines, I think it always blanks out the existing text first. So
I don't know that this actually works properly, but I assume we should support
this?

This code uses rootEditableElement() as opposed to the base node of the
selection. rootEditableElement() didn't seem to work properly in
insertTextAndMoveCaret() and I don't really understand why.

https://codereview.chromium.org/2568093003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:402: if
(!baseNode)
Note: if I add the baseNode->isTextNode() check that insertText() has, this
fails on a node whose path looks like this:
/HTML[0]/BODY[0]/FORM[@id="textform" and position()=0]/TEXTAREA[@id="text" and
position()=0]/#shadow-root[0]/DIV[@id="inner-editor"]

Not sure exactly what's going on.

[rlanday, 2016-12-16 21:01:35]

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

https://codereview.chromium.org/2568093003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:368:
addCompositionUnderlines(underlines, rootEditableElement, textStart);
On 2016/12/16 at 20:55:33, rlanday wrote:
> I don't think the Voice IME actually ever replaces a composition span with
text containing underlines, I think it always blanks out the existing text
first. So I don't know that this actually works properly, but I assume we should
support this?


I'm going to try to add a test case to InputMethodControllerTest.cpp that tests
this behavior, that should solve this problem...

https://codereview.chromium.org/2568093003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:368:
addCompositionUnderlines(underlines, rootEditableElement, textStart);
On 2016/12/16 at 20:55:33, rlanday wrote:
> I don't think the Voice IME actually ever replaces a composition span with
text containing underlines, I think it always blanks out the existing text
first. So I don't know that this actually works properly, but I assume we should
support this?


I'm going to try to add a test case to InputMethodControllerTest.cpp that tests
this behavior, that should solve this problem...

[commit-bot: I haz the power, 2016-12-16 22:35:38]

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

[commit-bot: I haz the power, 2016-12-16 22:35:39]

Dry run: This issue passed the CQ dry run.

[rlanday, 2016-12-17 02:56:38]

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

[commit-bot: I haz the power, 2016-12-17 02:57:09]

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

[commit-bot: I haz the power, 2016-12-17 03:00:26]

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

[commit-bot: I haz the power, 2016-12-17 03:00:27]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[rlanday, 2016-12-17 03:08:12]

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

[commit-bot: I haz the power, 2016-12-17 03:08:35]

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

[rlanday, 2016-12-17 03:08:40]

After talking to aelias, I unified all the underline-adding code to use
rootEditableElement() instead of the selection base element (the reason this
wasn't working properly before was that my addCompositionUnderlines method was
not properly constructing the range to count in plain text characters). I also
added a test case so we can verify the underlines get added properly for a
replace operation.

Note: I discovered when testing the test case that insertTextAndMoveCaret() is
actually used for replace operations too, if there's no compose span set so
we're replacing the selection instead...weird...

[commit-bot: I haz the power, 2016-12-17 03:13:17]

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

[commit-bot: I haz the power, 2016-12-17 03:13:18]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[rlanday, 2016-12-19 18:04:48]

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

[commit-bot: I haz the power, 2016-12-19 18:05:05]

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

[commit-bot: I haz the power, 2016-12-19 18:54:45]

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

[commit-bot: I haz the power, 2016-12-19 18:54:46]

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_arm6...)

[rlanday, 2016-12-19 19:42:04]

Apparently the test case doesn't like depending on DocumentMethodController.
Using it requires me to add this header:
#include
"third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h"

This seems to build fine, but then the analyze tool complains that core/editing
isn't listed as a dependency. But then if I list it as a dependency, the build
fails with a bunch of unresolved reference errors.

I'm currently not sure how to solve this problem, and I'm also not sure how to
write the test case without having access to the markers. There aren't currently
any methods in InputMethodController that expose them.

[Yuta Kitamura, 2016-12-20 05:28:20]

On 2016/12/19 19:42:04, rlanday wrote:
> Apparently the test case doesn't like depending on DocumentMethodController.
> Using it requires me to add this header:
> #include
> "third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h"
> 
> This seems to build fine, but then the analyze tool complains that
core/editing
> isn't listed as a dependency. But then if I list it as a dependency, the build
> fails with a bunch of unresolved reference errors.

That's dependency violation; you can't directly use WebKit/Source contents
from content/.

[rlanday, 2016-12-20 05:55:57]

On 2016/12/20 at 05:28:20, yutak wrote:
> On 2016/12/19 19:42:04, rlanday wrote:
> > Apparently the test case doesn't like depending on DocumentMethodController.
> > Using it requires me to add this header:
> > #include
> > "third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h"
> > 
> > This seems to build fine, but then the analyze tool complains that
core/editing
> > isn't listed as a dependency. But then if I list it as a dependency, the
build
> > fails with a bunch of unresolved reference errors.
> 
> That's dependency violation; you can't directly use WebKit/Source contents
> from content/.

I’m trying to use this file:
third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h

from this file:
third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp

They’re both in WebKit/Source/core/editing/. It looks like core/editing isn’t
set up as something that can be built independently (it appears that it can only
be built as part of core), so listing core/editing as a dep of the editing
unit_tests fails to build. One thing that seems to work is listing
//third_party/WebKit/Source/core/editing as a public_dep of core (together with
platform and wtf) so the header include becomes allowed:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/BUILD.gn?...

But that doesn‘t feel right. It seems like the way things are set up now, the
test case is not supposed to depend on any headers other than the one for the
class it’s testing, but I don’t know how to make it work that way because the
information I’m checking in the test case (the currently-set markers) is not
exposed in that class.

Other (crappy) potential solutions I can think of are including the
DocumentMarkerController.h header in InputMethodController.h even though it’s
not needed there, and adding a method in InputMethodController to expose the
information the test case needs (but it would be otherwise unused).

[Yuta Kitamura, 2016-12-20 07:09:14]

> I’m trying to use this file:
> third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h
>
> from this file:
> third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp

This is allowed. The error is probably caused by the way how
you write #include statement; see below.

Regarding the link error, you probably need to add CORE_EXPORT to
a few places to fix it. You need to expose the symbols needed by
the unit test target.

https://codereview.chromium.org/2568093003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp
(right):

https://codereview.chromium.org/2568093003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:19:
#include
"third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h"
You should write this as: #include "core/editing/..."

[rlanday, 2016-12-20 18:50:59]

On 2016/12/20 at 07:09:14, yutak wrote:
> Regarding the link error, you probably need to add CORE_EXPORT to
> a few places to fix it. You need to expose the symbols needed by
> the unit test target.

I think the link errors only came up when I was trying to mess with the
dependencies; since fixing the header include resolves the analyze issue without
having to do this, I don't think there will be any link errors to worry about.

[rlanday, 2016-12-20 19:12:41]

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

[commit-bot: I haz the power, 2016-12-20 19:13:29]

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

[rlanday, 2016-12-20 19:42:35]

Something is not right here; this is breaking highlighting in the Japanese input
IME...

[rlanday, 2016-12-20 20:34:43]

https://codereview.chromium.org/2568093003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp
(right):

https://codereview.chromium.org/2568093003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp:19:
#include
"third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h"
On 2016/12/20 at 07:09:14, Yuta Kitamura wrote:
> You should write this as: #include "core/editing/..."

Ah, that seems to resolve the issue. Thanks! I suppose I should've noticed the
other headers written like this...

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

https://codereview.chromium.org/2568093003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:627:
addCompositionUnderlines(underlines, rootEditableElement, textStart);
There's a bug here: this needs to use the composition range, not the selection
range. I will probably add a test case for this

[commit-bot: I haz the power, 2016-12-20 21:15:36]

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

[commit-bot: I haz the power, 2016-12-20 21:15:37]

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_...)

[rlanday, 2016-12-20 21:44:39]

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

[commit-bot: I haz the power, 2016-12-20 21:45:40]

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

[commit-bot: I haz the power, 2016-12-20 22:06:27]

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

[commit-bot: I haz the power, 2016-12-20 22:06:28]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[rlanday, 2017-01-09 19:24:23]

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

[commit-bot: I haz the power, 2017-01-09 19:24:55]

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

[commit-bot: I haz the power, 2017-01-09 21:37:59]

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

[commit-bot: I haz the power, 2017-01-09 21:38:00]

Dry run: This issue passed the CQ dry run.

[rlanday, 2017-01-09 21:55:17]

aelias + pfeldman, does this look good?

My current plan is still to pass the SuggestionSpans into commitText() in the
underlines vector, although it's possible we'll end up tweaking the API a bit to
further distinguish between SuggestionSpans and formatting-only spans.

https://codereview.chromium.org/2568093003/diff/200001/content/browser/render...
File content/browser/renderer_host/ime_adapter_android.cc (right):

https://codereview.chromium.org/2568093003/diff/200001/content/browser/render...
content/browser/renderer_host/ime_adapter_android.cc:170: void
ImeAdapterAndroid::CommitText(JNIEnv* env,
In theory I think this method should make the Android BackgroundSpans and
UnderlineSpans persist until the text is overwritten...I could do this but I
don't know of anything that actually needs this functionality...

[aelias_OOO_until_Jul13, 2017-01-10 01:45:35]

https://codereview.chromium.org/2568093003/diff/200001/content/browser/render...
File content/browser/renderer_host/ime_adapter_android.cc (right):

https://codereview.chromium.org/2568093003/diff/200001/content/browser/render...
content/browser/renderer_host/ime_adapter_android.cc:170: void
ImeAdapterAndroid::CommitText(JNIEnv* env,
On 2017/01/09 at 21:55:17, rlanday wrote:
> In theory I think this method should make the Android BackgroundSpans and
UnderlineSpans persist until the text is overwritten...I could do this but I
don't know of anything that actually needs this functionality...

I think that will likely be needed sooner or later (especially in mixed
voice/typing input modes), could you add a boolean to the
WebCompositionUnderline structure about whether it's persistent or not?

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

https://codereview.chromium.org/2568093003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:618: // Can't
use addCompositionUnderlines here because the ranges are constructed
Is there a good reason for the difference that you know of?  Could we switch it
to use the same method?

[rlanday, 2017-01-10 05:02:18]

https://codereview.chromium.org/2568093003/diff/200001/content/browser/render...
File content/browser/renderer_host/ime_adapter_android.cc (right):

https://codereview.chromium.org/2568093003/diff/200001/content/browser/render...
content/browser/renderer_host/ime_adapter_android.cc:170: void
ImeAdapterAndroid::CommitText(JNIEnv* env,
On 2017/01/10 at 01:45:35, aelias wrote:
> On 2017/01/09 at 21:55:17, rlanday wrote:
> > In theory I think this method should make the Android BackgroundSpans and
UnderlineSpans persist until the text is overwritten...I could do this but I
don't know of anything that actually needs this functionality...
> 
> I think that will likely be needed sooner or later (especially in mixed
voice/typing input modes), could you add a boolean to the
WebCompositionUnderline structure about whether it's persistent or not?

That’s basically what I did in
https://codereview.chromium.org/2611813002

It’s kind of a sizable CL, should I just revive it?

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

https://codereview.chromium.org/2568093003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:618: // Can't
use addCompositionUnderlines here because the ranges are constructed
On 2017/01/10 at 01:45:35, aelias wrote:
> Is there a good reason for the difference that you know of?  Could we switch
it to use the same method?

addCompositionUnderlines() takes a root element and an offset measured in plain
text characters which the underline offsets are added too. Here, the baseOffset
measures DOM child nodes of the base node, not plain text characters
(underline.startOffset() and underline.endOffset() are used the same way, I’m
not sure this is correct)...maybe we can consolidate it by passing a Position
instead of a base node? I'll take another look

[rlanday, 2017-01-10 19:23:10]

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

[commit-bot: I haz the power, 2017-01-10 19:23:31]

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

[rlanday, 2017-01-10 19:25:10]

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

https://codereview.chromium.org/2568093003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:618: // Can't
use addCompositionUnderlines here because the ranges are constructed
On 2017/01/10 at 05:02:17, rlanday wrote:
> On 2017/01/10 at 01:45:35, aelias wrote:
> > Is there a good reason for the difference that you know of?  Could we switch
it to use the same method?
> 
> addCompositionUnderlines() takes a root element and an offset measured in
plain text characters which the underline offsets are added too. Here, the
baseOffset measures DOM child nodes of the base node, not plain text characters
(underline.startOffset() and underline.endOffset() are used the same way, I’m
not sure this is correct)...maybe we can consolidate it by passing a Position
instead of a base node? I'll take another look

baseNode is guaranteed to be a text node (there's a check above), so
addCompositionUnderlines() actually works if I pass baseNode's parent
(baseOffset is the offset of baseNode relative to its parent, not an offset
added to baseNode like I think I thought it was)...I have updated the CL

[commit-bot: I haz the power, 2017-01-10 20:54:55]

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

[commit-bot: I haz the power, 2017-01-10 20:54:56]

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

[aelias_OOO_until_Jul13, 2017-01-10 21:46:14]

aelias@chromium.org changed reviewers:
+ fsamuel@chromium.org
- pfeldman@chromium.org

[aelias_OOO_until_Jul13, 2017-01-10 21:46:14]

aelias@chromium.org changed required reviewers:
+ fsamuel@chromium.org
- pfeldman@chromium.org

[aelias_OOO_until_Jul13, 2017-01-10 21:46:30]

https://codereview.chromium.org/2568093003/diff/200001/content/browser/render...
File content/browser/renderer_host/ime_adapter_android.cc (right):

https://codereview.chromium.org/2568093003/diff/200001/content/browser/render...
content/browser/renderer_host/ime_adapter_android.cc:170: void
ImeAdapterAndroid::CommitText(JNIEnv* env,
On 2017/01/10 at 05:02:17, rlanday wrote:
> On 2017/01/10 at 01:45:35, aelias wrote:
> > On 2017/01/09 at 21:55:17, rlanday wrote:
> > > In theory I think this method should make the Android BackgroundSpans and
UnderlineSpans persist until the text is overwritten...I could do this but I
don't know of anything that actually needs this functionality...
> > 
> > I think that will likely be needed sooner or later (especially in mixed
voice/typing input modes), could you add a boolean to the
WebCompositionUnderline structure about whether it's persistent or not?
> 
> That’s basically what I did in
> https://codereview.chromium.org/2611813002
> 
> It’s kind of a sizable CL, should I just revive it?

OK, this patch is large enough, let's land it as is instead of trying to include
more stuff just now.  Maybe the persistency boolean could be added in the same
patch that adds the list of strings for SuggestionSpan (since they're both
touching CompositionUnderline struct).

[aelias_OOO_until_Jul13, 2017-01-10 21:47:33]

Adding fsamuel@ for OWNERS of trivial changes in content/browser/browser_plugin/
and content_common/browser_plugin/.  (I think this should be the last OWNERS
needed -- pfeldman@ is a devtools reviewer.)

[aelias_OOO_until_Jul13, 2017-01-10 21:48:32]

lgtm for content/renderer, Source/web, content/browser/renderer_host/

[Fady Samuel, 2017-01-10 21:55:59]

browser_plugin lgtm

[rlanday, 2017-01-10 23:34:59]

The CQ bit was checked by rlanday@chromium.org

[rlanday, 2017-01-10 23:34:59]

The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org,
yutak@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2568093003/#ps220001
(title: "Use addCompositionUnderlines() where I said I couldn't")

[commit-bot: I haz the power, 2017-01-10 23:36:00]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-11 00:14:37]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1484091299429030,
"parent_rev": "abcbc58fa094efca8d7b46d6b6537c37b0e90e63", "commit_rev":
"7efe230106176ba759388532a0993f0f0bef67d8"}

[commit-bot: I haz the power, 2017-01-11 00:15:15]

Description was changed from

==========
Support parsing BackgroundSpans and UnderlineSpans in Android IME's commitText()

Android InputConnection has two related methods

- setComposingText() replaces the current compose region with a given piece of
  text and sets a composing span around the new text

- commitText() replaces the text the same way but then closes out the compose
  region instead of putting it around the new text.

Support was added for parsing BackgroundSpan and UnderlineSpan annotations in
setComposingText() to support the Google Japanese Input IME (newly entered text
in the compose region is underlined and highlighted in a blue background).

The Android Voice IME instead uses commitText() because it wants the highlighted
regions to persist after the compose region is closed out. Closing out the
compose region after calling setComposingText() would clear the highlight
regions. So to support this, we need to parse the spans for commitText() in
ImeAdapterAndroid just as we do for setComposingText(), and also thread this
information all the way back to InputMethodController as we do for
setComposingText().

I tested this by forwarding calls of setComposingText() to commitText() and
verifying that highlighting works for the Google Japanese IME. It's highlighting
the character immediately ahead of the insertion point instead of the newly
typed character when I do this; I suspect this may be because the IME is
expecting something about the setComposingText() behavior (e.g. the compose
range) that's different in commitText(), but it's also possible I have a bug in
the new logic.

Once I write some code to start parsing out SuggestionSpans from the Voice IME,
I'll be able to test this better (I might be able to get this done tomorrow).

BUG=673491
==========

to

==========
Support parsing BackgroundSpans and UnderlineSpans in Android IME's commitText()

Android InputConnection has two related methods

- setComposingText() replaces the current compose region with a given piece of
  text and sets a composing span around the new text

- commitText() replaces the text the same way but then closes out the compose
  region instead of putting it around the new text.

Support was added for parsing BackgroundSpan and UnderlineSpan annotations in
setComposingText() to support the Google Japanese Input IME (newly entered text
in the compose region is underlined and highlighted in a blue background).

The Android Voice IME instead uses commitText() because it wants the highlighted
regions to persist after the compose region is closed out. Closing out the
compose region after calling setComposingText() would clear the highlight
regions. So to support this, we need to parse the spans for commitText() in
ImeAdapterAndroid just as we do for setComposingText(), and also thread this
information all the way back to InputMethodController as we do for
setComposingText().

I tested this by forwarding calls of setComposingText() to commitText() and
verifying that highlighting works for the Google Japanese IME. It's highlighting
the character immediately ahead of the insertion point instead of the newly
typed character when I do this; I suspect this may be because the IME is
expecting something about the setComposingText() behavior (e.g. the compose
range) that's different in commitText(), but it's also possible I have a bug in
the new logic.

Once I write some code to start parsing out SuggestionSpans from the Voice IME,
I'll be able to test this better (I might be able to get this done tomorrow).

BUG=673491

Review-Url: https://codereview.chromium.org/2568093003
Cr-Commit-Position: refs/heads/master@{#442738}
Committed:
https://chromium.googlesource.com/chromium/src/+/7efe230106176ba759388532a099...
==========

[commit-bot: I haz the power, 2017-01-11 00:15:17]

Committed patchset #12 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/7efe230106176ba759388532a099...