1228778 - Keyboard prediction stops working on Telegram (reproducible)

Status: RESOLVED FIXED

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

Description

[Mihai Barbat]

User-Agent: Mozilla/5.0 (Mobile; rv:45.0) Gecko/45.0 Firefox/45.0

I am using Telegram app and I keep noticing how the keyboard prediction just stops working. I have two keyboards installed. Romanian and English. When the English prediction stops working I need to press on the button that switches the languages and then the prediction restarts.

Comment 1

[Mihai Barbat]

I am using OTA 20151128030224 on a Flame phone.

Comment 2

[Sayak Sarkar]

I can confirm this bug. At times the predictions stop working after the first time the keyboardis used.

Comment 3

[Sayak Sarkar]

I am using a Z3 compact foxfooding device with the 20151019102225 build identifier.

Comment 4

[Mihai Barbat]

You can always reproduce this with Telegram. The prediction only works once until you send a message. Then you need to press the power button to close the screen and go back to Telegram to get it working. Or you can also switch the language of your keyboard. Both of this actions bring back the prediction.

Comment 5

[Mihai Barbat]

Attachment: prediction stops working

Comment 6

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

STR in comment 4.

Comment 7

[Julien Wajsberg [:julienw]]

I don't see much logs, I see this:

12-08 10:27:32.247  1571  1571 W Built-in Keyboard: Content JS WARN: InputMethodGlue: call sendCandidates() when inputContext does not exist. 
12-08 10:27:32.247  1571  1571 W Built-in Keyboard:     at InputMethodGlue.prototype.sendCandidates (app://keyboard.gaiamobile.org/gaia_build_defer_index.js:247:762)

But not sure this is related as I see it in other situations as well.

Comment 8

[Julien Wajsberg [:julienw]]

And I totally reproduce the issue with the same STR. Tim you can find me if you want to debug on my phone.

Comment 9

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

(In reply to Julien Wajsberg [:julienw] from comment #8)
> And I totally reproduce the issue with the same STR. Tim you can find me if
> you want to debug on my phone.

Thanks! I am currently blocked by other bugs so if we want to fix this quick I will be very happy if someone would like to try. I suspect prediction.js is blocked in some state preventing it to output more predictions. The easy way to debug this is to put breakpoint on latin.js to inspect it's variables.

(Unfortuately bug 1003097 is not fixed yet so we cannot see the scope of the worker and inspect prediction.js and worker.js)

Comment 10

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

I can produce this on my Flame. Looks like a timing issue b/t Telegram and Keyboard app, unsurprisingly, since it went away as soon as I put a breakpoint in latin.js.

Would need to console.log() everything to find the real offending behavior.

Comment 11

[Julien Wajsberg [:julienw]]

My initial debugging shows that inputText is not properly updated after sending a message.

As a result the prediction logic thinks the cursor is in the middle of a word and does not offer any suggestion (atWordEnd() returns false).

Comment 12

[Julien Wajsberg [:julienw]]

Attachment: testcase-focus.html

I managed to produce a testcase here.

So what happens is that a contenteditable content is wiped without the focus being removed.

Note I couldn't reproduce with a <input>.

Comment 13

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

Nice! Thank you very much. It's a Input Method API problem then since keyboard app expects programmatic contenteditable changes triggers the same set of events like <input>s.

Comment 14

[Julien Wajsberg [:julienw]]

Note 2: with a <input> activate() get properly called to reset inputText. But not with contenteditable.

Could be interesting to check what happens with contenteditable when we change textContent to any other text too.

Comment 15

[Julien Wajsberg [:julienw]]

I did a little more debugging.

* when clearing the content with the input, we get an "inputcontextchange" event. This comes from [1]. When receiving this event, the keyboard app gets the new value in "activate".
* when clearing the content with a contenteditable, we only get a "selectionchange" event. This comes from [2].

[1] https://dxr.mozilla.org/mozilla-central/source/dom/inputmethod/forms.js#573
[2] https://dxr.mozilla.org/mozilla-central/source/dom/inputmethod/forms.js#541

Now what could happen is that when we have a contenteditable we either fail to get the editor in [3][4] or the editor does not call EditAction properly.

[3] https://dxr.mozilla.org/mozilla-central/source/dom/inputmethod/forms.js#504
[4] https://dxr.mozilla.org/mozilla-central/source/dom/inputmethod/forms.js#1422-1430

I could not debug more as I can't seem to be able to put breakpoints in forms.js with WebIDE so I guess I (or someone) will need to put more logs around these bits and flash the new file.

I won't be able to do this soon so Tim if you have some bandwidth feel free to continue the investigation.

Comment 16

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

(In reply to Julien Wajsberg [:julienw] (PTO -> 2016) from comment #15)
> I did a little more debugging.
> 
> * when clearing the content with the input, we get an "inputcontextchange"
> event. This comes from [1]. When receiving this event, the keyboard app gets
> the new value in "activate".
> * when clearing the content with a contenteditable, we only get a
> "selectionchange" event. This comes from [2].

Will be on working on this soon. Interesting problem here, since keyboard should also reset it's state when hearing standalone selectionchange event.

We should probably unify this behavior assuming there is no bug in the keyboard being hidden in the <input> case.

Comment 17

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

This bug require fix in both interface and the app.

Comment 18

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

https://github.com/timdream/gaia/tree/keyboard-text-update

Comment 19

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

Note to myself: This bug is blocked on bug 1234459 because we would need to reset |inputText| in latin.js too, when Telegram resets the contenteditable; without bug 1234459, it would be almost impossible to construct |inputText| from textBeforeCursor and textAfterCursor.

Comment 21

[GitHub Autolander Bot]

Attachment: [gaia] timdream:keyboard-text-update > mozilla-b2g:master

Comment 22

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

Comment on attachment 8704476 [details] [review]
[gaia] timdream:keyboard-text-update > mozilla-b2g:master

Ray,

Could you review this? You can read the bug and the comment to tell what the change is for.

Since the text value is available under MozInputContext#text, I've remove a detour in StateManager that would call MozInputContext#getText().

Thanks!

Comment 23

[Ray Lin[:ralin]]

Comment on attachment 8704476 [details] [review]
[gaia] timdream:keyboard-text-update > mozilla-b2g:master

Thanks!!! It looks good!

We expect inputcontextchange will triggered after sending text, but what we actually get is selectionchange.  

Therefore, we do need to update inputText as well to ensure inputText corresponds to content in this case, then the prediction function can work against right information. 

It is also great to see the inputContext can be updated in synchronous way. 
Thanks :)

Comment 24

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

master: https://github.com/mozilla-b2g/gaia/commit/4cf692f3cfce65f6dd7bda6b18d22d027376241e

Comment 25

[Julien Wajsberg [:julienw]]

\o/ Thx !

Comment 26

[:gerard-majax]

On a build containing that patch I have spotted very nasty usability regressions with the text prediction feature

Comment 27

[GitHub Autolander Bot]

Attachment: [gaia] mozilla-b2g:revert-33760-keyboard-text-update > mozilla-b2g:master

Comment 28

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

master revert: https://github.com/mozilla-b2g/gaia/commit/676237f80cf72182500356fabc49365d3471c0e6

Comment 29

[Ray Lin[:ralin]]

Hi, Tim

Sorry I didn't spot that problem. Since both handleKey() and stateChange() update inputText at different timing, it might make inputText unpredictable.

My debugging: 
1. Typing 'h' -> stateChange() update inputText to 'h' -> handleKey() update inputText to 'hh'
2. Typing 'e' -> stateChange() update inputText to 'he' -> handleKey() update inputText to 'hee'
That is to say, when suggestion is picked, the length of oldWord would be longer than real oldWord by 1.

Example:
Steps: Type 'he he' and pick suggestion 'her'
Expected: he her
Actual:   heher
In this case, what replaceBeforeCursor() tend to replace is not 'he' but 'hee'(length = 3), so ' he' will be replaced with 'her'. 

Hope it helps and sorry for my careless mistake.

Comment 30

[GitHub Autolander Bot]

Attachment: [gaia] timdream:keyboard-test-update2 > mozilla-b2g:master

Comment 31

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

Comment on attachment 8704986 [details] [review]
[gaia] timdream:keyboard-test-update2 > mozilla-b2g:master

It's mine own mistake. I was trying to remove |ownAction| from latin.js but I didn't realize the expected inputText will not change until the sendKey() promise call resolves.

Given the fact we are not removing it from the main keyboard code anyway (because of JSPinyin) I simply add it back and we could do it in another bug.

Strangely, this should have been caught by Gij test I think? Does it make sense to create more tests here?

Comment 32

[Ray Lin[:ralin]]

Comment on attachment 8704986 [details] [review]
[gaia] timdream:keyboard-test-update2 > mozilla-b2g:master

Thanks for doing these works, I agree we could do this in another bug

Gij should catch the regression, but just temporarily be disabled. I'm trying to bring them back.

Comment 33

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

master: https://github.com/mozilla-b2g/gaia/commit/c5f9f07b6521cb1605d6160733f22ebce5aea2c8