Closed Bug 1075306 Opened 9 years ago Closed 8 years ago

Implement InputWindow

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
2.1 S8 (7Nov)

People

(Reporter: mnjul, Assigned: mnjul)

References

Details

(Whiteboard: [p=8])

Attachments

(2 files, 1 obsolete file)

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

In this bug let's implement InputWindow. This will move things from InputFrameManager and possibly allow AppTransitionController to assume the role of  InputAppTransitionManager.

For the scope of this bug, I intend to keep KeyboardManager intact and also still retain InputFrameManager (but probably for its exposed interface to KeyboardManager; its inner workings regarding the actual keyboard frame should be delegated to the implemented InputWindow).
Assignee: nobody → jlu
Whiteboard: [p=3]
Target Milestone: --- → 2.1 S6 (10oct)
Attached file WIP Patch v1 (PR @ GitHub) (obsolete) —
Alright, here is the working WIP.

- We now have InputWindow, inheriting from AppWindow. As what you'd expect with an AppWindow, one InputWindow instance holds one input app, just like before.

- InputFrameManager no longer creates <iframe> elements by itself; it now instantiates InputWindows and book-keeps them.

- InputAppTransitionManager has been eliminated, in favor of the already-built AppTransitionController. State transition has been bridged and performs correctly.

- 3rd-party keyboard switching works correctly. (Tested LOL keyboard and Latex keyboard)

Some issues not yet tackled:

- Tests

- Clearer comments for InputWindow members

- Nasty coupledness between KeyboardManager and InputFrameManager.
-- For example, IFM probably should be knowing what the active InputWindow is, to avoid having KeyboardManager relay the information every time.
-- In a follow-up bug, I will redo KeyboardManager and InputWindowManager (probably resulting in InputManager and InputWindowManager).

Something that requires extra note:
In the past, we had only one keyboard frame container; when we switched from one input app to another input app (i.e. different iframes), we did not hide and re-show the keyboard frame container -- we just hid (and not slide-down) the old iframe and showed (and not slide-up) the new iframe. (So it was possible you see a black area during the switching -- when the new input app was being loaded/launched).
Yet, now that we have many containers, one for each InputWindow, whose transitions are controlled by AppTransitionController, such hiding-showing -- and retaining the black area -- would be rather impossible. As such, I have reconstructed the flow, which is unfortunately a bit opaque:
- I hide the old InputWindow (using |AppWindow.close()| with the |immediate| class, so it's not a slide-down).
- When the old frame is hidden (as |closed| event is triggered), a callback to KeyboardManager is done, and KeyboardManager shows/launches the new InputWindow. The callback is book-kept as |_showedCallbacks| in InputFrameManager, but if we feel like so, it can also be a property of each InputWindow instance (but I don't like the coupledness it sounds).
- At this time, KeyboardManager indicates that the new InputWindow should show without animation (the |showImmediately| parameter propagating into the InputWindow instance, and becoming |inputWindow.openImmediately|. When the input app is ready and we receive mozbrowserresize event, we see |inputWindow.openImmediately| and use |AppWindow.open()| with the |immediate| class, so animation is skipped.


Anyway, an end user should not observe any changes from this patch as this is a refactoring work.

Tim and Alive, could you check? Thanks.
Attachment #8502357 - Flags: feedback?(timdream)
Attachment #8502357 - Flags: feedback?(rlu)
Attachment #8502357 - Flags: feedback?(alive)
Whiteboard: [p=3] → [p=5]
Target Milestone: 2.1 S6 (10oct) → 2.1 S7 (Oct24)
(In reply to John Lu [:mnjul] [MoCoTPE] from comment #1)
> Created attachment 8502357 [details] [review]
> WIP Patch v1 (PR @ GitHub)
> Some issues not yet tackled:
> 
> - Tests
> 
> - Clearer comments for InputWindow members
> 
> - Nasty coupledness between KeyboardManager and InputFrameManager.
> -- For example, IFM probably should be knowing what the active InputWindow
> is, to avoid having KeyboardManager relay the information every time.
> -- In a follow-up bug, I will redo KeyboardManager and InputWindowManager
> (probably resulting in InputManager and InputWindowManager).

IMO there will be only one Manager. I don't really care the naming.
The responsibility of the only one InputWindowManager is to
* Create the InputWindow
* Maintain a list of InputWindow and provide the active InputWindow info
* Provide the height of the active InputWindow
* Dispatch InputEvent to correct InputWindow (if needed)
Otherwise all mozInputMethod API manipulation should be done in InputWindow or InputWindow's sub module.

> 
> Something that requires extra note:
> In the past, we had only one keyboard frame container; when we switched from
> one input app to another input app (i.e. different iframes), we did not hide
> and re-show the keyboard frame container -- we just hid (and not slide-down)
> the old iframe and showed (and not slide-up) the new iframe. (So it was
> possible you see a black area during the switching -- when the new input app
> was being loaded/launched).
> Yet, now that we have many containers, one for each InputWindow, whose
> transitions are controlled by AppTransitionController, such hiding-showing
> -- and retaining the black area -- would be rather impossible.

We could. Just like all AppWindow.element are living in #window,s all InputWindow.element should reside in #keyboard(renaming?)
and we could paint the #keyboard background color as whatever you want.

Haven't read your patch though.
Comment on attachment 8502357 [details] [review]
WIP Patch v1 (PR @ GitHub)

Thank you for the heavy lifting. Since I am not familiar with AppWindow etc, I shouldn't get to review this. Just take my feedback+ and have Alive (or other peers) to review it.
Attachment #8502357 - Flags: feedback?(timdream) → feedback+
Comment on attachment 8502357 [details] [review]
WIP Patch v1 (PR @ GitHub)

We had an offline meeting to make sure what's the final module responsibility between InputWindowManager/InputWindow/InputWindowFactory. Also the inputWindow switch mechanism to simplify the effort of InputWindowManager.

John would update later if he comes up with a migrating plan to achieve that final goal.
Attachment #8502357 - Flags: feedback?(alive) → feedback+
Attachment #8502357 - Flags: feedback?(rlu)
Alive and I had a (somewhat massive-scale) discussion on not only the implementation of the InputWindow but input management as a whole, and the mechanism of some other system-wide events related to it:

So here’s some responsibilities we’ll be having:
1. Load and parse installed keyboard layouts
2. Respond to system-wide inbound events that we have to:
-a. Switch keyboards
-b. Show keyboards
-c. Hide keyboards
-d. Kill keyboards
3. InputWindow management & inner working
-a. instantiation, including basic setup
-b. showing, including setVisible(true), setInputMethodActive(true)
-c. hiding, including setVisible(false), setInputMethodActive(false)
-d. switching to another keyboard app (like what I said in comment 1, we need to hide and show two different InputWindows consecutively without animation) 
-e. switching to another layout within the same keyboard app (by merely changing browser element’s URL hash)
4. InputWindow broadcasting events to the input management system; and subsequently to the outside world (i.e. other system modules)

I’m planning to have these module mapped to the responsibilities (subject to changes, of course):

Modules: KeyboardManager (to be renamed to InputManager), InputFrameManager (to be renamed from InputWindowManager), InputWindow ; note that KeyboardManager already has a submodule InputLayouts that deals with chores of installed keyboards/layouts.

> 1. Load and parse installed keyboard layouts
KeyboardManager with the sub module InputLayouts

> 2. Respond to system-wide inbound events that we have to:
> -a. Switch keyboards
> -b. Show keyboards
> -c. Hide keyboards
> -d. Kill keyboards
KeyboardManager, and see 3a below

> 3. InputWindow management & inner working
> -a. instantiation, including basic setup
KeyboardManager will create InputWindows if it’s not created yet; It will ask InputFrameManager to “launch” the InputWindow.

> -b. showing, including setVisible(true), setInputMethodActive(true)
InputFrameManager <-> InputWindow; and this is the most tricky part, see [*]

> -c. hiding, including setVisible(false), setInputMethodActive(false)
InputFrameManager <-> InputWindow;

> -d. switching to another keyboard app (like what I said in comment 1, we need to hide and show two different InputWindows consecutively without animation) 
KeyboardManager tells InputFrameManager about this and it adjusts the launch/close callbacks for the two InputWindows.

> -e. switching to another layout within the same keyboard app (by merely changing browser element’s URL hash)
KeyboardManager tells InputFrameManager about this and it adjusts the showing logics.

> 4. InputWindow broadcasting events to the input management system; and subsequently to the outside world (i.e. other system modules)
InputWindow -> InputFrameManager.

My planned key working items in this bug would still mainly be about InputWindow itself and its relation to the input management inner (i.e. I’ll not be touching things like outside world event interface).
- [*] The most tricky part is that we currently use |mozbrowserresize| event (triggered by keyboard app’s resizeTo()) to be told of both situations that 1) keyboard is ready and 2) keyboard would like to resize itself.
-- The current sequence is when we’d like to “launch” a keyboard, we instantiate the app’s iframe, setVisible(true) and setInputMethodActive(true) on the keyboard app frame, and wait until we receive the first |mozbrowserresize| call where we can start to show the keyboard app frame as it is ready.
-- Hwoever, we may receive |mozbrowserresize| events whenever keyboard app decides to change its size. So, with the past and current codes we have to do some if-else on mozbrowserresize event handlers. Alive has proposed some mechanism to make the flow more streamlined.
- I’ll try to streamline the calls for activating and deactivating keyboards. As we all see now, currently when we are to activate a keyboard, we activateKeyboard -> launchFrame -> constructFrame or reuseFrame -> setKeyboardToShow -> setupFrame, which is really PITA (I’m not even detailing those parameters and callbacks!). It would be awesome if we could abstract and streamline these things with already-established verbs such as AppWindow.ready().

Please note that it is my intended scope for this bug that I’ll retain KeyboardManager and InputFrameManager and their exposed interface: received/sent events from/to the outside world will remain the same. And yes, I assume that I will leave a somewhat messy KeyboardManager and InputFrameManager when I finish this bug (and get the follow-up bugs defined then).

In later bugs (to be tracked by bug 1074749) I’ll gradually make InputFrameManager and KeyboardManager more coherent to the responsibilities and module-responsibility mapping above.
Oh and, a final diagram will be drawn when… well, things are final and landed ;)
CC greg as he's doing bug 1079706
Alright, I have re-architected the patch. Notably the flow of showing/hiding InputWindows has been streamlined more.

Alive, please check if this looks closer to what you expect.

Tim & Rudy, please feel free to check out the patch. I'd like to ask you to especially check the summary comments on the beginning of InputFrameManager (which we'll change to InputWindowManager in the future): https://github.com/mozilla-b2g/gaia/pull/25249/files#diff-0e82454d6baf3617a9b14747c412e2d1 to see if it's clear enough.

Note I have largely changed the InputWindow bookkeeping logics. For these layouts:
'layoutEn': { manifestURL: 'app://App1', id: 'en', path: 'index.html#en' }
'layoutFr': { manifestURL: 'app://App1', id: 'fr', path: 'index.html#fr' }
'layoutEs': { manifestURL: 'app://App1', id: 'es', path: 'index2.html#es' }
'layoutDe': { manifestURL: 'app://App1', id: 'de', path: 'index2.html#de' }
'layoutZhu': { manifestURL: 'app://App2', id: 'zhu', path: 'index.html#zhu' }
'layoutPin': { manifestURL: 'app://App2', id: 'pin', path: 'index.html#pin' }
'layoutJp': { manifestURL: 'app://App2', id: 'jp', path: 'indexKana.html#jp' }

We used to keep this kinda of bookkeeping structure (suppose En and Zhu was the last active layout of App1 and App2 respectively, and Es & Jp have been launched before):

{
  'app://App1': {
    'en': |InputWindow|,
    'fr': undefined,      // actually it's a deleted property
    'es': |InputWindow|,
    'de': undefined       // actually it's a deleted property
  },
  'app://App2': {
    'zhu': |InputWindow|,
    'pin': undefined,     // actually it's a deleted property
    'jp': |InputWindow|
  }
}

With my patch, I'm changing the structure to:

{
  'app://App1': {
    'index.html': |InputWindow|,
    'index2.html': |InputWindow|
  },
  'app://App2': {
    'index.html': |InputWindow|
    'indexKana': |InputWindow|
  }
}

I assume the new structure (and its associated logics) better reflect the fact that we reuse an InputWindow when manifestURLs match and paths only differ in the hash part, as in [1].

(And Alive: earlier this week I said we reuse InputWindow as long as they're the same app (same manifestURLs) and that *was wrong*. We reuse InputWindow if they're the same app AND the path are the same except for the hash part.)

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/input_frame_manager.js#L147-L164
Attachment #8502357 - Attachment is obsolete: true
Attachment #8506740 - Flags: feedback?(timdream)
Attachment #8506740 - Flags: feedback?(rlu)
Attachment #8506740 - Flags: feedback?(alive)
Comment on attachment 8506740 [details] [review]
Patch v3 with tests (PR @ GitHub)

What you do is a good improvement. I think we need to create some Gij tests to assert the behavior of input mgmt?
Attachment #8506740 - Flags: feedback?(timdream) → feedback+
Comment on attachment 8506740 [details] [review]
Patch v3 with tests (PR @ GitHub)

Much better than the previous one; need one more round and we could finish it I think.
Attachment #8506740 - Flags: feedback?(alive) → feedback+
Comment on attachment 8506740 [details] [review]
Patch v3 with tests (PR @ GitHub)

Alright, here we go again. It does seem that we have a few occasions that KeyboardManager needs to know what input apps have been loaded by InputFrameManager, and let’s see how that can be resolved in a follow-up bug.

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #9)
> What you do is a good improvement. I think we need to create some Gij tests
> to assert the behavior of input mgmt?

Say we create a follow-up bug and let bug 928805 track it? What do you think?
Flags: needinfo?(timdream)
Attachment #8506740 - Flags: feedback+ → feedback?(alive)
(In reply to John Lu [:mnjul] [MoCoTPE] from comment #11)
> (In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from
> comment #9)
> > What you do is a good improvement. I think we need to create some Gij tests
> > to assert the behavior of input mgmt?
> 
> Say we create a follow-up bug and let bug 928805 track it? What do you think?

Yeah that's fine, as long as we don't regress.
Flags: needinfo?(timdream)
Comment on attachment 8506740 [details] [review]
Patch v3 with tests (PR @ GitHub)

See you in review!
Attachment #8506740 - Flags: feedback?(alive) → feedback+
Comment on attachment 8506740 [details] [review]
Patch v3 with tests (PR @ GitHub)

Looks good.
I found that with this change, dragging down the utility would dismiss the keyboard. 
We should address that before landing.

Thanks.
Attachment #8506740 - Flags: feedback?(rlu) → feedback+
(In reply to Rudy Lu [:rudyl] from comment #14)
> I found that with this change, dragging down the utility would dismiss the
> keyboard. 

Sounds bad...I'll look into that.
FWIW, we have an integration test to cover this case,
https://github.com/mozilla-b2g/gaia/blob/0f8a3e2300add6fbd20bbe2df7e082565e039dd9/apps/keyboard/test/marionette/launch_test.js#L109

but seems the test case is not valid anymore or something so that it did not catch this.
Quick sitrep: issue described in comment 14 has been fixed. I have been trying to fix python integration tests (Gip), which unfortunately was more than merely fixing selectors -- I probably have altered some keyboard-activation/deactivation behavior that the integration tests relied on (though I thought my patch  came with no apparent real user-facing changes).

Will keep you posted.
Quick sitrep:

Gip issues were fixed. Gij issues were mostly fixed except for one related to SMS/MMS composition, which I'm still tracking (it could be that my implementation in this patch regresses some behavior)

My temp PR for all those testing is at https://github.com/mozilla-b2g/gaia/pull/25480, in case anyone is interested.
Comment on attachment 8506740 [details] [review]
Patch v3 with tests (PR @ GitHub)

Alright, all the unit/integration tests have been fixed/modified/added/removed as should be.

(Most of the review requests are for tests, except for that of Alive)

Zac, please check the modified python UI test -- we changed the selectors for the keyboard frame in this patch and that's (only) what you'll see there.

Rudy & Tim, please check if the modified marionette-JS tests for keyboard are good. Do note that I slightly changed long-press latency from 2.0s to 1.0s in [1] as I found that the old latency gave some intermittent test errors ('release' fired onto marionette server would sometimes result in NS_ERROR_ILLEGAL_VALUE from Gecko). For Tim, please also check if KeyboardManager unit tests still cover things reasonably -- note that a lot of keyboard switching and transition tests have been eliminated: test for switching is now done at InputFrameManager's testing (as it's now its job) and there is no need to separately test for transition as I'm now using the System-app-standard AppTransitionController.

(and of course, Rudy and Tim, please feel free to check the unit tests for InputFrameManager/InputWindows too)

I understand that KeyboardManager's unit tests have been messy -- and still are -- but as we'll step into refactoring the roles of  KeyboardManager & InputFrameManager (to be renamed to InputWindowManager, again), I believe we shall keep the changes to KM tests as minimal as possible in this patch.

Now, Alive -- aside from the issues below -- I modified statusbar.js [2] to add an extra guard against undefined property access, as they were giving me Gij errors [3] that I couldn't reproduce locally despite various efforts. And you may want to know about some issues of my patch I encountered when I was fixing tests (uh...to be clear: the issues are fixed in the PR you're reviewing right now):
1. InputWindow launched in-process stole focus and resulted in behavioral errors (such as that input field would lose focus and would lead keyboard to close). I decided to use |ignoreuserfocus| regardless of InputWindow's in/out-of-process in BrowserFrame [4]. Alternatively we can do that in AppTransitionController._shouldFocusApp() [5], but I think my current way is more centralized. (Note: before this patch, we never explicitly focused any in-process Keyboard frame).
2. If somebody calls InputWindow.close() immediately after it calls InputWindow.open() (and before |ready| triggers), we need to ignore that |ready| when it occurs later. Thus, now, in InputWindow.close(), |ready| event listener is removed. As |ready| handler now has to be known to more than one methods, I have to extract it out. This scenario happens in integration tests a lot.

An inter-diff between v2 and v3, of application codes only (i.e. excluding test codes) is available at attachment 8514133 [details] [diff] [review] for anyone's reference (but I'm not exactly sure if I'm doing the interdiff right, so for formal review comments please use the GitHub PR).

[1] https://github.com/mozilla-b2g/gaia/pull/25249/files#diff-1
[2] https://github.com/mozilla-b2g/gaia/pull/25249/files#diff-9
[3] https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=69c90449a1c1
[4] https://github.com/mozilla-b2g/gaia/pull/25249/files#diff-ea9afc22ac8fb7d2d2791b9bf2a6e259R73
[5] https://github.com/mnjul/gaia/blob/bug_1075306_input_window_stage3/apps/system/js/app_transition_controller.js#L287-L294
Attachment #8506740 - Attachment description: WIP Patch v2 (PR @ GitHub) → Patch v3 with tests (PR @ GitHub)
Attachment #8506740 - Flags: review?(zcampbell)
Attachment #8506740 - Flags: review?(timdream)
Attachment #8506740 - Flags: review?(rlu)
Attachment #8506740 - Flags: review?(alive)
Comment on attachment 8506740 [details] [review]
Patch v3 with tests (PR @ GitHub)

r+, tested it on device and it works fine. Thanks for working on the tests :)
Attachment #8506740 - Flags: review?(zcampbell) → review+
Comment on attachment 8506740 [details] [review]
Patch v3 with tests (PR @ GitHub)

I didn't read through old keyboard_manager.js and the part deal with layout; r+ for other parts. Great done!

Note: still need to rule out inputWindow to lock/set orientation;
as well as we need to handle mozbrowsererror inside inputWindow.
Attachment #8506740 - Flags: review?(alive) → review+
Attachment #8506740 - Flags: review?(timdream) → review+
Comment on attachment 8506740 [details] [review]
Patch v3 with tests (PR @ GitHub)

Looks good to me for the Gij and keyboard manager part.
Thanks.
Attachment #8506740 - Flags: review?(rlu) → review+
Here we are: https://github.com/mozilla-b2g/gaia/commit/82c3899a7fd73cb3d7c406696c67f12ed461547c
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [p=5] → [p=8]
Target Milestone: 2.1 S7 (24Oct) → 2.1 S8 (7Nov)
Depends on: 1094122
You need to log in before you can comment on or make changes to this bug.