1127776 - [Autocorrect] Typing 2 digits is autocorrected to $US (with French keyboard)

Status: VERIFIED FIXED

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

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

STR:
Type 33 and space, in an autocorrected field, e.g. in a SMS message.

Expected:
33 is printed.

Actual:
33 is autocorrected by "$US".


Gaia-Rev        54d92cc0755e5102223276ab23063b5eee74b514
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/522d6c980917
Build-ID        20150125001312
Version         34.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141018.194004
FW-Date         Sat Oct 18 19:40:15 EDT 2014
Bootloader      L1TC00011880

Comment 1

[Johan Lorenzo [:jlorenzo]]

QA wanted for a branch check.

Comment 2

[Pi Wei Cheng [:piwei] (inactive)]

Unable to reproduce the bug on the reported 2.1 build. See video:

https://www.youtube.com/watch?v=_WIikY7y3PE

Can't branch check since I can't repro the bug.

Tested on:
Device: Flame 2.1 (shallow flash, 319MB mem)
BuildID: 20150125001312
Gaia: 54d92cc0755e5102223276ab23063b5eee74b514
Gecko: 522d6c980917
Version: 34.0 (2.1) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Comment 3

[Yeojin Chung [:YeojinC]]

This issue reproduces on Flame 2.1.

Result: In French keyboard, any 2-digit number in the Message input field is auto-corrected with $US.

Device: Flame 2.1 (319mb, full flash)
Build ID: 20150130001202
Gaia: 01bf4bd548be38a762c4e4582925159e6226dfa2
Gecko: 934de7047791
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 34.0 (2.1)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

===============================
This issue does NOT reproduce on Flame Master, 2.2, and 2.0.

Result: In French keyboard, a 2-digit number in the Message input field is NOT auto-corrected.

Device: Flame Master (319mb, full flash)
Build ID: 20150130010210
Gaia: 8238eeacc7030b2cdbf7ab4eba2f36779b702599
Gecko: 29b05d283b00
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 38.0a1 (3.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:38.0) Gecko/38.0 Firefox/38.0

Device: Flame 2.2 (319mb, full flash)
Build ID: 20150130002501
Gaia: d6141fa3208f224393269e17c39d1fe53b7e6a05
Gecko: 63b63054948d
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 37.0a2 (2.2)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

Device: Flame 2.0 (319mb, full flash)
BuildID: 20150130000204
Gaia: 2989f2b2bd12fcc0e9c017d2db766e76a55873b8
Gecko: 6150867fc6a8
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 32.0 (2.0) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Comment 4

[Mike Lien[:mlien]]

Let's get the regression-window first

Comment 5

[Brogan Zumwalt [Inactive]]

Regression Window:

Last working B2G-34 build:
Device: Flame 2.1
BuildID: 20150121100601
Gaia: 5745e44273aeda4861d0bf369fc47851c69a040b
Gecko: d26b49d9c2fe
Version: 34.0 (2.1)
Firmware: V18D-1
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0


First broken B2G-34 build:
Device:  Flame 2.1
BuildID: 20150121143637
Gaia: 2055fc40a8bd2af1908979cb45da6b7d1c4ced0b
Gecko: 38ac70ca969b
Version: 34.0 (2.1)
Firmware: V18D-1
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0


Working Gaia with Broken Gecko issue does NOT reproduce:
Gaia: 5745e44273aeda4861d0bf369fc47851c69a040b
Gecko: 38ac70ca969b

Working Gecko with Broken Gaia issue DOES reproduce:
Gaia: 2055fc40a8bd2af1908979cb45da6b7d1c4ced0b
Gecko: d26b49d9c2fe

Gaia pushlog:
https://github.com/mozilla-b2g/gaia/compare/5745e44273aeda4861d0bf369fc47851c69a040b...2055fc40a8bd2af1908979cb45da6b7d1c4ced0b

Comment 6

[KTucker [:KTucker][Inactive 3/4/2016]]

Jan, can you take a look at this? Looks like the work done on Bug 1093011 might have caused this issue to occur.

Comment 7

[Jan Jongboom [:janjongboom] (inactive)]

Reproduced on gaia master.  When 1 or 2 digits are typed '$US' is suggested. When 3 digits are typed '$US' is autocorrect (blue) suggestion. Interesting. The suggestion comes actually from the dictionary, which holds $US with quite a high frequency (87): https://github.com/mozilla-b2g/gaia/blob/master/apps/keyboard/js/imes/latin/dictionaries/fr_wordlist.xml (line 27593 today).

Now the problem here is that when we encounter a dictionary word that is a punctuation character, we just add it to the candidate list with a boost, because maybe the user omitted the punctuation char out of ease-of-type (https://github.com/mozilla-b2g/gaia/blob/a98a6c44a03613aed97858510b197245c36f1ec2/apps/keyboard/js/imes/latin/predictions.js#L776). Normally not such a problem, but I don't think we should do this for start of words, but a las. Now because $ is no longer in the variant forms it is now treated as a punctuation character, which is wrong, and thus it gets added to every input that starts with a char that is in the character table (1,2,3 are there because they are in dict. words). Normally gets filtered out because nearbyKeyMap or suggestions, but numbers aren't in the nearbyKeymap.

Comment 8

[GitHub Autolander Bot]

Attachment: [PullReq] comoyo:bug1127776 to mozilla-b2g:master

Comment 9

[Jan Jongboom [:janjongboom] (inactive)]

Comment on attachment 8559097 [details] [review]
[PullReq] comoyo:bug1127776 to mozilla-b2g:master

Quite straightforward:

- Add $ to the rootToAccentedForm table, with itself as alternative to avoid it being treated as punctuation
- Update xml2dict.py to not treat $ as alt for S anymore
- Add some lines in the tests so we can test other languages than en_us

Comment 10

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

Comment on attachment 8559097 [details] [review]
[PullReq] comoyo:bug1127776 to mozilla-b2g:master

It turns out I am not a suitable reviewer for this part of code, as I don't understand why master behaves differently with v2.1, is it because of Bug 971688?

I would like to ask for John's help to see if he could help review this change.

John, could you help us here?
Thank you.

Comment 11

[Jan Jongboom [:janjongboom] (inactive)]

No, it's because $ was treated as alternative to S. We removed this in bug 1093011.

Comment 12

[Jan Jongboom [:janjongboom] (inactive)]

The above bug has approval for 2.1, so we might need it here too, or remove the patch from 2.1.

Comment 13

[Daniel Holbert [:dholbert]]

[Tracking Requested - why for this release]:
Regression from something that was backported to 2.1 (see comment 12).

Comment 14

[John Lu [:mnjul][Please NI or I'll miss]]

Comment on attachment 8559097 [details] [review]
[PullReq] comoyo:bug1127776 to mozilla-b2g:master

Jan,

Thanks for spotting the issue and for the patch.

The rationale behind the patch and the patch itself look pretty good, but I think we should use AZERTY layout for fr tests. Indeed it is not closely related to this bug, but to prevent potential future false negative/positive in fr suites I think we'd better fix it. But it's your call whether to put that in a follow-up bug, though -- in that case please just convert this f+ to r+.

(Also you've got a Linter error at Gaia-try :( )

(In reply to Jan Jongboom [:janjongboom] (Telenor) from comment #9)
> - Update xml2dict.py to not treat $ as alt for S anymore

Ah! Unfortunately the diacritics table isn't really used there. If you look at the code you can see the TSTTree.normalizeChar call has been commented out (by David long time ago), which nullifies the need for _Diacritics dict. I discussed with David some weeks ago regarding this and I decided to remove the table (to avoid confusion like this) in bug 1124150, but I haven't had time to finish the bug yet.

(In reply to Jan Jongboom [:janjongboom] (Telenor) from comment #7)
> Normally gets filtered out because nearbyKeyMap or suggestions, but numbers
> aren't in the nearbyKeymap.
I think we might need to ponder this over w.r.t the whole architecture. The word list allows for arbitrary letters, not just those from the first page of a keyboard layout, while the prediction engine more-or-less relies on nearbyKeymap to grade candidates, which are only set for the first-page layout. When we need to deal with the characters out of the map (numbers and $ as we've seen [1]), things get a bit muddled out there. Any thoughts?

[1] and probably those punctuation marks that don't really should get boosted as apostrophes in "dont -> don't"

Comment 15

[Daniel Holbert [:dholbert]]

[This is only an issue with French keyboard, per comment 3, right? --> clarifying summary w/ that]

Comment 16

[Jan Jongboom [:janjongboom] (inactive)]

:dholbert, it's an issue with any locale that has a dictionary word that starts with a $. Which happens to be only French ;-)

Comment 17

[Jan Jongboom [:janjongboom] (inactive)]

Comment on attachment 8559097 [details] [review]
[PullReq] comoyo:bug1127776 to mozilla-b2g:master

Hi, yeah, lazyness from my side to use qwerty there. I added the azerty keymap as well.

Thanks for spotting the linting error, didn't install gjslint on my new laptop yet ;-)

Let's see if try passes. https://treeherder.mozilla.org/#/jobs?repo=gaia-try&revision=a1e90c7786e4

Comment 18

[John Lu [:mnjul][Please NI or I'll miss]]

Comment on attachment 8559097 [details] [review]
[PullReq] comoyo:bug1127776 to mozilla-b2g:master

Here we go \o/ !

Comment 19

[Jan Jongboom [:janjongboom] (inactive)]

Alright try 6 doesn't want to re-run, but passes locally. https://treeherder.mozilla.org/#/jobs?repo=gaia-try&revision=bd7aec9ac7f6

Anyway gaia is closed, so let's try to land on Monday.

Comment 20

[Julien Wajsberg [:julienw]]

[Blocking Requested - why for this release]: I definitely get this issue in v2.1, this is _very_ painful.

Comment 21

[Jan Jongboom [:janjongboom] (inactive)]

Unless we ship in France in 2.1 I would not block on this issue.

Comment 22

[Jan Jongboom [:janjongboom] (inactive)]

Try is green. Landed https://github.com/mozilla-b2g/gaia/commit/db5648d1c637bf068a9f460148a3d4d0aac89b27

Comment 23

[Julien Wajsberg [:julienw]]

We shipped in France with v1.3 but a lot of users have upgraded to more recent versions.

I think we can ask approval for v2.1/v2.2. Thanks for the fix !

Comment 24

[Daniel Holbert [:dholbert]]

(In reply to Julien Wajsberg [:julienw] from comment #23)
> I think we can ask approval for v2.1/v2.2.

(Jan, mind requesting backport approval?)

Comment 25

[Jan Jongboom [:janjongboom] (inactive)]

Comment on attachment 8559097 [details] [review]
[PullReq] comoyo:bug1127776 to mozilla-b2g:master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): bug 1093011
[User impact] if declined: Users with French dictionary get $US suggested whenever they type a number
[Testing completed]: Landed, no regressions so far
[Risk to taking this patch] (and alternatives if risky): Changes in the autocorrect algorithm are always a bit scary. Alternative: just remove $US entry from french dictionary for 2.1
[String changes made]: None

Comment 26

[Benjamin Bouvier [:bbouvier] (inactive)]

Hi, any chance we could get this in v2.1 soon, please? It's pretty annoying when typing numbers...

Comment 27

[Johan Lorenzo [:jlorenzo]]

The patch looks good enough to me to be uplifted to 2.1 but release management has the final call here.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

This needs rebasing for v2.1 uplift. Also, this still needs a v2.2 approval request AFAICT?

Comment 29

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

If it got 2.1? approved, might as well make it a 2.1+

Jan, Could we get a rebase of the patch for 2.1 please?

Comment 30

[GitHub Autolander Bot]

Attachment: [gaia] comoyo:bug112776-v2.1 > mozilla-b2g:v2.1

Comment 31

[Jan Jongboom [:janjongboom] (inactive)]

Comment on attachment 8567830 [details] [review]
[gaia] comoyo:bug112776-v2.1 > mozilla-b2g:v2.1

Try running https://treeherder.mozilla.org/#/jobs?repo=gaia-try&revision=3c9a9704c3bf

Comment 32

[Jan Jongboom [:janjongboom] (inactive)]

New try https://treeherder.mozilla.org/#/jobs?repo=gaia-try&revision=c22c1a64bf03

Comment 33

[Jan Jongboom [:janjongboom] (inactive)]

Can't get the test to work properly on try, passes fine local. Removed it: https://treeherder.mozilla.org/#/jobs?repo=gaia-try&revision=4a338743d3dd

But all other test suites are busted so I don't know if we are supposed to land on v2.1... Ryan, you know what is going on on this branch?

Comment 34

[Ryan VanderMeulen [:RyanVM]]

Gu matches a known intermittent. Gij is known to be unreliable on v2.1. LGTM.

What about the v2.2 approval request?

Comment 35

[Ryan VanderMeulen [:RyanVM]]

v2.1: https://github.com/mozilla-b2g/gaia/commit/86af0ca427adad12c3109124f31bef2fd9614e47

Comment 36

[Jan Jongboom [:janjongboom] (inactive)]

Comment on attachment 8559097 [details] [review]
[PullReq] comoyo:bug1127776 to mozilla-b2g:master

Already has 2.1+

Comment 37

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/228d8ef45bb34da313e6bca0e339f04231b6d8d8

Comment 38

[Yeojin Chung [:YeojinC]]

This issue is verified fixed on Flame Master, 2.2, and 2.1.

Result: 2-digit numbers do not get auto-corrected in French keyboard.
 
Device: Flame Master (KK, 319mb, full flash)
Build ID: 20150302010223
Gaia: f34ce82a840ad3c0aed3bfff18517b3f6a0eb37f
Gecko: eea6188b9b05
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 39.0a1 (3.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:39.0) Gecko/39.0 Firefox/39.0

Device: Flame 2.2 (KK, 319mb, full flash)
Build ID: 20150302002504
Gaia: 77609916ca5ab721150fab2b7bc5c37f43ee3a5a
Gecko: 27ab8aa34201
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 37.0 (2.2)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

Device: Flame 2.1  (KK, 319mb, full flash)
Build ID: 20150302001220
Gaia: 5d3479fdd438412adee4452720856b6b771fe5cd
Gecko: 9bf4c663241f
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 34.0 (2.1)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0