1031505 - Using a different keyboard layout type than English in messaging enables capslocks

Status: VERIFIED FIXED

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

Description

[Delphine Lebédel [:delphine - Use needinfo]]

Tested on Flame device, on today's 2.0 and Master builds (affected).
Also checked on Buri 1.4 (not affected)

STRs:
* Device is in English, with English qwerty keyboard by default
* start typing a message
* go to settings and enable another built-in keyboard with different layout, in my case French azerty
* go back to message
* continue typing your message

Actual result: 
Capslock becomes enabled for the rest of the message. Impossible to go back to normal. Even in further messages, this continues.

Expected: keyboard continues to function normally, without capslocks automatically enabling itself throughout the message

This is a regression, thus nominating

Comment 1

[Delphine Lebédel [:delphine - Use needinfo]]

Forgot one step...
Step 4bis: you need to click on the Icon on the keyboard to actually switch keyboard layout for the given message

Comment 2

[Johan Lorenzo [:jlorenzo]]

Actually, there is no need to execute step 3, if you already have the French keyboard. In other words, anybody who has more than one keyboard layout is affected if he switches in the middle of a text.

Comment 3

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

I could reproduce this with v2.1, will try to find out the root cause and provide a fix,
===
Gaia      b3324d031fe91b864090461ffcacc6ca605a2903
Gecko     https://hg.mozilla.org/mozilla-central/rev/afa67a2f7905
BuildID   20140629160201
Version   33.0a1
ro.build.version.incremental=eng.cltbld.20140623.194406
ro.build.date=Mon Jun 23 19:44:23 EDT 2014

Comment 4

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

Tried to bisect on this, it seems that it is bug 1016283 that caused this issue.

Comment 5

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

But I could not reproduce this issue on v2.0, and this is reasonable because the offending patch in bug bug 1016283 was never uplifted to v2.0.

Delphine, could you please help double confirm this won't happen on v2.0?
Thank you.

Comment 6

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

Confirmed that reverting https://github.com/mozilla-b2g/gaia/commit/c0d1670ba57666210a7e7fd021ca4d9b47ab990c solves this issue. Tim and I discussed and the patch will be backed out from master due to this bug being 2.0+.

I will try to solve the issue within Tim's patch though, as I plan to dive into Input Management in the future.

Comment 7

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

Patch for bug 1016283 was backed out from master, anyone to confirm this issue gets resolved?

Comment 8

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

Closing this bug for now, since the patch in bug 1016283 has been backed out, see https://bugzilla.mozilla.org/show_bug.cgi?id=1016283#c14

I'm leaving the status-b2g-2.0 as ?, since I'd like Delphine's help to take a look at Comment 5.
Thanks.

Comment 9

[Delphine Lebédel [:delphine - Use needinfo]]

Theo and I have tried reproducing it on today's 2.0 on Flame, and indeed, we cannot reproduce on 2.0

Comment 10

[Tim Guan-tin Chien [:timdream] (please needinfo)]

So this is not a 2.0+ after all. After talking to John offline we decided to re-land bug 1016283 on master and fix the regression here.

Comment 11

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Should be 2.1? and 2.1+ when triaged.

Comment 12

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

Attachment: Patch (PR @ GitHub)

Here is the patch (tests are being run as of writing).

The cause of the problem (KB = "keyboard"):

1. Without Tim's patch of bug 1016283, resetShowingKeyboard() was always called in switchToNext() in https://github.com/mnjul/gaia/blob/1412afd61342da6a0517c7bd2e5e22ef57715517/apps/system/js/keyboard_manager.js#L619 . Subsequently it would call KB frame's setInputMethodActive(false) in setLayoutFrameActive in https://github.com/mnjul/gaia/blob/1412afd61342da6a0517c7bd2e5e22ef57715517/apps/system/js/keyboard_manager.js#L688. Then, in switchToNext(), when setKeyboardToShow() is called, it will call KB frame's setInputMethodActive(true). This triggers the inputcontextchange event in KB app. Additionally, the hashchange event in KB app is also triggered per the "change layout" mechanism.

2. In Tim's patch, call to resetShowingKeyboard() in the above function was removed if the next layout was from the same KB app. The inputcontextchange event is not triggered in KB app, since there is no activeness flipping resulting from setInputMethodActive() calls.

3. In the KB app, only in the inputcontextchange event is inputContext.getText() called (https://github.com/mnjul/gaia/blob/1412afd61342da6a0517c7bd2e5e22ef57715517/apps/keyboard/js/keyboard.js#L313). In the hashchange event, it is not called. Thus, since with Tim's patch only the hashchange event (and not inputcontextchange event) is dispatched to KB app, the KB app does not have a chance to get the up-to-date value of the input field; also, since the inputContextGetTextPromise from prior call is not deleted after use, the subsequent activation of another layout would retrieve the old value in the input field (old = last occurrence of inputcontextchange).
The data path is from switchIMEngine() (https://github.com/mnjul/gaia/blob/1412afd61342da6a0517c7bd2e5e22ef57715517/apps/keyboard/js/keyboard.js#L1210 into https://github.com/mnjul/gaia/blob/1412afd61342da6a0517c7bd2e5e22ef57715517/apps/keyboard/js/keyboard.js#L1223) to InputMethodManager.switchCurrentIMEngine() (https://github.com/mnjul/gaia/blob/1412afd61342da6a0517c7bd2e5e22ef57715517/apps/keyboard/js/keyboard/input_method_manager.js#L330) to latin IME's activation (https://github.com/mnjul/gaia/blob/1412afd61342da6a0517c7bd2e5e22ef57715517/apps/keyboard/js/imes/latin/latin.js#L188)

4. Now, from the data path and the latin IME's activate() illustrated in 3, note that while the retrieved text is stale, the "cursor" variable is up-to-date, because the selectionStart variable is freshly-retrieved from inputContext when we're just about to switch layouts: https://github.com/mnjul/gaia/blob/1412afd61342da6a0517c7bd2e5e22ef57715517/apps/keyboard/js/keyboard.js#L1221 . With the STR in this bug[*], the condition stands that the value of cursor is larger than the length of the stale input field text, since we switch to another layout after we have typed something in the previous layout. Under this condition, in latin KB's determining whether the keyboard should automatically switch on capslock in https://github.com/mnjul/gaia/blob/1412afd61342da6a0517c7bd2e5e22ef57715517/apps/keyboard/js/imes/latin/latin.js#L914-L916 , isUpperCase() would be called with an empty string (as cursor > inputText.length). And isUpperCase() actually returns true for empty string: https://github.com/mnjul/gaia/blob/1412afd61342da6a0517c7bd2e5e22ef57715517/apps/keyboard/js/imes/latin/latin.js#L932-L937 . Thus the latin KB erroneously determines capslock should be on.

[*] For better illustration, if anyone wants to validate my claim, please type more than 2 letters before switching layouts or the bug would be harder to reproduce.

So in this patch, inputContext.getText() is modified to be also called in hashchange event: https://github.com/mnjul/gaia/blob/43009247d781daa9d690e4df9563ae678592c809/apps/keyboard/js/keyboard.js#L291-L293 . To avoid unnecessarily calling getText() under the scenario that both hashchange and inputcontextchange events are fired, now getText() is only called when the getText Promise is null/undefined; it is set to null when the promise would be used in switchIMEngine().

Comment 13

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

BTW, we don't seem to have unit test codes for keyboard.js of KB app, right?

Comment 14

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

Comment on attachment 8449218 [details] [review]
Patch (PR @ GitHub)

Tests are green. Tim & Rudy, could you review the patch for me? Thanks.

Comment 15

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8449218 [details] [review]
Patch (PR @ GitHub)

Looks good, thank you. Unfortunately this part of code is not covered in the test.

Comment 16

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

Here we ago.

Master: https://github.com/mozilla-b2g/gaia/commit/dd3ec3e1d93ecdacd40120532dbe23a784929882

Comment 17

[Delphine Lebédel [:delphine - Use needinfo]]

I have been able to reproduce this issue again today, on the latest master, Flame device. 
However this time I didn't even need to add the French keyboard layout because my device was already in French (but I don't think this is a different bug, is it? Which is why I've changed the title of the bug to reflect that).
Seems like this happens as long as you are using a different keyboard layout than the English one, whether switching it or using it from the start.

STRs:
* Have the device in French
* start writing a message (I tried both when the message was still contained in 1 total message space, and another time when it took up 2 message space)
* go in your message, and start adding words in it, taking some out, changing the spelling, until the cap lock issue I've already described happens (it shouldn't take too long)

re-opening

Comment 18

[Delphine Lebédel [:delphine - Use needinfo]]

Oops sorry, patch has landed, will therefore open a new bug for this

Comment 19

[Rachel Pribble [:rpribble]]

VERIFIED FIXED in v2.1.

Environmental Variables:
Device: Flame 2.1 KK (319 MB)
BuildID: 20141012001201 (full flash)
Gaia: d18e130216cd3960cd327179364d9f71e42debda
Gecko: 610ee0e6a776
Gonk: 52c909e821d107d414f851e267dedcd7aae2cebf
Version: 34.0a2 (2.1)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Keyboard functions as expected, no automatically enabled caps lock encountered.