874011 - [keyboard] No change in word suggestion language even when the keyboard layout is changed.

Status: VERIFIED FIXED

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

Description

[Leo]

Title: No change in word suggestion language even when the keyboard layout is changed.
2.Precondition : Settings > keyboard > Select word Suggestion > Select English, spanish,Portuguese
3.Tester's Action:1.Settings > Language > English.
                  2.Message > text > keyboard > Press GLOBE icon> select Spanish 
                   OR 
                   1.Settings > Language > Espanol 
                   2.Message > text > keyboard > Press GLOBE icon> select English
4. Detailed Symptom (ENG.) : Word suggestion does not take into account of keyboard layout langauge.it suggest words according to the setting language.
5. GAIA version : 706139a6c6b08fcb5cac78f6be4a1b08121a5c63
6. Expected : Word suggestion should suggest words in the langauge in which the keybaord layout is set.
7.Reproducibility: Y
1)Frequency Rate : 100%
8.Comparison Results : 
1)Model Comparing : 
9. Attached files: 
1)Log : 
2)Test Contents : 
3)Video file :

Comment 1

[Walter Chen[:ypwalter][:wachen]]

Hi,

I believe that we currently only do suggestion for English language. I will cc PM for answering this question.

Joe, 

I am not sure who would be familiar with this. Could you needinfo from the right person to answer this bug?

Comment 2

[Joe Cheng [:jcheng] (please needinfo)]

rlu for initial comment?

Comment 3

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

For now, the word suggestion is based on the user language, instead of the keyboard layout.

Right now, we have a similar issue, Bug 867175, to address this, but it did not get priority, so we have not reviewed/evaluated the solution.

Thanks.

Comment 4

[Leo]

Attachment: Bug_874011_Patch for v1-train

Hi,

Provided the patch for v1-train.
Please review and provide your comments.

Thanks,
Leo

Comment 5

[David Flanagan [:djf]]

I don't have time to review this at the moment, but please see also bug 867175. Is that patch similar to the one attached there?

Leo, would you take a look at Mihai's patch in bug 867175?

Mihai, would you take a look at Leo's patch in this bug?

I am late in reviewing both of them. Hopefully we do not need two separate patches here. 

Also, given that this bug was just filed two days ago, is a duplicate, and hasn't been triaged yet, can it really be part of the QE2 milestone?

Comment 6

[Mihai Cirlanaru [:mihai][:mcirlanaru]]

Based on the description, this is definitely a duplicate of bug 867175.

My patch for that bug goes in similar lines with the one attached here from what I see, with the addition of disabling auto-correction when switching to keyboards that don't have a dictionary (i.e. no candidate panel is shown).

Comment 7

[David Flanagan [:djf]]

Thanks Mihai.  Do you know if your patch will apply cleanly on v1-train?  (If you don't know how to figure that out, that's okay; I can check)

Comment 8

[Mihai Cirlanaru [:mihai][:mcirlanaru]]

Hi David, my patch shows 3 minor conflicts on v1-train of the form:

++<<<<<<< HEAD
++=======
+   currentKeyboardState = state;
+   currentInputMode = state.inputmode;
++>>>>>>> 873ec1b... Bug 867175 - [Keyboard] Set suggestions based on keyboard language

-- definitely quick to fix upon uplift.

Let me know if I can help with a PR to v1-train fixing the conflicts once the patch for bug 867175 is r+.

Comment 9

[Leo]

(In reply to Mihai Cirlanaru [:mihai][:mcirlanaru] from comment #6)

The patch uploaded for Bug 867175 by you is made from master.
I tried with Master Gaia code and the Patch seems to work fine in master 
but is not working for V1 train.

Also

1.>The auto correction feature is not supported for V1 train for right now.
 As your code is written for master,auto correction part of code is useful
 but is not needed for V1 train.

 for ex -
    disableAutoCorrection: disableAutoCorrection,
    ..............................................
     function disableAutoCorrection() {
    correcting = false;
    } ..
 and other codes related to auto correction in patch.

2> needscandidatepanel() is not defined in V1 train

3> Also there is duplicate piece of code for inputMethod.activate { }
same codes at two place.

Please check and give your comment.

Comment 10

[Mihai Cirlanaru [:mihai][:mcirlanaru]]

(In reply to Leo from comment #9)
> 1.>The auto correction feature is not supported for V1 train for right now.
>  As your code is written for master,auto correction part of code is useful
>  but is not needed for V1 train.
> 
>  for ex -
>     disableAutoCorrection: disableAutoCorrection,
>     ..............................................
>      function disableAutoCorrection() {
>     correcting = false;
>     } ..
>  and other codes related to auto correction in patch.

I was not aware this is not in v1-train yet.

> 
> 2> needscandidatepanel() is not defined in V1 train

Although the function needsCandidatePanel() is not defined for v1-train, the entries in keyboard layouts have needsCandidatePanel and it's checked as follows in keyboard.js:

  if (!Keyboards[keyboardName].needsCandidatePanel && candidatePanelEnabled)

I am not sure if the patch introducing the needsCandidatePanel function (cf. bug 863680) will be uplifted soon, since it landed almost two weeks ago.

> 
> 3> Also there is duplicate piece of code for inputMethod.activate { }
> same codes at two place.

This duplicate you are mentioning is needed because the inputMethod.activate needs to be called when:

1. showKeyboard is called: to properly initialize the input method with the currently active keyboard when displaying the keyboard.

2. SWITCH_KEYBOARD key (globe button) is pressed: to properly activate the language dictionary of the newly selected keyboard.

From my check, the patch in bug 867175, being based on master, depends on the patches for bug 863680 (needsCandidatePanel function), bug 860462 (auto-correction in latin.js), and most probably bug 865484 (auto-correction refactoring). I will add the respective bugs as blocking for bug 867175.

Comment 11

[Leo]

(In reply to Mihai Cirlanaru [:mihai][:mcirlanaru] from comment #10)

David,

THe respective bugs which are blocking bug 867175 mentioned by  Mihai Cirlanaru
seem less likely to be uplifted in V1-train as said by you.
The patch in duplicate  Bug 867175 cannot be uplifted in V1-train in such condition.
Please see the above comment.

The patch uploaded here(bug_874011) is working fine with v1-train and can be applied cleanly.
Please check and give your suggestions if it can be reviewed.

Comment 12

[David Flanagan [:djf]]

Leo, Mihai: thanks to both of you for producing patches for this bug.  I'd like to get it fixed on master and on v1-train. But I can't uplift without approval.  I'm way behind on my reviews, and this takes lower priority than reviewing patches that do have approval for v1-train, so it may be a few more days.

Mihai: Leo is right that we're unlikely to be able to justify uplifting all the other patches to v1-train just to get this fix in.  I'd like to have a single patch that works on both branches, but if necessary, I'll land both of your patches on different branches.

But as I said, I haven't even had time to look at them yet.  Sorry for the delay!

Comment 13

[David Flanagan [:djf]]

Comment on attachment 752713 [details] [diff] [review]
Bug_874011_Patch for v1-train

Review of attachment 752713 [details] [diff] [review]:
-----------------------------------------------------------------

I don't think this patch can work.  See my comments on bug 867175 (and on the github patch attached to that bug) for more details and for an approach that might work.

::: apps/keyboard/js/imes/latin/latin.js
@@ +118,4 @@
>  
>    // This gets called whenever the keyboard pops up to tell us everything
>    // we need to provide useful typing assistance.
> +  function activate(lang, state, options) {

It doesn't make sense to change the function signature here without adding any code that uses the new options object.

::: apps/keyboard/js/keyboard.js
@@ -1314,5 @@
> -     * when the keyboards have the same input method.
> -     * So to switch from a latin to an asian keyboard, you'd have to
> -     * lose focus and then refocus the input field.  In practice, I think
> -     * that asian keyboards have input methods that handle the latin case
> -     * so this probably isn't an issue.

This is an important comment that explains why what you're trying to do here won't work.  You can't just delete the comment :-)

@@ +1441,5 @@
>    IMERender.ime.classList.remove('full-candidate-panel');
>  
>  }
> +//activate input method
> +function activateInputmethod(currentKeyboardName) {

It seems unnecessary to create a new function to do this. And I don't understand why you changed the signature of the activate() method.

@@ +1442,5 @@
>  
>  }
> +//activate input method
> +function activateInputmethod(currentKeyboardName) {
> +  inputMethod.activate(currentKeyboardName, state, {

The problem with this patch is that you are using a stale version of the state object. That object includes the content of the text field. But if the user has already edited the text field before switching keyboards, the state is no longer current. Passing it to activate will cause incorrect word suggestions.

Comment 14

[Leo]

Attachment: Modified patch for v1-train

Hi David,

Please review the patch modified as per the suggestions provided by you.Tested on v1-train.
Provide your comments on this.
Thanks for your support.

Thanks,
Leo

Comment 15

[Leo]

@Mihai Cirlanaru,

Please review the patch.This patch can also work in master if changes are applied manually.This patch is tested on v1-train.

Thanks,

Comment 16

[David Scravaglieri [:scravag]]

We need a UX input on this bug. If I use a French keyboard Layout to write a message in English. I would like English word suggestion.
So the question what is the right behavior.

Comment 17

[Leo]

Attachment: Patch for V1-train

Patch with some corrections.

Comment 18

[David Scravaglieri [:scravag]]

For this version according to UX we will keep the behavior expected in Description and a better word suggestion management will happen in next release.

Comment 19

[Stephany Wilkes]

Assigning to Francis, as keyboard is his domain. Per Comment 18, Francis, we will add a feature for 1.2 that more completely addresses suggestions like adding a setting for having the keyboard and word suggestion languages match until the user decides otherwise, etc. 

For this one that is leo, then, please verify the expected behavior above and I'll add the 1.2 work to our list for keyboard.

Comment 20

[Stephany Wilkes]

Adding Francis since this is the keyboard domain. In triage, we discussed (for 1.2) rephrasing this feature as "The keyboard and word suggestion languages should match until the user changes a setting otherwise." 

For this release, David's comment 16 question on what the expected behavior is, is what the needinfo is for.

Comment 21

[Rob MacDonald [:robmac]]

(In reply to David Scravaglieri [:scravag] from comment #16)
> We need a UX input on this bug. If I use a French keyboard Layout to write a
> message in English. I would like English word suggestion.
> So the question what is the right behavior.

Hi David...

By default, the desired behaviour is that word suggestions be displayed for the language of the current keyboard only. First, because it prevents the user from being overwhelmed with word choices in the event they've enabled multiple languages. Second, if the user wants suggestions for a different language, they can easily change the language selection on the keyboard.

That said, although this should be the default behaviour, I would propose providing a setting for future releases that allows bilingual word suggestions. For users that do switch back and forth, this is a very handy feature. The key point, though, is that users would have to opt into this as opposed to it being a default.

- Rob

Comment 22

[Leo]

Attachment: Pull Request #10146 : Change in word suggestion langauge supported

Please check the pull request at
 https://github.com/mozilla-b2g/gaia/pull/10146

and verify if it fits the UX input given by Rob MacDonald
The patch will check if user langauge is same as keyboard Name or not and
if its different,then word suggestion is changed according to keyboardName.

Comment 23

[Dylan Oliver [:doliver]]

Comment on attachment 756955 [details]
Pull Request #10146 : Change in word suggestion langauge supported

Redirecting review to Mihai.

Comment 24

[Mihai Cirlanaru [:mihai][:mcirlanaru]]

Comment on attachment 756955 [details]
Pull Request #10146 : Change in word suggestion langauge supported

Thanks for the patch Leo, it seems to work on master (without addressing auto-correction as bug 867175), however, I could not test it on v1-train since you based your pull request on gaia/master and there are some merge conflicts (when applying it to v1-train):
> error: patch failed: apps/keyboard/js/keyboard.js:1454

There is still some work to be done, for which I have commented on GitHub. Let me know if you have any questions (feel free to reply on GitHub).

Comment 25

[Leo]

(In reply to Mihai Cirlanaru [:mihai][:mcirlanaru] from comment #24)

> however, I could not test it on v1-train
> since you based your pull request on gaia/master and there are some merge
> conflicts (when applying it to v1-train):
> > error: patch failed: apps/keyboard/js/keyboard.js:1454

Thanks Mihai,
This conflict occurs because 
             "correct: correctionsEnabled;"
is not defined for V1-train.This patch is made from Master Gaia.
It can be applied manually there.No problem with patch.

> There is still some work to be done, for which I have commented on GitHub.
> Let me know if you have any questions (feel free to reply on GitHub).

I have replied to your comments needing some suggestions and review.
Please check it.Will make a new patch then.

Thank you

Comment 26

[David Flanagan [:djf]]

Comment on attachment 756955 [details]
Pull Request #10146 : Change in word suggestion langauge supported

I like the setLanguage() approach taken in this patch.

But r-; see my comments on github.

Between you and Mihai, I think we'll get this bug fixed one way or another!

Comment 27

[Julien Wajsberg [:julienw]]

Mihai, David, please dupe one of this bug or Bug 867175... this should have been done since the start because this adds confusion, as we now have 2 different patches for 2 different branches.

Comment 29

[David Flanagan [:djf]]

This bug has been idle. Mihai has been on vacation. I was going to grab the other bug and start working on it there, but I'll work on it here.

This bug has had two developers working on multiple patches and I keep giving r- to them. Its a tricky bug to get right because:

1) Our settings app has a weird "other latins" keyboard category that enables a bunch of less-frequently used european keyboard layouts as one big group. No one who works on the keyboard knows why we do that. The best guess is that it is a hack left over from the very early days of Gaia.

2) Some of the keyboards in that group do not use the latin input method.

3) Even if we make the keyboards in that group use the latin im, we don't have dictionaries for some of the languages.

It is relatively easy to switch the Latin IM from one dictionary to another when the keyboard layout changes.  But switching from one IM to another is impossible to do in the general case.  And setting a keyboard layout to use the latin im when there is not a dictionary for that language is sort of a silly thing to do.

Fortunately, Andreas recently noticed that Android has released a lot of new wordlists for languages that we don't support.  Bug 884752 gives us lots of new dictionaries.

That means we have the option of simplifying this bug by converting all of our keyboard layouts to use the latin IM.

Unfortunately, if we all all the dictionaries we have to the keyboard app, we end up tripling the size of the keyboard from 10mb to 30mb.  Andreas thinks (see bug 884752) that this should be configurable.

I propose, therefore, that we spend some extra time on this bug to resolve all these issues and end up with something that is simpler and easy to configure so that our partners can create builds with customized sets of keyboard layouts and dictionaries.

This will probably involve:

1) Some configuration option for specifying which keyboards and dictionaries should be in the build. I think we currently use a makefile variable for localization languages, so maybe we can do something similar for the set of keyboards.

2) Keyboard layouts which specify that they use the latin im can also specify the dictionary they use.

3) The settings app currently uses a static JSON file (I think) to determine which keyboards to list.  We'll need to generate that file from the Makefile, I think.

4) We'll need to keep master copies of all the supported dictionaries outside of the keyboard app, and have the Makefile copy in only the dictionaries required for the build.

5) We'll probably want to get rid of the "otherlatins" keyboard group so that French speakers don't have to also enabled the Czech keyboard, etc. I think we need to keep the concept of a keyboard group, however, because Serbia uses both the latin and cyrillic alphabets and we want to enable both of those keyboards with one switch.

This sounds like a big change, but if we do it right, I think it will end up as an overall simplication. Given that this bug (and 867175) have been open for a month and haven't been solved, uplifting a larger but simpler patch might actually be less risky than doing a minimal complicated patch here.

Comment 30

[David Flanagan [:djf]]

Taking this for myself.

Comment 31

[David Flanagan [:djf]]

I think I'll actually start work on the keyboard/dictionary configurability stuff in bug 884752 instead of here.

Comment 32

[Julien Wajsberg [:julienw]]

Please ping me if you need any help (testing, being a duck, etc) as I'd really want this to land ;)

Comment 33

[Mihai Cirlanaru [:mihai][:mcirlanaru]]

Hi David,

Glad to see that we are in the end concentrating on one bug instead of two. There is however one aspect that bug 867175 was considering (as per comment 2 from bug 867175) and this bug skipped, which I think is relevant: auto-correction (i.e. disabling it when switching keyboards).

Feel free to take some of the code/ideas I submitted in the pull request for bug 867175 (https://github.com/mozilla-b2g/gaia/pull/9774) or let me know if can help improving it for the current bug.

(In reply to David Flanagan [:djf] from comment #29)
> 
> 5) We'll probably want to get rid of the "otherlatins" keyboard group so
> that French speakers don't have to also enabled the Czech keyboard, etc. I
> think we need to keep the concept of a keyboard group, however, because
> Serbia uses both the latin and cyrillic alphabets and we want to enable both
> of those keyboards with one switch.
> 

There is a bug that tracks this (bug 867883), but due to its low priority (tracking-b2g+), I didn't work on it yet. I will most probably find some time to fix it this upcoming week.

Comment 34

[David Flanagan [:djf]]

comment 29 is much too ambitious.  We need a simple fix for this, like one of the versions that Mihai produced for 867175.

Comment 35

[David Flanagan [:djf]]

I've run out of time to get this done on the 24th, so I'm going to miss the QE3 deadline.

The patch I'm working on will be similar to ones that Mihai has done, but I'm introducing a simplifcation. In Layout.js, every layout that has imEngine: 'latin', will also have an autoCorrectLanguage property that specifies the dictionary to be used with the layout.

Comment 36

[David Flanagan [:djf]]

Attachment: link to patch on github

Mihai: I think you're the person who has thought about this bug the most, and are probably the best qualified to review this patch.  What do you think? Have I missed anything that was in your patch?

Leo: if you're still following this bug, you're welcome to review as well.

Comment 37

[Mihai Cirlanaru [:mihai][:mcirlanaru]]

Comment on attachment 767477 [details]
link to patch on github

Looks very good, David! All the special cases I was testing in my approach for bug 867175 are taken care of in your patch.

I have a few comments on GitHub regarding the use of 'needsCandidatePanel' in layout.js, I think it is not really necessary if 'autoCorrectLanguage' is introduced. Feel free to request the review again once you address my comments.

Comment 38

[David Flanagan [:djf]]

Comment on attachment 767477 [details]
link to patch on github

Mihai,

Thanks for the reminder about the needsCandidatePanel properties whose values were strings. I've removed those because you are right that they are redundant for any keyboard layout that also has autoCorrectDictionary defined.  But I've left them for the asian keyboard layouts. (I honestly don't know whether those are supported at all anymore, but I'm afraid to remove them in case we do have builds that are depending on them.)

I've modified the needsCandidatePanel() function accordingly.

I also found and fixed two other minor issues in this update:

1) If both the Cyrillic and Serbian keyboard groups were enabled, then sr-cyrl was getting added to the list of keyboards twice, which means that our technique of cycling through keyboards when you just tap the globe key does not work and gets stuck in a cycle. So I added code to prevent duplicates in supportedKeyboardNames.

2) switchKeyboard() was calling resetKeyboard() which was undoing the capitalization state of the keyboard. I removed the call to resetKeyboard(). This also means that if the user is in one of the alternate layouts when the keyboard is switched, they'll stay in that layout, which is probably the right thing.

Comment 39

[Mihai Cirlanaru [:mihai][:mcirlanaru]]

Comment on attachment 767477 [details]
link to patch on github

Thanks for taking care of those issues as well, David.

While checking the code and doing some more testing I noticed a couple more problems that might appear with the patch (check my comments on GitHub), however, once those are addressed, this should be good to land.

Marking the patch with r+ with the condition that you fix the issues I pointed. Thanks!

Comment 40

[David Flanagan [:djf]]

Mihai,

Thanks for catching those other bugs.  I've fixed them but am asking for another round of review because this bug has proven so challenging.  My fix to switching between layouts that use different IMs is to file a new bug, and in the meantime, force the keyboard closed when that happens so that the user has to refocus the input field, which will allow the new im to be properly initialized. Its bad UX but it is better than having a broken IM. And it should be rare, since I don't think we're shipping to countries that use the hebrew and arabic keyboards.

Comment 41

[Mihai Cirlanaru [:mihai][:mcirlanaru]]

(In reply to David Flanagan [:djf] from comment #40)
> Mihai,
> 
> Thanks for catching those other bugs.  I've fixed them but am asking for
> another round of review because this bug has proven so challenging.  My fix
> to switching between layouts that use different IMs is to file a new bug,
> and in the meantime, force the keyboard closed when that happens so that the
> user has to refocus the input field, which will allow the new im to be
> properly initialized. Its bad UX but it is better than having a broken IM.
> And it should be rare, since I don't think we're shipping to countries that
> use the hebrew and arabic keyboards.

I think forcing the keyboard closed for that special case is the best approach to solving the problem, thanks for adding the logic for this.

Unfortunately, I found another edge case that fails with auto-correction (see my comment here: https://github.com/mozilla-b2g/gaia/pull/10620#discussion_r4950737): when the user switches from a kb with dict to one without (both using the latin IM), with a word partially typed, typing space auto-corrects the word, although the current layout doesn't have a dict -- sorry I didn't notice this in my previous rounds of testing.

Ping me once that special case is sorted and the patch rebased on master (it conflicts with the one for bug 796600 for now).

Comment 42

[David Flanagan [:djf]]

Mihai,

Thank you again for finding that bug. I've updated the PR and have fixed it by adding a line to terminateWorker().

While fixing the merge conflict with bug796000, I found a bug where pressing the switch layout button and holding it until the menu appears and then releasing without selecting a keyboard would switch to the next keyboard. That wasn't supposed to happen, so I've made a couple of small changes to switchKeyboard() and to the code that calls switchKeyboard() so that it does not get called in that case.

I start vacation on July 3rd. If you give r+ after then, please land the patch yourself (or find someone to).  Or, if you find more bugs and give r-, then please assign the bug to yourself.

Thank you for all your help on this bug, both writing and reviewing patches for it.

Comment 43

[Mihai Cirlanaru [:mihai][:mcirlanaru]]

Hi David, it works perfectly now! Thanks for all the work and diligence in spotting all tiny issues that appeared on the way.

Comment 44

[Mihai Cirlanaru [:mihai][:mcirlanaru]]

Comment on attachment 767477 [details]
link to patch on github

Marking the patch with r+, feel free to land on master.

Comment 45

[David Flanagan [:djf]]

Landed on master: https://github.com/mozilla-b2g/gaia/commit/e9027411eb0863e15f1f81fa6f74cc1a043fe85c

Comment 46

[David Flanagan [:djf]]

uplifted to v1-train: https://github.com/mozilla-b2g/gaia/commit/2c40ccec96271ea9f34aa6fbdfc6563a540548b3

Comment 47

[William Hsu [:whsu]]

Hi, all,

Thanks for your help.
I verified the patches of Gaia master and v1-train.
Now, word suggestion follows the keyboard layout to show the suggested wording.

* Test Build:(Mozilla-central-unagi-eng/2013-07-02-15-30-59)
 + Mercurial-Information:
   - Gecko: "c193fdeb4932"
   - Gaia: ""
 + Git-Information:
   - Gecko: "8dd0b9e58b24fce70a34c1935516ad49a15eb6bf"
   - Gaia: "5bf9d534c17567e54e3d5b71af5065a78f95865b"

* Test Build:(mozilla-b2g18-unagi-eng/2013-07-02-23-02-06)
 + Mercurial-Information:
   - Gecko: "a078412b1671"
   - Gaia: ""
 + Git-Information:
   - Gecko: "38e6b257a832fbd6c91fac54bf8a8c5d3572e3fb"
   - Gaia: "a890df3624df350574cb942d13711e1d858118d3"

Also, attaching the screenshots.

Comment 48

[William Hsu [:whsu]]

Attachment: Word suggestion follows the keyboard layout (Gaia master)

Comment 49

[William Hsu [:whsu]]

Attachment: Word suggestion follows the keyboard layout (v-train)

Comment 50

[John Ford [:jhford] CET/CEST Berlin Time]

v1.1.0hd: 2c40ccec96271ea9f34aa6fbdfc6563a540548b3
v1.1.0hd: 877976ebbca04ed9772ac2749434d7360651b3ee

Comment 51

[William Hsu [:whsu]]

Verified it on V1.1.0 HD.

* Test build:(Mozilla-b2g18_v1_1_0_hd-helix/2013-08-05-23-22-02)
  + Mercurial-Information
    - Gecko revision="f0e9fcb591c2"
  + Git-information
    - Gaia revision="1788602b9e6bfdd6c91f46e1d72b0ed134e05b41"