Closed Bug 901377 Opened 9 years ago Closed 9 years ago

Autocorrect doesn't update internal state on cursor movement, causing incorrect replacements to be made

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:leo+, b2g18 verified, b2g-v1.1hd fixed)

RESOLVED FIXED
blocking-b2g leo+
Tracking Status
b2g18 --- verified
b2g-v1.1hd --- fixed

People

(Reporter: janjongboom, Assigned: djf)

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

The problems are in all text boxes.

For normal textbox, type:
Jan dkk bla

Jump to the last 'k', type a 'g'
Jump to last character
Press space, 'bla' is replaced by 'dike' which makes no sense.

Appears both in contenteditable and normal textboxes. Possible cause might be bug 890207.
blocking-b2g: --- → leo?
Nominating for leo? because this is a regression that is a serious UX flow, causing text to be wrongly replaced.

Possible fix: backporting onselectionchange to b2g18
I can reproduce this on 1.1.  (Haven't tried master yet)

This is a very serious regression that basically breaks autocorrection.

Asking QA for help determining the regression range.

Who can give this Leo+?
Yuan,

Any idea what might be causing this? Do you want to take this bug? If not, please assign me.
Flags: needinfo?(xyuan)
Tested with latest master branch and the bug is caused by gaia commit 8ff99a4b7cc0ad47c310401467a96e94b2f63d99 of bug 874011.

When cursor moves, the latin IME will get the new cursor position by the callback function of `activate(language, suggestionsEnabled, inputstate)` in latin.js. Then the callback function calls `updateSuggestions()` to update suggestion word list.

With bug 874011 fixed, the callback function fails to call `updateSuggestions()` if the keyboard language hasn't been change since last call.
Assignee: nobody → xyuan
Flags: needinfo?(xyuan)
Triage - leo+'ing regression bug.
blocking-b2g: leo? → leo+
Attachment #785672 - Flags: review?(dflanagan)
Yuan,

Thank you for investigating this and patching it so quickly!

I'm embarassed to find that it is my regression! I assumed that something had changed in forms.js that was preventing activations from being sent to the keyboard. I didn't stop to investigate whether my code in latin.js was ignoring those activations! I apologize for being quick to assume that the bug was in your code!

I was just about to land your patch when I notice that activate() should probably also reset the correctionDiabled flag. If it is set and the user moves the cursor, we won't get autocorrection and the new location.

While that is not technically part of this regression, I'd like to fix it here.

(I'm also seeing something more ominous where after tapping around in my input a few times, the suggestion bar goes away entirely, and I'd like to investigate this some more, too.)

So I'm going to take this bug and ask you to review my patch when I have it ready.
Assignee: xyuan → dflanagan
Attached file link.html
Yuan,

I'm replacing your pull request with mine.  Mine includes the fix you found, but also fixes another regression: When the user moved the cursor we did not reset the 'correctionDisabled' flag, so if the user made an autocorrection, reverted it with a backspace, and then moved the cursor, we'd remain in the disabled state until they typed a space.

Also, your patch introduced another bug: when the language did not need to change, your patch calls updateSuggesetions() instead of setLanguage().  That fixes the bug reported here, but introduces a new problem because the idle timer never gets reset, so if the user dismisses the keyboard and then brings it up again, autocorrection stops working after about 30 seconds.

This patch fixes that problem by moving the clearTimeout() code into activate() (where it should have been all along) instead of setLanguage().

Thank you again for your quick diagnosis of this bug!
Attachment #785672 - Attachment is obsolete: true
Attachment #785672 - Flags: review?(dflanagan)
Attachment #785973 - Flags: review?(xyuan)
Comment on attachment 785973 [details]
link.html

Setting feedback? for Jan and Mihai because Jan reported the bug and Mihai and I both worked on the bug that caused this regression.

Jan and Mihai: no action is needed from you. I just wanted to let you know about this patch in case you are interested.
Attachment #785973 - Flags: feedback?(mihai)
Attachment #785973 - Flags: feedback?(janjongboom)
Comment on attachment 785973 [details]
link.html

The patch is great and r=me.
It's a pleasant to work with you.
Attachment #785973 - Flags: review?(xyuan) → review+
Landed to master: https://github.com/mozilla-b2g/gaia/commit/42c4efb7550820b7b6d6086d419a32a9e0cad174
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Comment on attachment 785973 [details]
link.html

Thanks for taking care of these regressions, David -- curious that we missed them while working on those keyboard features.
Attachment #785973 - Flags: feedback?(mihai) → feedback+
Yay \o/
Uplifted 42c4efb7550820b7b6d6086d419a32a9e0cad174 to:
v1-train: dc96979e15d0eb562002d00c46167fe48655a0e1
v1.1.0hd: dc96979e15d0eb562002d00c46167fe48655a0e1
Verified fixed on

Build ID: 20130814041202
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/15813d776a69
Gaia: dd3959fa74e356a528daa76ffee14c2c728a4b56
Platform Version: 18.1
RIL Version: 01.01.00.019.190 

Following steps on bug issue response correctly
Attachment #785973 - Flags: feedback?(janjongboom)
You need to log in before you can comment on or make changes to this bug.