1353799 - Cursor displayed at wrong position after entering space

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Keyboards and IME, defect)

Description

[(inactive) Jim Chen [:jchen] [:darchons]]

STR:
1. Using Gboard, enter a long word in a text field.
2. Place the cursor in the middle of the word.
3. Press space.

The cursor is displayed at the end of the new, second word, but if you enter a letter, the cursor jumps back to the correct position at the beginning of the second word. I suspect this is a regression from bug 1137567.

Comment 1

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: 1. Make icMaybeSendComposition accept a flag parameter (v1)

Use a flag parameter instead of individual boolean parameters to make
it more convenient to add more options.

Comment 2

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: 2. Update current composition when setting/removing spans (v1)

Update the composition when setting/removing spans, so that we update
the selection/cursor during a composition. However, we must limit any
updating to the current composition only (as indicated by the
keep-current-composition flag), because the Facebook comment box behaves
incorrectly if we repeatedly start and end new compositions.

Comment 3

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: 3. Add types and timestamps in testInputConnection (v1)

* Include the type of the editor (input, textarea, contentEditable,
  designMode) in BasicInputConnectionTest, so we can work around the
  differences in behavior among the different editor types.

* Add timestamps to key events, because lack of timestamps was
  triggering a crash when running testInputConnection.

Comment 4

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: 4. Add composition event tests to testInputConnection (v1)

Add two tests to testInputConnection that record the sequence of
composition events during editing, and check that the sequence is what
we expected.

The first test makes sure that we reuse the current composition on the
Gecko side when setting composing text; otherwise the Facebook comment
box behaves incorrectly.

The second test makes sure that we can move the cursor inside the
current composition, to fix this particular bug.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f21f31eafc6d
1. Make icMaybeSendComposition accept a flag parameter; r=esawin
https://hg.mozilla.org/integration/mozilla-inbound/rev/904e65ad1fb8
2. Update current composition when setting/removing spans; r=esawin
https://hg.mozilla.org/integration/mozilla-inbound/rev/29de8f4097d3
3. Add types and timestamps in testInputConnection; r=esawin
https://hg.mozilla.org/integration/mozilla-inbound/rev/94046d0bb7e3
4. Add composition event tests to testInputConnection; r=esawin

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f21f31eafc6d
https://hg.mozilla.org/mozilla-central/rev/904e65ad1fb8
https://hg.mozilla.org/mozilla-central/rev/29de8f4097d3
https://hg.mozilla.org/mozilla-central/rev/94046d0bb7e3

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval on this when you feel it's ready for uplift.

Comment 8

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Patch for Beta

Approval Request Comment
[Feature/Bug causing the regression]: bug 1307816
[User impact if declined]: Possible incorrect cursor placement when entering text (but only visually incorrect; functionality should be correct)
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: No
[Is the change risky?]: Somewhat
[Why is the change risky/not risky?]: Change is covered by new testcases, but the change involves some new code that makes it somewhat risky.
[String changes made/needed]: None

Comment 9

[Gerry Chang [:gchang]]

Track 54+ as regression.

Comment 10

[Gerry Chang [:gchang]]

Hi Mihai,
Can you help verify if this issue was fixed as expected on a latest Nightly build? Thanks!

Comment 11

[u549602]

Hi,

Verifying as fixed on the latest Nightly build (55.0a1 - 2017-05-08)

This issue was tested using the latest version of Gboard on a Samsung Galaxy S6 EDGE (Android 6.0), a Nexus 6P (Android 7.1.1) and a Samsung Galaxy Tab Active (Android 5.1.1)

Comment 12

[Gerry Chang [:gchang]]

Comment on attachment 8863033 [details] [diff] [review]
Patch for Beta

Fix a regression and was verified. Beta54+. Should be in 54 beta 7.

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/87551f39e8bff109c05160cc39c124dac81f54c4

Comment 14

[Carsten Book [:Tomcat]]

had to back this out for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=97899570&repo=mozilla-beta

Comment 15

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Patch for Beta (v2)

There's something weird going on with the selectionchange listeners, so I just removed them for this new Beta patch. The tests still test against this bug even without those listeners.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/63f0a11c6840

Comment 17

[Oana Horvath [:ohorvath]]

Verified as fixed, following the described STR, on Beta 54.0b8. 
Devices:
-Huawei Honor 5X (Android 5.1.1)
-HTC 10 (Android 6.0.1)

Comment 18

[Stefan Deiac]

Based on comment 17 I will remove qe-verify flag.