Closed
Bug 1005752
Opened 9 years ago
Closed 9 years ago
[Keyboard] Should not setLayoutFrameActive(false) if we don't need to
Categories
(Firefox OS Graveyard :: Gaia::System::Input Mgmt, defect, P3)
Tracking
(Not tracked)
RESOLVED
FIXED
2.1 S4 (12sep)
People
(Reporter: timdream, Assigned: mnjul)
References
Details
(Keywords: perf, Whiteboard: [c=progress p= s= u=] [win=10ms?])
Attachments
(1 file)
+++ 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.
Reporter | ||
Updated•9 years ago
|
Whiteboard: [win=10ms?]
Reporter | ||
Comment 1•9 years ago
|
||
The first |setLayoutFrameActive| call happens at KeyboardManager.launchLayoutFrame() line 390, and the second one at setKeyboardToShow() at 547.
Updated•9 years ago
|
Assignee | ||
Comment 2•9 years ago
|
||
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
Assignee | ||
Comment 3•9 years ago
|
||
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)
Reporter | ||
Comment 4•9 years ago
|
||
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)
Assignee | ||
Comment 5•9 years ago
|
||
(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)
Assignee | ||
Comment 6•9 years ago
|
||
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)
Reporter | ||
Updated•9 years ago
|
Attachment #8480470 -
Flags: review?(timdream) → review+
Assignee | ||
Comment 7•9 years ago
|
||
Final testing before landing: https://tbpl.mozilla.org/?rev=b332b4bd4bfa7bda5e6ea070f807ffe56a44cd22&tree=Gaia-Try
Target Milestone: --- → 2.1 S4 (12sep)
Assignee | ||
Comment 8•9 years ago
|
||
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: 9 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•