1005752 - [Keyboard] Should not setLayoutFrameActive(false) if we don't need to

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System::Input Mgmt, defect, P3)

Description

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

+++ This bug was initially created as a clone of Bug #970188 +++

Currently keyboard_manager.js launches keyboard in background _then_ try to figure out if it should be at the foreground. It should be smarter about it.

Comment 1

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

The first |setLayoutFrameActive| call happens at KeyboardManager.launchLayoutFrame() line 390, and the second one at setKeyboardToShow() at 547.

Comment 2

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

Attachment: Patch (PR @ GitHub)

Well, this actually is easier than it might have appeared. The code in question is [1], in |IFM.launchFrame| function. |IFM.launchFrame| is only called in |KM.setKeyboardToShow| [2]. In |KM.setKeyboardToShow|, we either call |KM.resetShowingKeyboard| which would call |IFM.resetFrame|, or call |IFM.setupFrame| -- both these two scenarios will re/set what [1] did. So in this patch the code in question is simply removed.

[1] https://github.com/mozilla-b2g/gaia/blob/03d26d72f1235796e1e60ebb9e6c00da9421e80f/apps/system/js/input_frame_manager.js#L101

[2] https://github.com/mozilla-b2g/gaia/blob/03d26d72f1235796e1e60ebb9e6c00da9421e80f/apps/system/js/keyboard_manager.js#L457

Things seem to behave normally with the patch, but I'll get it tested before asking for review.

Comment 3

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

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

TBPL is happy with the patch. Tim, could you check? Thanks.

Comment 4

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

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

Would you mind doing more test on this? I know the keyboard frame will not receive inputcontext until we do frame.setInputMethodActive(true), i.e. the input method is launched with inactive, but AFAIK frames are launched with visible state (in terms of visibility controlled by setActive()).

Please make sure frame are launched in hidden state and IM-inactive state when the phone boot-up.

Comment 5

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

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #4)
> Comment on attachment 8480470 [details]
> Patch (PR @ GitHub)
> 
> Would you mind doing more test on this? I know the keyboard frame will not
> receive inputcontext until we do frame.setInputMethodActive(true), i.e. the
> input method is launched with inactive, but AFAIK frames are launched with
> visible state (in terms of visibility controlled by setActive()).
> 
> Please make sure frame are launched in hidden state and IM-inactive state
> when the phone boot-up.

Makes sense. I will do that. Setting NI on myself for reminder.

Comment 6

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

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

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #4)
> Comment on attachment 8480470 [details]
> Patch (PR @ GitHub)
> 
> Would you mind doing more test on this? I know the keyboard frame will not
> receive inputcontext until we do frame.setInputMethodActive(true), i.e. the
> input method is launched with inactive, but AFAIK frames are launched with
> visible state (in terms of visibility controlled by setActive()).
> 
> Please make sure frame are launched in hidden state and IM-inactive state
> when the phone boot-up.

So, I checked and frames are indeed visible when just appendChild()'ed at https://github.com/mnjul/gaia/blob/561f053a8eb8da05d6c8432f35e01db36b55d1fd/apps/system/js/input_frame_manager.js#L115 . 

While I don't actually think it's a problem with the *current* codes since we'd eventually either |setFrameActive(false)| or |setFrameActive(true)| as I said in comment 2, I believe it's much better to have stronger contract that the inner functions of IFM would guarantee constructed frames always be hidden if they're not to be showed/activated. Patch has been modified accordingly.

Comment 7

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

Final testing before landing: https://tbpl.mozilla.org/?rev=b332b4bd4bfa7bda5e6ea070f807ffe56a44cd22&tree=Gaia-Try

Comment 8

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

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

Patch was landed despite Gij failures in calendar app; the failures are there in other PRs too.