Closed Bug 1048792 Opened 6 years ago Closed 6 years ago

Keyboard should not autocorrect words with different char count than input

Categories

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

x86
macOS
defect
Not set
normal

Tracking

(b2g-v2.2 fixed)

RESOLVED FIXED
Tracking Status
b2g-v2.2 --- fixed

People

(Reporter: janjongboom, Assigned: janjongboom)

References

Details

Attachments

(1 file, 1 obsolete file)

46 bytes, text/x-github-pull-request
janjongboom
: review+
Details | Review
Related bugs: bug 971688 and bug 981602

After working with FFOS 2.0 for a while again I see that a lot of my annoyances with autocorrect come from the fact that it suggests words with a different char count than my input. I think that in 90% of the time that's not correct behavior. So I suggest to never autocorrect words that have a char count mismatch with the input. We can do suggestions of course.
Summary: Keyboard should not suggest words with different char count than input → Keyboard should not autocorrect words with different char count than input
Attached file Patch (obsolete) —
Just counting \w, so we can still do stuff like dont -> don't
Assignee: nobody → janjongboom
Attachment #8467641 - Flags: review?(dflanagan)
Comment on attachment 8467641 [details] [review]
Patch

From a technical standpoint, I'd move the regexp matching code into the if statement so that we don't have to run the regexps if we're not going to autocorrect for other reasons.

But r- here because I suspect that requiring an exact match is too strict. How about allowing +/- 1 for the lengths? That would still prevent obviously bad things like asjan->ashanti (as you point out in bug 981602) but would allow correction of spelling errors like Folow->Follow 

Can we try out a less severe change first?
Attachment #8467641 - Flags: review?(dflanagan) → review-
Also note that if we allowed a +/- 1 difference in length then no special case would be required for English contractions and we wouldn't have to run a regexp over both strings.
(In reply to David Flanagan [:djf] from comment #2)
> Comment on attachment 8467641 [details] [review]
> Patch
> 
> From a technical standpoint, I'd move the regexp matching code into the if
> statement so that we don't have to run the regexps if we're not going to
> autocorrect for other reasons.
> 
> But r- here because I suspect that requiring an exact match is too strict.
> How about allowing +/- 1 for the lengths? That would still prevent obviously
> bad things like asjan->ashanti (as you point out in bug 981602) but would
> allow correction of spelling errors like Folow->Follow 
> 
> Can we try out a less severe change first?

Yeah, makes sense. Android does it more or less this way. Although they take frequency into account as well. F.e. 'Folow' -> 'Follow', but 'Zoolgy' does not autocorrect to 'Zoology'.
Flags: needinfo?(dflanagan)
Comment on attachment 8467641 [details] [review]
Patch

Alright, I changed it around a bit.

Length diff = 0 -> Always OK
Length diff >= 2 -> Always NOK
Length diff = 1 -> OK if freq is over 5

That should do the trick, Follow is f.e. 6.2.
Attachment #8467641 - Flags: review- → review?(dflanagan)
Flags: needinfo?(dflanagan)
Comment on attachment 8467641 [details] [review]
Patch

Looks good to me, but note that I think Tim has recently removed all the const keywords, so this can run in regular web browsers. So you might need to update the patch to use var instead of const.

Sorry this review took so long.
Attachment #8467641 - Flags: review?(dflanagan) → review+
Changed const -> var and landed @ https://github.com/mozilla-b2g/gaia/commit/694f5b760eed6104eb536824af2a8546cfdd94bf
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Backed out for causing Gip errors like: https://tbpl.mozilla.org/php/getParsedLog.php?id=47179302&tree=B2g-Inbound

These were also present in the gaia-try run: https://tbpl.mozilla.org/?rev=7b3c14a3fa0906728730c6900b30a1364efc8833&tree=Gaia-Try

https://github.com/mozilla-b2g/gaia/commit/000e17ab3f3e67b40333a4baf70e5aea9a6a2205
Status: RESOLVED → REOPENED
Flags: needinfo?(janjongboom)
Resolution: FIXED → ---
But that's an intermittent with a bug report et al, or is it persistent?
Flags: needinfo?(kgrandon)
Hjm, seems persistent. Not good. My bad.
Flags: needinfo?(kgrandon)
Appears this may have been falsely blamed as this failure persists on b-i. Investigating, and if it's not the cause we can re-land.
Attached file Patch v2
Attachment #8467641 - Attachment is obsolete: true
Attachment #8482810 - Flags: review+
@Kevin, test failure was correct. Thanks for backing out.
Flags: needinfo?(janjongboom)
Jan,

With this change, "keyboard" won't be shown as a suggestion, when you type "keyboa" but will be when you type "keyboar".
Is this as expected?
Flags: needinfo?(janjongboom)
Nope, but this is not the culprit, I would earlier suspect that changing bug 1048869 makes us hit bug 971688, but let me check when I get in the office.
Flags: needinfo?(rlu)
Ok, thanks for digging into this.
Flags: needinfo?(rlu)
Duplicate of this bug: 1090861
You need to log in before you can comment on or make changes to this bug.