Keyboard prediction stops working on Telegram (reproducible)

RESOLVED FIXED

Status

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mihaibn, Assigned: timdream)

Tracking

({foxfood})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [bzlite])

Attachments

(5 attachments)

(Reporter)

Description

3 years ago
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.
(Reporter)

Comment 1

3 years ago
I am using OTA 20151128030224 on a Flame phone.
(Reporter)

Updated

3 years ago
Component: Gaia::Feedback → Gaia::Keyboard

Comment 2

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

Updated

3 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 3

3 years ago
I am using a Z3 compact foxfooding device with the 20151019102225 build identifier.
(Reporter)

Comment 4

3 years ago
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.
(Reporter)

Comment 5

3 years ago
Created attachment 8693276 [details]
prediction stops working
STR in comment 4.
Flags: needinfo?(timdream)
Summary: Keyboard prediction stops working → Keyboard prediction stops working on Telegram (reproducible)
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.
And I totally reproduce the issue with the same STR. Tim you can find me if you want to debug on my phone.
(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)
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.
Assignee: nobody → timdream
Flags: needinfo?(timdream)
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).
Created attachment 8697574 [details]
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>.
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.
Component: Gaia::Keyboard → DOM: Device Interfaces
Product: Firefox OS → Core
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.
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.
(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.
This bug require fix in both interface and the app.
Component: DOM: Device Interfaces → Gaia::Keyboard
Product: Core → Firefox OS
Depends on: 1234459
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.
(Reporter)

Updated

3 years ago
Duplicate of this bug: 1233320
Created attachment 8704476 [details] [review]
[gaia] timdream:keyboard-text-update > mozilla-b2g:master
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!
Attachment #8704476 - Flags: review?(ralin)

Comment 23

3 years ago
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 :)
Attachment #8704476 - Flags: review?(ralin) → review+
master: https://github.com/mozilla-b2g/gaia/commit/4cf692f3cfce65f6dd7bda6b18d22d027376241e
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
\o/ Thx !
On a build containing that patch I have spotted very nasty usability regressions with the text prediction feature
Depends on: 1237332
Created attachment 8704939 [details] [review]
[gaia] mozilla-b2g:revert-33760-keyboard-text-update > mozilla-b2g:master
master revert: https://github.com/mozilla-b2g/gaia/commit/676237f80cf72182500356fabc49365d3471c0e6
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 29

3 years ago
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.
Created attachment 8704986 [details] [review]
[gaia] timdream:keyboard-test-update2 > mozilla-b2g:master
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?
Attachment #8704986 - Flags: review?(ralin)

Comment 32

3 years ago
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.
Attachment #8704986 - Flags: review?(ralin) → review+
master: https://github.com/mozilla-b2g/gaia/commit/c5f9f07b6521cb1605d6160733f22ebce5aea2c8
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Resolution: --- → FIXED
Duplicate of this bug: 1090883
You need to log in before you can comment on or make changes to this bug.