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

RESOLVED FIXED in 2.1 S4 (12sep)

Status

defect
P3
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: timdream, Assigned: mnjul)

Tracking

({perf})

unspecified
2.1 S4 (12sep)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [c=progress p= s= u=] [win=10ms?])

Attachments

(1 attachment)

+++ 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.
Whiteboard: [win=10ms?]
The first |setLayoutFrameActive| call happens at KeyboardManager.launchLayoutFrame() line 390, and the second one at setKeyboardToShow() at 547.
Keywords: perf
Priority: P1 → P3
Whiteboard: [win=10ms?] → [c=progress p= s= u=] [win=10ms?]
Posted file 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.
Assignee: nobody → jlu
Comment on attachment 8480470 [details]
Patch (PR @ GitHub)

TBPL is happy with the patch. Tim, could you check? Thanks.
Attachment #8480470 - Attachment description: Proposed Patch (PR @ GitHub) → Patch (PR @ GitHub)
Attachment #8480470 - Flags: review?(timdream)
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.
Attachment #8480470 - Flags: review?(timdream)
(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.
Flags: needinfo?(jlu)
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.
Attachment #8480470 - Flags: review?(timdream)
Flags: needinfo?(jlu)
Attachment #8480470 - Flags: review?(timdream) → review+
Final testing before landing: https://tbpl.mozilla.org/?rev=b332b4bd4bfa7bda5e6ea070f807ffe56a44cd22&tree=Gaia-Try
Target Milestone: --- → 2.1 S4 (12sep)
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.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.