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

VERIFIED FIXED in Firefox OS v2.0M

Status

Firefox OS
Gaia::Keyboard
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: julienw, Assigned: janjongboom)

Tracking

({regression})

unspecified
2.1 S6 (10oct)
ARM
Gonk (Firefox OS)
regression
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.0M+, b2g-v1.4 affected, b2g-v2.0 affected, b2g-v2.0M verified, b2g-v2.1 verified, b2g-v2.2 verified)

Details

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

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

Updated

4 years ago
OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → ARM

Updated

4 years ago
QA Contact: 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
QA Whiteboard: [QAnalyst-Triage?]
status-b2g-v2.0: --- → affected
status-b2g-v2.1: --- → affected
status-b2g-v2.2: --- → affected
Flags: needinfo?(jmitchell)
Keywords: qawanted
Not a regression - always occurs in the KK-base life-cycle
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(jmitchell)
(Reporter)

Comment 3

4 years ago
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.
Keywords: regression
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

4 years ago
Triage: blocking. Hi Rudy, can you take a look? thanks.
Assignee: nobody → rlu
blocking-b2g: 2.0? → 2.0+
Stealing it.
Assignee: rlu → janjongboom
Created attachment 8497566 [details] [review]
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.
Attachment #8497566 - Flags: review?(rlu)
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.
Attachment #8497566 - Flags: review?(rlu)
Attachment #8497566 - Flags: review?(dflanagan)
Attachment #8497566 - Flags: feedback+
Status: NEW → ASSIGNED
Rudy, Looks like bug 1015541 only make the latin.js update the suggestion but not the auto correct when space is pressed?
Flags: needinfo?(rlu)
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.
Flags: needinfo?(rlu)
status-b2g-v1.4: --- → affected
status-b2g-v2.0M: --- → affected
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!)
Attachment #8497566 - Flags: review?(dflanagan) → review-
Flags: needinfo?(rlu)
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.
(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.
(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.
Flags: needinfo?(rlu)
Created attachment 8500313 [details] [review]
Patch v2

Updated the patch based on the comments from djf. Also got rid of the sleep(1) in the test because of that. :-)
Attachment #8497566 - Attachment is obsolete: true
Attachment #8500313 - Flags: review?(rlu)
Comment on attachment 8500313 [details] [review]
Patch v2

Looks good to me.
Thanks.
Attachment #8500313 - Flags: review?(rlu) → review+
(Reporter)

Comment 21

4 years ago
Hey Jan, please ask approval for the branches when you have time :) Thanks !
Flags: needinfo?(janjongboom)
status-b2g-v2.2: affected → fixed
Target Milestone: --- → 2.1 S6 (10oct)
Comment on attachment 8500345 [details] [review]
Patch v3

Already is marked 2.0+
Attachment #8500345 - Flags: approval-gaia-v2.1?
Attachment #8500345 - Flags: approval-gaia-v2.0?
Flags: needinfo?(janjongboom)

Updated

4 years ago
Blocks: 1080481
(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...

Updated

4 years ago
Blocks: 1054172
(Reporter)

Updated

4 years ago
Flags: needinfo?(janjongboom)
Keywords: branch-patch-needed
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
Flags: needinfo?(janjongboom)
Attachment #8500345 - Flags: approval-gaia-v2.1? → approval-gaia-v2.1+
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
Flags: needinfo?(janjongboom)
Created attachment 8505602 [details] [review]
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
Flags: needinfo?(janjongboom)
(Assignee)

Updated

4 years ago
Keywords: branch-patch-needed
Created attachment 8505956 [details] [review]
PR for v2.0m

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

Comment 30

4 years ago
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!
blocking-b2g: 2.0+ → 2.0M+
Flags: needinfo?(hochang)

Comment 31

4 years ago
(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

4 years ago
Discussed with Product, no need to land this on 2.0 at this point.
Flags: needinfo?(hochang)
Comment on attachment 8500345 [details] [review]
Patch v3

Minusing approval per : https://bugzilla.mozilla.org/show_bug.cgi?id=1073870#c32
Attachment #8500345 - Flags: approval-gaia-v2.0? → approval-gaia-v2.0-
Created attachment 8524363 [details]
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
status-b2g-v2.0M: fixed → verified
status-b2g-v2.1: fixed → verified
status-b2g-v2.2: fixed → verified

Updated

4 years ago
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.