1053647 - [Keyboard] Auto correct improvement

Status: RESOLVED FIXED

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

Description

[Bruce Huang [:bhuang] (inactive)]

Attachment: Keyboard Autocomplete cases

This bug tracks some updates to our auto correct behavior.  Details are in the attached document.  4 cases are addressed:

1. Using space for auto correct: pressing backspace after an auto correct should bring us back to the previous state without a space.
2. Selecting the currently typed word: when the "x" is tapped to use the current word, don't insert an extra space.  Pressing backspace deletes the last letter.
3. Selecting a word in the candidate list: don't insert a space after user selects from the word list.  Backspace brings user back to the state before auto correct kicked in.
4. Typing compound words: after a user selects from a word list, they can then continue typing to create compound words.

Comment 1

[Jan Jongboom [:janjongboom] (inactive)]

(In reply to Bruce Huang [:bhuang] <[e-mail]> from comment #0)
> Created attachment 8472818 [details]
> Keyboard Autocomplete cases
> 
> This bug tracks some updates to our auto correct behavior.  Details are in
> the attached document.  4 cases are addressed:
> 
> 1. Using space for auto correct: pressing backspace after an auto correct
> should bring us back to the previous state without a space.

I don't agree. That would mean (example words in Spanish): 'teka' SPACE -> 'tela ' BACKSPACE -> 'teka'. This is inconsistent with both Android and iOS.

Android: 'teka' SPACE -> 'tela ' BACKSPACE -> 'teka '
iOS: 'teka' SPACE -> 'tela ' BACKSPACE -> 'tela' w/ old suggestion hover under it.

I like the Android way better tbh.

> 2. Selecting the currently typed word: when the "x" is tapped to use the
> current word, don't insert an extra space.  Pressing backspace deletes the
> last letter.

This is what we're already doing in 2.1.

> 3. Selecting a word in the candidate list: don't insert a space after user
> selects from the word list.  Backspace brings user back to the state before
> auto correct kicked in.

I don't agree at all on this. A user has the full intend of selecting a word. That means he's finished with it and wants to continue typing. Manually adding a space seems overkill. Android does this by not showing the space, but when you continue typing it auto adds one. Maybe that's also a nice way of implementing.

> 4. Typing compound words: after a user selects from a word list, they can
> then continue typing to create compound words.

I think the number of times I want to type compound word is lower than the times I just want to type single word. But I guess that depends on the language.

Comment 2

[David Flanagan [:djf]]

Jan: this bug is motivated by long discussions with a partner who has strong feelings about how autocorrect should work. I believe that some of these changes are committed for 2.1.  I forget what the bug is that makes change #1 important.  As you point out, we've apparently already done #2.  #3 is about improving the ability to type compound words, I think. I think #4 is really just the same thing as #3.

Bruce or Omega: the attached spreadsheet is quite helpful about what changes are to be made. It would also be really useful to have an explanation (with specific examples) of why the current behavior is considered buggy and how the changes are going to fix those bugs.

Comment 3

[Omega Feng [:Omega] [:馮於懋]]

Attachment: Keyboard Autocomplete cases

I've updated the attachment. Please see red and green background parts for more explanations for why we make these decisions.

Comment 4

[David Flanagan [:djf]]

Attachment: link to patch on github

Jan: I know you disagree with some of these changes, but could you review the patch anyway? It doesn't look like there are existing tests for the old behavior. I did not write new tests for the new behavior.

Omega: I think that this patch handles the cases in your spreadsheet, but please verify that this is how you want it to behave.

Comment 5

[Omega Feng [:Omega] [:馮於懋]]

Comment on attachment 8475009 [details] [review]
link to patch on github

Hi David,
This patch handles most cases in the spreadsheet except the step 3 of case 3. The "good" in the candidate list should not be highlighted (should not autocorrect). Could you fix that one, thanks!

Comment 6

[David Flanagan [:djf]]

Comment on attachment 8475009 [details] [review]
link to patch on github

Thanks for catching that error Omega. I've updated the pull request with a fix. Would you check it again, please?

Also, I'm redirecting the review from Jan to Rudy. Rudy: I'm going to be on PTO and will not return until after the 2.1 feature complete date. If this patch looks good to you, would you land it for me, please?

Comment 7

[Omega Feng [:Omega] [:馮於懋]]

Comment on attachment 8475009 [details] [review]
link to patch on github

Looks good to me. :)

Comment 8

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

Comment on attachment 8475009 [details] [review]
link to patch on github

Looks good to me, but seems the Gip failed, so I'll amend the patch to make them passed.

Comment 9

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

Attachment: Patch ammended to fix the tests

Updated patch to fix the tests.
Will land after the Gaia tree is re-opened.

Comment 10

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

Landed to Gaia master,
https://github.com/mozilla-b2g/gaia/commit/39cad6c82122b964f12a66771bfbcc14fb342d9e

Comment 12

[Jan Jongboom [:janjongboom] (inactive)]

This breaks suggestions.

On Flame 2.1, English language:

STR:

Type 'Tes', suggestions are Ted* Tea Test
Pick Tea
Press space

Autocorrects to Ted.

Reverted this patch https://github.com/mozilla-b2g/gaia/commit/8fe20d99c35b118cee556b2d2e2c64753637462b

Comment 13

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

It seems the backout in comment 12 did not revert the changes made to python integration tests, so caused the Gaia tree closed.

I have a pull request here to revert the patch here correctly,
https://github.com/mozilla-b2g/gaia/pull/23606

--
Jan, thanks for catching this issue and sorry that I did not test/verify this patch carefully.

Comment 14

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

The tests were backed out as well with,
https://github.com/mozilla-b2g/gaia/commit/b9636e72f1b03a11d0bfce2b41acaccd70eee977

Comment 15

[David Flanagan [:djf]]

Omega: what do you think is the correct autocorrect behavior after selecting a word suggestion.

If the user selects a suggested word like "suggest" and then starts extending that word by adding "ion" should we ever offer an autocorrection, or should autocorrections be turned off for selected word suggestions?

Comment 16

[Omega Feng [:Omega] [:馮於懋]]

(In reply to David Flanagan [:djf] from comment #15)
> If the user selects a suggested word like "suggest" and then starts
> extending that word by adding "ion" should we ever offer an autocorrection,
> or should autocorrections be turned off for selected word suggestions?

In this case, I think autocorrection should be turned off.

Comment 17

[David Flanagan [:djf]]

Attachment: new version of the patch

Jan,

Thanks for backing out the previous version of this patch; I obviously did not test it very well. If you have time, please review this new version of the patch, or if not, please pass the review on to Rudy.

The problem was that the existing code cleared the suggestions but did not fully reset the autocorrect state. Because we used to insert a space after word suggestions, however autocorrects were never triggered on the next keystroke. But when the earlier version of this patch removed that automatic space, we'd get the old autocorrect behavior even though the blue word had been hidden.

This patch now changes that so that the autocorrect state is completely reset.

Omega: as we discussed, in addition to fixing the bug that Jan pointed out, this new version does not allow any autocorrection while the user is adding a suffix to a selected word.

Comment 18

[Omega Feng [:Omega] [:馮於懋]]

Comment on attachment 8485620 [details] [review]
new version of the patch

Looks no problem from me.

Comment 19

[Jan Jongboom [:janjongboom] (inactive)]

Attachment: UI Tests

Hi David, patch seems to work well, but tests fail. I think a rebase went wrong. Anyway, to make sure we are guarded against bugs like this a little better I wrote some UI tests around common scenarios. Can you please incorporate them in the PR? (Also required to pass Try run so hey :-)).

Comment 20

[Jan Jongboom [:janjongboom] (inactive)]

Comment on attachment 8485620 [details] [review]
new version of the patch

r+ if try is green

Comment 21

[Bruce Huang [:bhuang] (inactive)]

Since this was backed out after 2.1 branched, do we need to get uplift approval to get it back in?

Comment 22

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

Comment on attachment 8487123 [details] [diff] [review]
UI Tests

David and Jan,

May I know if this patch is ready to land with the tests?
Please let me know if there is anything I can help with.

Thanks.

Comment 23

[Jan Jongboom [:janjongboom] (inactive)]

Attachment: Patch + tests

I combined UI tests + patch into new PR. Let's see if Try succeeds.

Comment 24

[Jan Jongboom [:janjongboom] (inactive)]

Green try https://tbpl.mozilla.org/?rev=2082f2b2212b14498540b0a03ae4d00024213d9a&tree=Gaia-Try
Landed https://github.com/mozilla-b2g/gaia/commit/112ed2a663de3c1c571f8d4ed1b2343b4c88a6a7

Comment 25

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

Change the flag to reflect the landing state.

Comment 26

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

Comment on attachment 8494375 [details] [review]
Patch + tests

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): This is a feature request to improve the auto-correction UX.
[User impact] if declined: worse usability around auto-correction.
[Testing completed]: Yes, with automated test.
[Risk to taking this patch] (and alternatives if risky): Should be low, covered by tests.
[String changes made]: N/A

Comment 27

[Bruce Huang [:bhuang] (inactive)]

Late request here, but this is a highly desired feature for a partner.

Comment 28

[bhavana bajaj [:bajaj]]

(In reply to Rudy Lu [:rudyl] from comment #26)
> Comment on attachment 8494375 [details] [review]
> Patch + tests
> 
> NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to
> better understand the B2G approval process and landings.
> 
> [Approval Request Comment]
> [Bug caused by] (feature/regressing bug #): This is a feature request to
> improve the auto-correction UX.
> [User impact] if declined: worse usability around auto-correction.
> [Testing completed]: Yes, with automated test.
> [Risk to taking this patch] (and alternatives if risky): Should be low,
> covered by tests.
> [String changes made]: N/A

Its rather too late to late this on 2.0, I think we meant to seek approval on 2.1. So switching the flag. Before getting this landed on 2.1 and keeping in mind the history of fallouts here, I am requesting some basic testing from QA on master/2.2 around this area to ensure there are no regressions or fallouts.

Comment 29

[bhavana bajaj [:bajaj]]

Comment on attachment 8494375 [details] [review]
Patch + tests

Clearing the 2.0 approval request and switching it to 2.1

Comment 30

[bhavana bajaj [:bajaj]]

Josh, can you please help with the verifyme request on this ? Doing some exploratory testing keeping the pass fallouts in mind should be good enough..Feel free to NI the developer if you need more input on testcases..Thanks!

Comment 31

[Bruce Huang [:bhuang] (inactive)]

Assuming we're keeping the ni here on Josh.

Comment 32

[Joshua Mitchell (Inactive)]

Sure- I'll have someone do some halo testing around the patch to make sure there is no fallout or unexpected results.

Comment 33

[Adam Alldredge [:AdamA][Inactive]]

While halo testing around this fix I noticed that the scenario presented in comment 15 isn't implemented. I chose "Suggest" from the suggestions and began to type "ion" and it was giving me word suggestions. Although this may go against the expected behaviour for case 4 in the spreadsheet.

I also noticed that pressing space to use a blue suggest word would place a space after the word, this would break the flow for typing a compound word which changes 3 and 4 were put in to make easier. When I selected the word from the list instead of pressing the space button a space would not appear after the text. I'm not sure if this is expected behaviour or not.

Comment 34

[Ryan VanderMeulen [:RyanVM]]

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