1073870 - [Keyboard][Autocorrect] Still wrong autocorrect in v2.0

Status: VERIFIED FIXED

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

Description

[Julien Wajsberg [:julienw]]

We fixed an issue in the past (bug 1015541) but a similar issue is still occurring in v2.0.

Here is a STR:
1. Open Messages
2. write "Hello worl"
3. put the caret after "Hello"
4. press "space"

Expected:
* a space is inserted

Actual:
* "Hello" is replaced by "work"

So it looks like bug 1015541 reappeared.

I get this very often (we often want to correct things in a written message) so I think we can't ship with this.

QA: please try this in various branches (including v1.4). If it's a regression please find a window :) Thanks!


Build informations:

Firefox OS v2.0 on Flame
BuildId: 20140927000202
Gaia hash: 5c2303ec

Comment 1

[Duane Dixon [:ddixon]]

Branch Check

Issue DOES occur in Flame 2.2, 2.1, 2.0, 2.0 Base Image. 

Device: Flame Master
BuildID: 20140929070359
Gaia: 4d663b1f7d63e4d3d69c181a58f21b38145044b2
Gecko: 66ab05e2a667
Version: 35.0a1 (Master) 
Firmware Version: v180
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0
------------------------------------------------------
Device: Flame 2.1
Build ID: 20140929064901
Gaia: 063de64a4ffc606e931ed7b09e93282713c46eca
Gecko: ba4f8bb18ef9
Version: 34.0a2 
Firmware Version: v180
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0
------------------------------------------------------
Device: Flame 2.0
Build ID: 20140929063300
Gaia: 5c2303ec4e367da060aa1b807d541a6549b3d72a
Gecko: d7b2e9cc93f8
Version: 32.0 (2.0)
Firmware Version: v180
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0
------------------------------------------------------
Device: Flame 2.0 (Base Image)
Build ID: 20140904160718
Gaia: 506da297098326c671523707caae6eaba7e718da
Gecko: 2b27becae85092d46bfadcd4fb5605e82e1e1093
Version: 32.0 (2.0)
Firmware Version: v180
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Comment 2

[Joshua Mitchell (Inactive)]

Not a regression - always occurs in the KK-base life-cycle

Comment 3

[Julien Wajsberg [:julienw]]

Please also test in v1.4, and possibly 1.3, as I requested in comment 0.

We're supposed to have fixed this in bug 1015541 on v1.4, and it was recognized as a regression from v1.3.

Comment 4

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

I could reproduce this issue with current master,
but could not repro bug 1015541 with the same revision, so this should be a different case from bug 1015541, 

===
Gaia-Rev        77ef35f5429bc3dfe9ca192b9aacc3c0bf8857de
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/7c24470b6b3a
Build-ID        20140929160203
Version         35.0a1
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  27
FW-Date         Thu Sep  4 14:59:02 CST 2014
Bootloader      L1TC10011800

Comment 5

[howie [:howie]]

Triage: blocking. Hi Rudy, can you take a look? thanks.

Comment 6

[Jan Jongboom [:janjongboom] (inactive)]

Stealing it.

Comment 7

[Jan Jongboom [:janjongboom] (inactive)]

Attachment: Patch

Still wondering where this comes from, especially because I haven't seen it myself on 2.0 (but I could reproduce of course). Maybe I never execute this particular sequence.

Anyway: patch + UI test.

Comment 8

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

Comment on attachment 8497566 [details] [review]
Patch

I looked at the patch and also tested this, it seem working fine.
However, I don't think I am familiar with this part of code that much.

David would be a better reviewer for this.

David, could you please help us to review this patch?
Thank you.

Comment 9

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

Rudy, Looks like bug 1015541 only make the latin.js update the suggestion but not the auto correct when space is pressed?

Comment 10

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

Yeah, this was my miss that did not test this case, the patch in bug 1015541 would make latin.js invoke updateSuggestions() when it receives selectionchange, assuming updateSuggestions() would set the autoCorrection variable correctly.

Comment 11

[David Flanagan [:djf]]

Comment on attachment 8497566 [details] [review]
Patch

I'm concerned that this patch does not clear autoCorrection quickly enough. I think this waits to clear autoCorrection until the worker has returned a new batch of corrections.  If the user moves the cursor and types space very very quickly, won't we still get the incorrect correction?  Also, this patch does not clear autoCorrection when the user moves the cursor to the middle of a word. (The bug doesn't manifest in that case, but it still seems like we should clear it.)

Rudy's comment #10 suggests that the place to fix this is in the selectionChange() method at the end of latin.js. Setting autoCorrection to null there would probably work.  Or calling dismissSuggestions might be cleaner.

Rudy: if Jan changed his patch to modify dismissSuggestions, is that something you'd be comfortable reviewing? (As Jan knows too well, I have trouble keeping up with my review queue!)

Comment 12

[David Flanagan [:djf]]

I'll add that the patch does seem to resolve the bug (though perhaps not completely) and it seems very safe, so if we just need to land something for 1.4 or 2.0 ASAP, I'd be okay landing Jan's patch as it stands. My r- is because I think we can create a better patch pretty easily. If I'm wrong, let's consider landing this as it is.

Comment 13

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

(In reply to David Flanagan [:djf] from comment #11)
> I'm concerned that this patch does not clear autoCorrection quickly enough.
> I think this waits to clear autoCorrection until the worker has returned a
> new batch of corrections.  If the user moves the cursor and types space very
> very quickly, won't we still get the incorrect correction?  Also, this patch
> does not clear autoCorrection when the user moves the cursor to the middle
> of a word. (The bug doesn't manifest in that case, but it still seems like
> we should clear it.)

This is probably related to bug 882863 ... and I don't think we should address it here since the user will experience bug 882863 anyway.

> Rudy's comment #10 suggests that the place to fix this is in the
> selectionChange() method at the end of latin.js. Setting autoCorrection to
> null there would probably work.  Or calling dismissSuggestions might be
> cleaner.
> 
> Rudy: if Jan changed his patch to modify dismissSuggestions, is that
> something you'd be comfortable reviewing? (As Jan knows too well, I have
> trouble keeping up with my review queue!)

I can help if Rudy is not available, but I don't think I am more comfortable on reviewing latin.js than Rudy.

Comment 14

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

(In reply to David Flanagan [:djf] from comment #11)
> Comment on attachment 8497566 [details] [review]
> Patch
> 
> I'm concerned that this patch does not clear autoCorrection quickly enough.
> I think this waits to clear autoCorrection until the worker has returned a
> new batch of corrections.  If the user moves the cursor and types space very
> very quickly, won't we still get the incorrect correction?  Also, this patch
> does not clear autoCorrection when the user moves the cursor to the middle
> of a word. (The bug doesn't manifest in that case, but it still seems like
> we should clear it.)

I think this is a valid concern that we should take care of.
Trying this on a much slower device, like buri, and it would occur just as David mentioned.
We have sleep(1) in the automated test, guess it was added for the same reason?

> 
> Rudy's comment #10 suggests that the place to fix this is in the
> selectionChange() method at the end of latin.js. Setting autoCorrection to
> null there would probably work.  Or calling dismissSuggestions might be
> cleaner.
> 
> Rudy: if Jan changed his patch to modify dismissSuggestions, is that
> something you'd be comfortable reviewing? (As Jan knows too well, I have
> trouble keeping up with my review queue!)

Yeah, sure, I'll try my best to review it.
David, thanks for your help to shed some light on this while you are already occupied.

Comment 15

[Jan Jongboom [:janjongboom] (inactive)]

Attachment: Patch v2

Updated the patch based on the comments from djf. Also got rid of the sleep(1) in the test because of that. :-)

Comment 16

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

Comment on attachment 8500313 [details] [review]
Patch v2

Looks good to me.
Thanks.

Comment 17

[Jan Jongboom [:janjongboom] (inactive)]

Ah fuck, landed it without finishing Try https://github.com/mozilla-b2g/gaia/commit/94809111055fdf890513a37a8cfa21e06bcc2253

Reverted: https://github.com/mozilla-b2g/gaia/commit/f6d4e5323bfed5c1eb5ecc1ea6c4ea66c787c052

Comment 18

[Jan Jongboom [:janjongboom] (inactive)]

Attachment: Patch v3

New PR, let's see what Try thinks. https://tbpl.mozilla.org/?rev=08b06c842cd5e043952039ccb1e50330a43b5429&tree=Gaia-Try

Comment 19

[Jan Jongboom [:janjongboom] (inactive)]

Weird Try run just now, new try: https://tbpl.mozilla.org/?rev=667a032b46df99581a0465b1b5791758d14b853c&tree=Gaia-Try

Comment 20

[Jan Jongboom [:janjongboom] (inactive)]

Try is green @ https://tbpl.mozilla.org/?rev=f3c98ab8200648e0d7acb4cbe171e840477c8204&tree=Gaia-Try

Landed https://github.com/mozilla-b2g/gaia/commit/cbaac7447d79620fd4aa1e14c802ff83d562ceb2

Comment 21

[Julien Wajsberg [:julienw]]

Hey Jan, please ask approval for the branches when you have time :) Thanks !

Comment 22

[Jan Jongboom [:janjongboom] (inactive)]

Comment on attachment 8500345 [details] [review]
Patch v3

Already is marked 2.0+

Comment 23

[[:fabrice] Fabrice Desré]

(In reply to Jan Jongboom [:janjongboom] (Telenor) from comment #22)
> Comment on attachment 8500345 [details] [review]
> Patch v3
> 
> Already is marked 2.0+

Please answer the approval form questions...

Comment 24

[Jan Jongboom [:janjongboom] (inactive)]

Comment on attachment 8500345 [details] [review]
Patch v3

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Not clear. The bug has been present for quite some time, but never showed because our selection handling was broken prior to 2.0.
User impact if declined: For the user it seems as if there is random replacements of words, user will consider autocorrect as broken.
Testing completed: Landed on master, haven't seen any follow up bugs regarding this behavior.
Risk to taking this patch (and alternatives if risky): Low. We didn't clear state when moving around the cursor.
String or UUID changes made by this patch: None

Comment 25

[Ryan VanderMeulen [:RyanVM]]

v2.1: https://github.com/mozilla-b2g/gaia/commit/3d437a0b60f6305181b79f6c0d4c5e26cc3c5ccc

Comment 26

[Kai-Zhen Li [:seinlin][:kli]]

Jan, Patch v3 can't be merged into v2.0 or v2.0m. Could you provide a patch for 2.0 branch? Thanks!

--
Changes to be committed:

	new file:   tests/python/gaia-ui-tests/gaiatest/tests/functional/keyboard/test_keyboard_bug_1073870.py

Unmerged paths:
  (use "git add/rm <file>..." as appropriate to mark resolution)

	both modified:      apps/keyboard/js/imes/latin/latin.js
	deleted by us:      apps/keyboard/test/unit/imes/latin/latin_test.js
	both modified:      tests/python/gaia-ui-tests/gaiatest/tests/functional/keyboard/manifest.ini

Comment 27

[Jan Jongboom [:janjongboom] (inactive)]

Attachment: Patch for v2.0

This is a patch for v2.0, let's see if try succeeds though because the UI tests didn't want to run locally for 2.0.

https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=2f130e1b34cc

Comment 28

[Kai-Zhen Li [:seinlin][:kli]]

Attachment: PR for v2.0m

Will merge it into v2.0m when tree is opened.

Comment 29

[Kai-Zhen Li [:seinlin][:kli]]

v2.0m: https://github.com/mozilla-b2g/gaia/commit/07c4e20833abc5558ffd3eef92019c3224d8ca40

Comment 30

[Josh Cheng [:josh]]

This is reported by partner but not major blocker for 2.0M. Make it 2.0M+.

Hi Howie,
It's generic bug. Please decide whether important enough to land on 2.0. Thanks!

Comment 31

[Josh Cheng [:josh]]

(In reply to Josh Cheng [:josh] from comment #30)
> This is reported by partner but not major blocker for 2.0M. Make it 2.0M+.
This is reported by partner for 2.0M. Make it 2.0M+.
> 
> Hi Howie,
> It's generic bug. Please decide whether important enough to land on 2.0.
> Thanks!

Correct first sentence...

Comment 32

[howie [:howie]]

Discussed with Product, no need to land this on 2.0 at this point.

Comment 33

[bhavana bajaj [:bajaj]]

Comment on attachment 8500345 [details] [review]
Patch v3

Minusing approval per : https://bugzilla.mozilla.org/show_bug.cgi?id=1073870#c32

Comment 34

[Shine(Leave from Mozilla)]

Attachment: video

According to issue steps, we cannot reproduce on Woodduck 2.0, Flame 2.1 and 2.2.
See attachment: Verify_video.MP4
Reproducing rate: 0/5

Woodduck2.0 build:
Gaia-Rev        60146ec47cd38a8be8ed22e0116902eceb9ac067
Gecko-Rev       cdfbe9866cf0b71b33454926638ce0cd8bb1fb00
Build-ID        20141117050313
Version         32.0

Flame 2.1 build:
Gaia-Rev        81160ad79e5b4c21967418dd63f1a1d08d77924e
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/3572aa3e6766
Build-ID        20141117001201
Version         34.0

Flame 2.2 build:
Gaia-Rev        ae3a84acaab80a5b35d5542d63e68462273c8a1b
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/2d0a51ef828d
Build-ID        20141117160200
Version         36.0a1