882866 - When correcting a word, previous letters and words seem to join on to the front of the word

Status: RESOLVED FIXED

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

Description

[Francis Djabri [:djabber]]

I've noticed when pressing backspace to go back and correct a mis-spelt a word, occasionally the auto-correct feature joins on previous letters and words to the word being corrected, even though they're separated by a space. This leads to auto-correct suggestions such as the following:

tha -> Ethan rtha ethane
im -> immune imm Inman
second -> andsecond
device -> devidvice

Comment 1

[David Flanagan [:djf]]

This might be a symptom of bug 882197. Can you still reproduce it on master?

Comment 2

[Francis Djabri [:djabber]]

I can reproduce it on master. This generally seems to happen when typing in text very fast. I think the issue might in fact be that characters that have been entered in are being somehow missed by the auto-correct feature. See the following attachments.

Comment 3

[Francis Djabri [:djabber]]

Attachment: Missing letters

Comment 4

[Francis Djabri [:djabber]]

Attachment: Missing letters

Comment 5

[Francis Djabri [:djabber]]

Attachment: Missing letters

Comment 6

[Francis Djabri [:djabber]]

Attachment: Missing letters

Comment 7

[David Flanagan [:djf]]

Thanks for these screenshots, Francis. 

I think I can reproduce what you're seeing (or something like it) with these steps:

1) Type "testin" and see that auto-correct is going to replace it with "testing".

2) Type two spaces very quickly. We get an autocorrect and a period.  But if I now backspace twice, and type space again (to autocorrect again) I get 'ttesting". This indicates that the latin im is confused about the current state of the input.  (I can reproduce this in both the MMS app that uses content editable and in the UI tests that use text area).

I noticed the other day that with the switch to using replaceSurroundingText(), the keyboard code in gecko seems to be re-activating the keyboard code in gaia after each word replacement. This happens asynchronously, and if the user is typing quickly, it might be possible for the gaia representation of input field content to get out of sync with the gecko reality, and this would cause auto-correct errors like the ones here seeing.

Yuan: do you know anything about this? Does calling replaceSurroundingText() end up re-activiting the keyboard somehow the way we would if the user tapped to move the cursor?

This seems bad, so I'm nominating it for Leo. I'm assigning the bug to myself so I don't forget about it. Yuan and Rudy: if either of you want to take this, please feel free.

Comment 8

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

(In reply to David Flanagan [:djf] from comment #7)
> Yuan: do you know anything about this? Does calling replaceSurroundingText()
> end up re-activiting the keyboard somehow the way we would if the user
> tapped to move the cursor?

Yes, you are right. mozKeyboard.replaceSurroundingText will sometime reactivate the keyboard. 

we added an observer to monitor all edit actions of the current input element. If the text of the element is changed by external actions, such as using JS to clear the content, we will get notification to make keyboard reactivate. When mozKeyboard.replaceSurroundingText is called to replace text, two edit actions will be observed: removal of the old text and insertion of the new text. The first action is ignored, while the second is treated as external actions mistakenly. 

I'll make a patch to resolve the issue of mozKeyboard.replaceSurroundingText, but I'm sure it could fix this bug.

Comment 9

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

Attachment: Ignore EditAction notification caused by mozKeyboard.replaceSurroundingText. r=fabrice

The patch ignored all edit actions caused by mozKeyboard to avoid reactivating the keyboard mistakenly.

Comment 10

[David Flanagan [:djf]]

Comment on attachment 764744 [details] [diff] [review]
Ignore EditAction notification caused by mozKeyboard.replaceSurroundingText. r=fabrice

Review of attachment 764744 [details] [diff] [review]:
-----------------------------------------------------------------

Fabrice is on vaction, I think, so I'm stealing this review.

The patch is straightforward. The code is clean. I've tested it and it fixes the bug described in comment 7.

I've never been able to type quickly enough to reproduce the exact symptoms that the orignal reporter described. But this patch does fix a major bug, so we should land it ASAP and ask the reporter to verify.

::: b2g/chrome/content/forms.js
@@ +275,2 @@
>        return;
>      }

Even with this code, we'll still have a race condition if the user is typing at the same time that a script modifies the text field content. In that case, the Latin IM may get confused and do bad auto-corrections.  

I can't think of any way to fix this with our current architecture, and it seems quite unlikely in practice.

@@ +376,5 @@
>          }.bind(this), 0);
>          break;
>  
>        case "keyup":
> +        this._ignoreEditActionOnce = false;

If a keyup event arrives before the edit action occurs, then we won't ignore the edit action.

But maybe this cannot ever occur. If it can, it is a different race condition than the one this patch is fixing.

Comment 11

[David Flanagan [:djf]]

Setting checkin-needed, and re-assigning to Yuan, since he's the one who wrote the patch.

Comment 12

[David Flanagan [:djf]]

This is a b2g patch, so checkin should be to the birch branch, of course.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/projects/birch/rev/f2deb05a7ba5

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f2deb05a7ba5

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/c1f696b4b80d

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/c1f696b4b80d

Comment 17

[leo.bugzilla.gecko]

can you upload full source about b2g/chrome/content/forms.js
??

Comment 18

[Jeni]

Verified fixed on Leo device
Build ID: 20130807071207
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/11bb1b0eefff
Gaia: 60ca81600a080dae33058b0692ecaa213556c926
Platform Version: 18.1

Auto correct is now functioning efficiently