1015541 - choosing a word suggestion after moving the caret changes the wrong text

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Keyboard, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(Note that I frequently hit this bug because of bug 1015539, but it's a separate problem and probably much more easily fixable.  See bug 1015539 for why I hit this case so often, though.)

When I choose a suggested spelling from the suggestion bar above the keyboard, I sometimes accidentally tap above the keyboard first, moving the caret (insertion point).  If I then tap on the suggestion, the keyboard app replaces the wrong text.

Steps to reproduce:
 1. In Settings App -> Keyboards -> Built-in Keyboard, make sure both "Auto correction" and "Word suggestion" are selected
 2. start Email app, and click the compose icon
 3. in the message body, type "This is some tect"
 4. Tap in the text in the message body, placing the caret at
    "This is som|e tect"
 5. Tap on the suggestion "text"

Actual results:
  "This istext |e tect" (where | is the caret)

Expected results:
  "This is some text |" (where | is the caret)

It seems like the code is assuming that the caret is at the same place it was when the suggestions were made, and an easy fix ought (I think) to be to place the caret where it was when the suggestions were made before making the replacement (and perhaps checking that the text being replaced is what was expected).

Comment 1

[Jan Jongboom [:janjongboom] (inactive)]

Another state inconsistency. Can we use this to up the need of doing proper state syncing between Gecko and Gaia?

There are events for this stuff (text before cursor changed) already.

Comment 2

[Tim Guan-tin Chien [:timdream] (please needinfo)]

This issue in particular is not a state inconsistency issue: we simply did not void the suggestions on Step 4. We should clean up the candidates upon getting caret changed event.

However, You are partly right: even if we do fix the thing above, we will still ended up suspectable to race if the user move the caret and the selection at almost the same time. We simply cannot predict the events in another process in this case. I would imagine a marionette test can easily reproduce that even on the fastest phones.

For the rest of the state inconsistency issues, as I already said in the e-mail thread, I think we should wrap the inputContext object in a module in the keyboard and queue all requests. Tell me what's needed to make it happen?

Comment 3

[Jan Jongboom [:janjongboom] (inactive)]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #2)
> For the rest of the state inconsistency issues, as I already said in the
> e-mail thread, I think we should wrap the inputContext object in a module in
> the keyboard and queue all requests. Tell me what's needed to make it happen?

Why should we queue the requests? I think the way the message handling in Gecko works it's already guaranteed that messages get there in the right order. And if not, third party keyboards will also suffer from it and we should fix in Gecko.

Comment 4

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Really sorry for the delay. Will respond today or tomorrow.

Comment 5

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Jan Jongboom [:janjongboom] (Telenor) from comment #3)
> (In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from
> comment #2)
> > For the rest of the state inconsistency issues, as I already said in the
> > e-mail thread, I think we should wrap the inputContext object in a module in
> > the keyboard and queue all requests. Tell me what's needed to make it happen?
> 
> Why should we queue the requests? I think the way the message handling in
> Gecko works it's already guaranteed that messages get there in the right
> order. And if not, third party keyboards will also suffer from it and we
> should fix in Gecko.

First, what we are talking about is more related to something like bug 1013570 instead of this bug. I am not sure we should keep use this bug as a forum but I will reply anyway.

So for issues like bug 1013570, we are facing an issue where we made a decision on what to do with the second tap of the space key before the previous one resolves. Since the first key ended up changing the states keeping in the keyboard app (in the case of that bug, e.g., the upper case state). The decisions we made for the second tap (e.g. |if (isUpperCase) ...|) ended up being wrong. The temporary solution of that bug was to implement a promise based queue in latin.js (the supposedly plugable IME script). I want to move that to keyboard app itself so every IME script will not have to do this again. I will do that maybe in one of the dependent bug of bug 1023729 (to be filed) when I move all the touch event processing to another module.

The platform cannot absorb this behavior for us. This is purely the implementation detail of keyboard app itself, because, for example, for some really simple app like LOL Keyboard (gaia/dev_apps/test-keyboard-app), keeping an internal state is a non-issue because there is no state to keep to begin with. Besides, I cannot think of any other way for platform to address this issue without making ALL inputContext methods synchronous, i.e. not return until the it does what it do.

Figuring these out will be a critical step before we move on to bug 929365. If we can't process the touch screen taps right, processing hardware key events will be a very fun ride.

Comment 6

[Jan Jongboom [:janjongboom] (inactive)]

Alright, sounds reasonable. Thanks for explaining.

Comment 8

[Julien Wajsberg [:julienw]]

Duplicate bug 1025675 is 1.4?. See bug 1025675 comment 4 for a list of version this reproduces/doesn't reproduce.

Comment 9

[Preeti Raghunath(:Preeti)]

Blocking on it for 1.4 because it has bad user experience and its regression

Comment 10

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Rudy, here is an unfortunate late 1.4+ bug.

I am very interested to know what is the regressed bug between 1.3/1.4.

Comment 11

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

I think the root cause is that on v1.4+, activate() in latin IME engine won't be invoked after we changed the cursor position.
Going to find out which revision changed the behavior.

Comment 12

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

After investigation, I suspect this might be a Gecko issue.
For v1.3,
  1. We would get inputcontextchange event when we changed the cursor position, which would invoke showKeyboard() and then to activate() function of latin IME.

However, for Gecko starting from v1.4+, it would trigger inputcontextchange event when we changed the cursor position.


--
Jan, Yuan,

Do you have any clue if we've done any changes around inputcontextchange event after v1.4?
Thanks.

Comment 13

[Jan Jongboom [:janjongboom] (inactive)]

This is a bug caused by bug 944681 I think then.

I think Gecko behavior is right.

Comment 14

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

(In reply to Rudy Lu [:rudyl] from comment #12)
> After investigation, I suspect this might be a Gecko issue.
> For v1.3,
>   1. We would get inputcontextchange event when we changed the cursor
> position, which would invoke showKeyboard() and then to activate() function
> of latin IME.
> 
> However, for Gecko starting from v1.4+, it would trigger inputcontextchange
                                                  ^ NOT
> event when we changed the cursor position.
> 

Jan, sorry for the misleading info here.

Comment 15

[Yuan Xulei [:yxl] (Leave from May 18)]

According to the design, inputcontextchange event should be fired when we focus a new input field or lose focus. We should not fire inputcontextchange when cursor changes.

However, in order to be compatible with the old mozKeyboard API, we also fire inputcontextchange event for v1.3. We fixed this issue with bug 944681 as Jan pointed out.

Rudy, I think we need a fix for this.

Comment 16

[Jan Jongboom [:janjongboom] (inactive)]

Yeah Rudy agreed to take this.

Comment 17

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Attachment: Patch V1

This is harder than I thought to handle this case.
Originally, I was trying to handle this in keyboard.js and listen to selectionchange event and invoke input engine's activate(), however, this would be not necessary for IM engines other than latin, and even worse, this could break jspinyin.

So, I changed latin Engine to make it listen to selectionchange.
This WIP could solve this issue, however, there is still an issue to be resolved that after the auto-correction is done, it would also trigger selectionchange event and we would do the updateSuggestions() again to cause the suggestion bar flicker and sometimes it would cause state inconsistency if we update the "cursor" value in selectionchange event handler.

Comment 18

[Jan Jongboom [:janjongboom] (inactive)]

In Android Selection handling logic it 'disables' selectionchange listening whenever a change will be made by local code (flag). Maybe we can do that.

Comment 19

[Yuan Xulei [:yxl] (Leave from May 18)]

(In reply to Rudy Lu [:rudyl] from comment #17)
> Created attachment 8442779 [details] [review]
> WIP
> 
> This is harder than I thought to handle this case.
> Originally, I was trying to handle this in keyboard.js and listen to
> selectionchange event and invoke input engine's activate(), however, this
> would be not necessary for IM engines other than latin, and even worse, this
> could break jspinyin.
FYI. jspinyin doesn't need cursor infomation :-)

Comment 20

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

(In reply to Jan Jongboom [:janjongboom] (Telenor) from comment #18)
> In Android Selection handling logic it 'disables' selectionchange listening
> whenever a change will be made by local code (flag). Maybe we can do that.

Yeah, thanks. I was thinking about doing this as you said.
WIP updated and will need to see if could pass CI and more manual tests.

> > could break jspinyin.
> FYI. jspinyin doesn't need cursor infomation :-)

Yes, that is why I tried to patch latin IME itself but not the jspinyin part.
Thank you.

Comment 21

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Comment on attachment 8442779 [details] [review]
Patch V1

I think this is ready to review.

[Solution]
1. Make latin IME listen to "selectionchange" event and update the suggestions if the cursor is changed.

2. Only update the suggestions when the cursor is "really" changed and there is no pending change that is going to happen with "sendKey()" or "replaceSurroundingText()", or we might see the suggestion bar more than one time when the user selects one of the suggestions.

Jan,

Could you please help review this change?
Thank you.

Comment 22

[Jan Jongboom [:janjongboom] (inactive)]

Potential issue is that selectionchange doesn't seem to work on mc, as per bug 1001325. 1.4 is fine.

Comment 23

[Jan Jongboom [:janjongboom] (inactive)]

Comment on attachment 8442779 [details] [review]
Patch V1

LGTM, thanks for fixing this annoying thing. One question on GH.

Comment 24

[Jan Jongboom [:janjongboom] (inactive)]

Tested on 1.4 btw

Comment 26

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Just re-send the pull request to include a change to make Gaia UI Test pass on Travis, i.e. Bug 1028906.
Will land to master after Travis is green.

Comment 27

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Landed to Gaia master,
https://github.com/mozilla-b2g/gaia/commit/73b9c780c54ec1c6fcf47c2ce467672d3c5aadedv

Comment 28

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

(In reply to Rudy Lu [:rudyl] from comment #27)
> Landed to Gaia master,
> https://github.com/mozilla-b2g/gaia/commit/
> 73b9c780c54ec1c6fcf47c2ce467672d3c5aadedv

The commit is:
https://github.com/mozilla-b2g/gaia/commit/73b9c780c54ec1c6fcf47c2ce467672d3c5aaded

Comment 29

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

This would need a v1.4 branch-specific patch, so please don't uplift to v1.4.

Comment 30

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Uplifted to v2.0,
74bab0ee1327236576d584d53a3ec52a35d061a4

--
The gaia ui test failure is irrelevant,
https://travis-ci.org/mozilla-b2g/gaia/builds/28380242
The Gi failure is also irrelevant here,
https://tbpl.mozilla.org/?tree=Gaia-Try&rev=9cd157aad33e49e4d73e5ecb3a7092581ae56a24

Comment 31

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Gaia v1.4,
https://github.com/mozilla-b2g/gaia/commit/934331eff4fb2922208e6ac2eb47ce6d1db62a22

--
The Travis all passed except for one irrelevant failure, which I re-triggered the jobs several times but could not get rid of,
https://travis-ci.org/mozilla-b2g/gaia/builds/28474705

Comment 32

[Coler]

This issue has been verified successfully on Flame2.0&2.1.
STRs： 
 1. "Auto correction" and "Word suggestion" are selected in Settings.
 2. start Email app, and click the compose icon
 3. in the message body, type "This is some tect"
 4. Tap in the text in the message body, placing the caret at "This is som|e tect"

Actual Result:
 4. The suggestion word will disappear when the caret at "som|e".

Reproducing rate: 0/5
See attachment: Verify_Flame_caret.mp4

Flame 2.0 build version:
Gaia-Rev        8d1e868864c8a8f1e037685f0656d1da70d08c06
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/c756bd8bf3c3
Build-ID        20141127000203
Version         32.0

Flame2.1 build version:
Gaia-Rev        5372b675e018b6aac97d95ff5db8d4bd16addb9b
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/f34377ae402b
Build-ID        20141127001201
Version         34.0

Comment 33

[Coler]

Attachment: Verify_Flame_caret.MP4