Omit calls to set composing region when pasting image.

Omit calls to set composing region when pasting image.

For some reason, JellyBean and below make an extra call
(not present in KitKcat or later) to set the entire region
as the "composing region" after an image is pasted.  This
patch just skips that call when image place-holders are
present.  You can't compose with an image anyway.

BUG=466755
Committed: https://crrev.com/711e1ea0a89d7b2acf88857222fff00ec7d07727
Cr-Commit-Position: refs/heads/master@{#333274}

[bcwhite, 2015-04-28 19:06:20]

bcwhite@chromium.org changed reviewers:
+ aurimas@chromium.org

[aurimas (slooooooooow), 2015-04-28 21:08:27]

This entire thing looks sooo hacky. Is there really no better way of doing this?

[bcwhite, 2015-04-29 12:01:19]

On 2015/04/28 21:08:27, aurimas wrote:
> This entire thing looks sooo hacky. Is there really no better way of doing
this?

Older Android builds are calling setComposingRegion(0,1) covering the single
"object" that was pasted.  It seems to me that making a composition out of an
object is not really something you'd ever want to do.

I could generalize it to ensure there are no "object" codes anywhere in the
composing region but I went with the single-character case for speed and to
limit changes when this gets merged to Beta.

[bcwhite, 2015-04-29 13:41:58]

On 2015/04/29 12:01:19, bcwhite wrote:
> On 2015/04/28 21:08:27, aurimas wrote:
> > This entire thing looks sooo hacky. Is there really no better way of doing
> this?
> 
> Older Android builds are calling setComposingRegion(0,1) covering the single
> "object" that was pasted.  It seems to me that making a composition out of an
> object is not really something you'd ever want to do.
> 
> I could generalize it to ensure there are no "object" codes anywhere in the
> composing region but I went with the single-character case for speed and to
> limit changes when this gets merged to Beta.

The more I think about it, the more I think the rule should simply be to ignore
compositions that encompass an "object".  It's more generic and a legitimate
constraint, I believe.

[aurimas (slooooooooow), 2015-05-01 18:40:01]

On 2015/04/29 at 13:41:58, bcwhite wrote:
> On 2015/04/29 12:01:19, bcwhite wrote:
> > On 2015/04/28 21:08:27, aurimas wrote:
> > > This entire thing looks sooo hacky. Is there really no better way of doing
> > this?
> > 
> > Older Android builds are calling setComposingRegion(0,1) covering the single
> > "object" that was pasted.  It seems to me that making a composition out of
an
> > object is not really something you'd ever want to do.
> > 
> > I could generalize it to ensure there are no "object" codes anywhere in the
> > composing region but I went with the single-character case for speed and to
> > limit changes when this gets merged to Beta.
> 
> The more I think about it, the more I think the rule should simply be to
ignore compositions that encompass an "object".  It's more generic and a
legitimate constraint, I believe.

I vaguely remember that some IMEs use setcomposition to delete text, they first
call setComposingRegion(x,y) and then confirmComposition(""). Your change would
break that, right?

[bcwhite, 2015-05-04 15:44:04]

> I vaguely remember that some IMEs use setcomposition to delete text, they
first
> call setComposingRegion(x,y) and then confirmComposition(""). Your change
would
> break that, right?

That it would.  My testing included only the Google Keyboard.  When I tried it
under Lollipop, the sequence for backspace over the image is:

W/AdapterInputConnection(22652): beginBatchEdit [true]
W/AdapterInputConnection(22652): deleteSurroundingText [1 0]
W/AdapterInputConnection(22652): beginBatchEdit [false]
W/AdapterInputConnection(22652): endBatchEdit [false]
W/AdapterInputConnection(22652): endBatchEdit [true]
W/AdapterInputConnection(22652): updateSelectionIfRequired [0 0] [-1 -1]
W/AdapterInputConnection(22652): updateState [] [0 0] [-1 -1] [false]

Under Lollipop but selecting the image and hitting backspace:

W/AdapterInputConnection(22652): beginBatchEdit [true]
W/AdapterInputConnection(22652): setSelection [1 1]
W/AdapterInputConnection(22652): deleteSurroundingText [1 0]
W/AdapterInputConnection(22652): beginBatchEdit [false]
W/AdapterInputConnection(22652): endBatchEdit [false]
W/AdapterInputConnection(22652): updateState [] [1 1] [-1 -1] [false]
W/AdapterInputConnection(22652): endBatchEdit [true]
W/AdapterInputConnection(22652): updateSelectionIfRequired [0 0] [-1 -1]
W/AdapterInputConnection(22652): updateState [] [0 0] [-1 -1] [false]

In both cases, "deleteSurroundingText" is called and operates properly. 
Switching to JellyBean...  Backspace yields either:

W/AdapterInputConnection( 2308): beginBatchEdit [true]
W/AdapterInputConnection( 2308): setComposingText [] [1]
W/AdapterInputConnection( 2308): beginBatchEdit [false]
W/AdapterInputConnection( 2308): endBatchEdit [false]
W/AdapterInputConnection( 2308): endBatchEdit [true]

which does not succeed or it does:

W/AdapterInputConnection( 2308): beginBatchEdit [true]
W/AdapterInputConnection( 2308): deleteSurroundingText [1 0]
W/AdapterInputConnection( 2308): beginBatchEdit [false]
W/AdapterInputConnection( 2308): endBatchEdit [false]
W/AdapterInputConnection( 2308): endBatchEdit [true]
W/AdapterInputConnection( 2308): updateSelectionIfRequired [0 0] [-1 -1]
W/AdapterInputConnection( 2308): updateState [] [0 0] [-1 -1] [false]

which does succeed.  The first press follows the first sequence and the second
press follow the second sequence.  Selecting the image and then pressing
backspace always does:

W/AdapterInputConnection( 2308): beginBatchEdit [true]
W/AdapterInputConnection( 2308): setSelection [1 1]
W/AdapterInputConnection( 2308): deleteSurroundingText [1 0]
W/AdapterInputConnection( 2308): beginBatchEdit [false]
W/AdapterInputConnection( 2308): endBatchEdit [false]
W/AdapterInputConnection( 2308): updateState [] [1 1] [-1 -1] [false]
W/AdapterInputConnection( 2308): endBatchEdit [true]
W/AdapterInputConnection( 2308): updateSelectionIfRequired [0 0] [-1 -1]
W/AdapterInputConnection( 2308): updateState [] [0 0] [-1 -1] [false]

which works.

The fix actually works pretty well but I've done some more testing where
different problems start appearing when pasting multiple times or among other
text.


I'll take a look at this on the native side.  The actual problem is a call
upwards from Blink that calls updateState(...) with empty content after a call
to nativeSetComposingRegion(...) with the object.  Lollipop doesn't make this
call.

[bcwhite, 2015-05-25 17:04:47]

Fixing this on the Blink side turned out not to be practical.  I've updated the
patch to be more limited so that it only fixes the specific bug.

This also avoids the backspace/delete issue you mentioned because those still
pass freely.  Testing shows that the call to setComposingRegion() only occurs
when deleting an image when there is preceding text.  For all but deleting an
image after a single character, a!=0.  When that is the case, there is only 1
character and it's not an image placeholder.  Two images at the start do not
trigger this call.

[aurimas (slooooooooow), 2015-05-27 20:28:13]

https://codereview.chromium.org/1115473002/diff/60001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java
(right):

https://codereview.chromium.org/1115473002/diff/60001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java:500:
// This fixes the problem where, on Android 4.3, pasting an image is followed
What does keyboard do if we do not set the composing region?

[bcwhite, 2015-05-28 11:18:15]

https://codereview.chromium.org/1115473002/diff/60001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java
(right):

https://codereview.chromium.org/1115473002/diff/60001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java:500:
// This fixes the problem where, on Android 4.3, pasting an image is followed
On 2015/05/27 20:28:13, aurimas wrote:
> What does keyboard do if we do not set the composing region?

I can find no difference in the behavior of the keyboard but I'll ask Sam if he
can try it with Asian keyboards.  Lollipop doesn't even make this call.

[aurimas (slooooooooow), 2015-05-28 22:22:42]

lgtm

[bcwhite, 2015-06-04 21:09:33]

I've verified with Sam that everything still works fine even on Asian keyboards.
 Committing.

[bcwhite, 2015-06-04 21:09:38]

The CQ bit was checked by bcwhite@chromium.org

[commit-bot: I haz the power, 2015-06-04 21:10:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1115473002/60001

[commit-bot: I haz the power, 2015-06-04 21:33:05]

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

[commit-bot: I haz the power, 2015-06-04 21:33:06]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[bcwhite, 2015-06-08 15:29:22]

The CQ bit was checked by bcwhite@chromium.org

[commit-bot: I haz the power, 2015-06-08 15:29:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1115473002/60001

[commit-bot: I haz the power, 2015-06-08 16:23:46]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-06-08 16:24:33]

Patchset 4 (id:??) landed as
https://crrev.com/711e1ea0a89d7b2acf88857222fff00ec7d07727
Cr-Commit-Position: refs/heads/master@{#333274}