Closed Bug 1162383 Opened 7 years ago Closed 7 years ago

Keyboard jumps from hiding when tapping between two inputs, w/ bug 1162360 applied

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: timdream, Assigned: timdream)

References

Details

Attachments

(1 file, 2 obsolete files)

STR:

1. Apply patch in bug 1162360
2. Go to Messages -> new message
3. Tap on To
4. Tap on Message

Expected:

1. Keyboard stay shown w/o transition (or with a non-visible transition)

Actual:

1. Keyboard transition out and then back
So after investigation Alive point me to AppTransitionController#requestOpen and AppTransitionController#requireClose.

The events resulting the transition are

1) First requireClose() changes the class names to "inputWindow top-most active transition-closing slide-to-bottom", in which there is a CSS animation in slide-to-bottom class.
2) During the animation, the requireOpen('immediate') call will change the classes to "inputWindow top-most active". The "active" class will move the frame to the opened position.

At first I thought slide-to-bottom and active was not set in the same function loop, but with the above result I can confirm they are set in the same requireOpen('immediate') function loop.

I now suspect this is a APZ/OMTA issue -- we abort the animation in the main thread but it did not happen visually. I will try to come up with a reduced test case for this.

==

The bottom line is, I can delay the first requireClose() call to ensure this does not happen too often, from InputWindowManager, but that just look too much like a workaround to me....
I can't seem to reproduce this with a reduced test case. Maybe we read back DOM computed styles during the transition? Or maybe the resizing of the foreground app cause the glitch.
So after overwriting |AppWindow.prototype._resize| I concluded the glitch I am seeing is the painting delay of the content process when the keyboard is being hidden (we resize the app to full size when the keyboard is hidden, and resize it back to half-size when the keyboard is transitioned back). It looks like the best way to avoid this glitch is to install an nextpaint monitor and only start the keyboard transition-out when the next paint completes. Of course, if the user happen to focus another input, the yet-to-happen transition need to be cancelled.
Comment on attachment 8603161 [details] [review]
[gaia] timdream:system-wait-nextpaint-before-hide-keyboard > mozilla-b2g:master

Per discussed, this patch is wired by introducing a waitUntil() in the detail object of the public keyboardhide and system-resize event. The waitUntil() naming convention comes from Service Worker spec.

I've grep'd the system code base to ensure I have patched all the related system-resize handlers, and also all impl of *Manager#resize, *Window#resize, and *Window#_resize. I don't think I've missed any overriding functions but you might have better way to verify that (other than grep).

The patch has been manually verified to produce the desired effect. If the code looks good I will follow up and start creating tests for them.

Thanks!
Attachment #8603161 - Flags: feedback?(alive)
Attachment #8603161 - Flags: feedback?(alive) → feedback+
Comment on attachment 8603161 [details] [review]
[gaia] timdream:system-wait-nextpaint-before-hide-keyboard > mozilla-b2g:master

Tests added -- unfortunately they are more complex than I want them to be ...

I've changed the impl of waitUntil() a little bit to ensure no one calls it out of the event handling loop.
Attachment #8603161 - Flags: review?(alive)
(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #3)
> So after overwriting |AppWindow.prototype._resize| I concluded the glitch I
> am seeing is the painting delay of the content process when the keyboard is
> being hidden (we resize the app to full size when the keyboard is hidden,
> and resize it back to half-size when the keyboard is transitioned back). It
> looks like the best way to avoid this glitch is to install an nextpaint
> monitor and only start the keyboard transition-out when the next paint
> completes. Of course, if the user happen to focus another input, the
> yet-to-happen transition need to be cancelled.

Note that while this indeed hide the glitch behind the keyboard frame, we still resize the app twice with bug 1162360 in the tree, if the patch-to-land does not change.
Comment on attachment 8603161 [details] [review]
[gaia] timdream:system-wait-nextpaint-before-hide-keyboard > mozilla-b2g:master

r=me, thanks!
Attachment #8603161 - Flags: review?(alive) → review+
http://docs.taskcluster.net/tools/task-graph-inspector/#RU1hsYdsRAi-xZh5e-2pEA

The pull request failed to pass integration tests. It could not be landed, please try again.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
So something a bit concerning is that the tests passed according to autolander, but this did break Gij12 as soon as it landed. It's late in Taipei, so I'm going to just try to submit a hotfix to see if it fixes it, if not we can do a revert.

Hotfix: https://github.com/mozilla-b2g/gaia/commit/23b474eec73690fc4c0cdc7c1260b4308687d51b
Failure: https://treeherder.mozilla.org/logviewer.html#?job_id=222681&repo=gaia-master
Tim, can we back this out since it is causing a smoketest blocker bug 1164357?
QA Whiteboard: [QAnalyst-Triage+]
Whiteboard: [backout-asap]
Whiteboard: [backout-asap]
The cause of bug 1164357 is because when the user hits send we will get a focus and a blur event, and the patch did not handle the last blur event correctly.
[Keyboard Manager] get focus event textarea keyboard_manager.js:61:7
[Keyboard Manager] set layout to display: group=text index=0 keyboard_manager.js:61:7
[AppWindow][Messages][AppWindow_5][3530.817] publishing internal event: inputmethod-contextchange app_window.js:1201:1
[AppWindow][Messages][AppWindow_5][3530.822] handling _inputmethod-contextchange app_window.js:1201:1
[Keyboard Manager] get blur event keyboard_manager.js:61:7
[AppWindow][Messages][AppWindow_5][3531.080] request RESIZE...active? ,true app_window.js:1201:1
[AppWindow][Messages][AppWindow_5][3531.089]  will resize...  app_window.js:1201:1
[AppWindow][Messages][AppWindow_5][3531.107] force RESIZE... app_window.js:1201:1
[AppWindow][Messages][AppWindow_5][3531.111] publishing internal event: withoutkeyboard app_window.js:1201:1
[AppWindow][Messages][AppWindow_5][3531.117] publishing internal event: resize app_window.js:1201:1
[AppWindow][Messages][AppWindow_5][3531.121] publishing external event: resize app_window.js:1201:1
[AppWindow][Messages][AppWindow_5][3531.134] W:,320,H:,569.3333333333334 app_window.js:1201:1
[AppWindow][Messages][AppWindow_5][3531.184] publishing internal event: inputmethod-contextchange app_window.js:1201:1
[AppWindow][Messages][AppWindow_5][3531.188] handling _inputmethod-contextchange app_window.js:1201:1
[AppWindow][Messages][AppWindow_5][3531.194] aria-hidden on browser element:false app_window.js:1201:1
[AppWindow][Messages][AppWindow_5][3531.300] request RESIZE...active? ,true app_window.js:1201:1
[AppWindow][Messages][AppWindow_5][3531.305]  will resize...  app_window.js:1201:1
[AppWindow][Messages][AppWindow_5][3531.319] force RESIZE... app_window.js:1201:1
[AppWindow][Messages][AppWindow_5][3531.324] publishing internal event: withkeyboard app_window.js:1201:1
[AppWindow][Messages][AppWindow_5][3531.331] publishing internal event: resize app_window.js:1201:1
[AppWindow][Messages][AppWindow_5][3531.335] publishing external event: resize app_window.js:1201:1
[AppWindow][Messages][AppWindow_5][3531.338] W:,320,H:,333.3333333333333 app_window.js:1201:1
[AppWindow][Messages][AppWindow_5][3531.638]  nextpaint is timeouted. app_window.js:1201:1
[AppWindow][Messages][AppWindow_5][3531.701]  Handling mozbrowserasyncscroll event... app_window.js:1201:1
[AppWindow][Messages][AppWindow_5][3531.751]  nextpainted. app_window.js:1201:1
[AppWindow][Messages][AppWindow_5][3531.997]  Handling mozbrowserasyncscroll event...
Here is better log with DEBUG flag for HierarchyManager/LayoutManager/KeyboardManager/AppWindow are all turned on:

[HierarchyManager][65567.854] mozChromeEvent base_module.js:491:1
[HierarchyManager][65567.954] mozChromeEvent base_module.js:491:1
[Keyboard Manager] get focus event textarea keyboard_manager.js:61:7
[Keyboard Manager] set layout to display: group=text index=0 keyboard_manager.js:61:7
[HierarchyManager][65568.082] mozChromeEvent base_module.js:491:1
[HierarchyManager][65568.086] handover mozChromeEvent to AppWindowManager base_module.js:491:1
[AppWindow][Messages][AppWindow_7][65568.089] publishing internal event: inputmethod-contextchange app_window.js:1201:1
[AppWindow][Messages][AppWindow_7][65568.093] handling _inputmethod-contextchange app_window.js:1201:1
[HierarchyManager][65568.149] mozChromeEvent base_module.js:491:1
[Keyboard Manager] get blur event keyboard_manager.js:61:7
[LayoutManager][65568.257] resize event got: ,keyboardhide layout_manager.js:210:1
[LayoutManager][65568.260] publish: system-resize layout_manager.js:210:1
[HierarchyManager][65568.270] system-resize base_module.js:491:1
[HierarchyManager][65568.274] handover system-resize to AppWindowManager base_module.js:491:1
[AppWindow][Messages][AppWindow_7][65568.277] request RESIZE...active? ,true app_window.js:1201:1
[AppWindow][Messages][AppWindow_7][65568.280]  will resize...  app_window.js:1201:1
[AppWindow][Messages][AppWindow_7][65568.285] force RESIZE... app_window.js:1201:1
[AppWindow][Messages][AppWindow_7][65568.289] publishing internal event: withoutkeyboard app_window.js:1201:1
[AppWindow][Messages][AppWindow_7][65568.293] publishing internal event: resize app_window.js:1201:1
[AppWindow][Messages][AppWindow_7][65568.297] publishing external event: resize app_window.js:1201:1
[AppWindow][Messages][AppWindow_7][65568.301] W:,320,H:,569.3333333333334 app_window.js:1201:1
[HierarchyManager][65568.353] mozChromeEvent base_module.js:491:1
[HierarchyManager][65568.357] handover mozChromeEvent to AppWindowManager base_module.js:491:1
[AppWindow][Messages][AppWindow_7][65568.361] publishing internal event: inputmethod-contextchange app_window.js:1201:1
[AppWindow][Messages][AppWindow_7][65568.369] handling _inputmethod-contextchange app_window.js:1201:1
[AppWindow][Messages][AppWindow_7][65568.373] aria-hidden on browser element:false app_window.js:1201:1
[LayoutManager][65568.445] resize event got: ,keyboardchange layout_manager.js:210:1
[LayoutManager][65568.449] publish: system-resize layout_manager.js:210:1
[HierarchyManager][65568.452] system-resize base_module.js:491:1
[HierarchyManager][65568.455] handover system-resize to AppWindowManager base_module.js:491:1
[AppWindow][Messages][AppWindow_7][65568.459] request RESIZE...active? ,true app_window.js:1201:1
[AppWindow][Messages][AppWindow_7][65568.461]  will resize...  app_window.js:1201:1
[AppWindow][Messages][AppWindow_7][65568.464] force RESIZE... app_window.js:1201:1
[AppWindow][Messages][AppWindow_7][65568.467] publishing internal event: withkeyboard app_window.js:1201:1
[AppWindow][Messages][AppWindow_7][65568.470] publishing internal event: resize app_window.js:1201:1
[AppWindow][Messages][AppWindow_7][65568.473] publishing external event: resize app_window.js:1201:1
[AppWindow][Messages][AppWindow_7][65568.476] W:,320,H:,333.3333333333333 app_window.js:1201:1
[AppWindow][Messages][AppWindow_7][65568.760]  nextpainted. app_window.js:1201:1
[AppWindow][Messages][AppWindow_7][65568.768]  nextpainted. app_window.js:1201:1
[HierarchyManager][65568.951] mozChromeEvent base_module.js:491:1
[HierarchyManager][65569.073] getting top most...,[object Object] base_module.js:491:1
[HierarchyManager][65569.098] windowclosed
Comment on attachment 8607930 [details] [review]
[gaia] timdream:system-wait-nextpaint-before-hide-keyboard2 > mozilla-b2g:master

Alive, could you review this again? Thanks.

The cause of the previous bug is because we did not unset _currentWindow synchronously, so when the to-be-hidden keyboard subsequently ask for high change, it ended up dispatch keyboardchange event to LayoutManager.

The fix here remove the flag introduced, and use _currentWindow to decide the behavior just like all other functions do. I should have been more careful when thinking about introducing flags.

To help you review here is the diff of the two patches:

diff --git a/apps/system/js/input_window_manager.js b/apps/system/js/input_window_manager.js
index ca3552c..66cd93d 100644
--- a/apps/system/js/input_window_manager.js
+++ b/apps/system/js/input_window_manager.js
@@ -400,9 +400,6 @@
       nextWindow = this._makeInputWindow(configs);
     }
 
-    // Prevent any pending async hideInputWindow from being triggered.
-    this._inputWindowToHide = false;
-
     if (this._currentWindow && nextWindow !== this._currentWindow) {
       // we have some displayed InputWindow and the new window is a different IW
       // let the new one show immediately, and this._lastWindow will also be
@@ -425,19 +422,19 @@
       return;
     }
 
-    // Set this flag so showInputWindow can cancel us.
-    this._inputWindowToHide = true;
+    var windowToClose = this._currentWindow;
+    this._currentWindow = null;
 
     // First we publish an keyboardhide event that would cause the
     // foreground app to resize.
     return this._kbPublish('keyboardhide', undefined)
       .then(function iwm_hideInputWindowSync() {
-        if (!this._inputWindowToHide) {
+        // We should not close ourselves if we are being set back to become
+        // the currentWindow again.
+        if (this._currentWindow === windowToClose) {
           return;
         }
 
-        var windowToClose = this._currentWindow;
-        this._currentWindow = null;
         windowToClose.close();
       }.bind(this))
       .catch((e) => { console.error(e); });
diff --git a/apps/system/test/unit/input_window_manager_test.js b/apps/system/test/unit/input_window_manager_test.js
index 4159c25..688e7ab 100644
--- a/apps/system/test/unit/input_window_manager_test.js
+++ b/apps/system/test/unit/input_window_manager_test.js
@@ -756,13 +756,12 @@ suite('InputWindowManager', function() {
                       '_makeInputWindow should be called with correct configs');
       });
 
-      test('Flip _inputWindowToHide to false', function() {
+      test('Fill _currentWindow', function() {
         this.sinon.stub(manager, '_extractLayoutConfigs').returns(configs);
         this.sinon.stub(manager, '_makeInputWindow').returns(inputWindow);
 
-        manager._inputWindowToHide = true;
         manager.showInputWindow(layout);
-        assert.isFalse(manager._inputWindowToHide);
+        assert.equal(manager._currentWindow, inputWindow);
       });
 
       suite('Determine reusability of InputWindows', function() {
@@ -889,11 +888,11 @@ suite('InputWindowManager', function() {
       manager.hideInputWindow();
 
       assert.isTrue(stubKBPublish.calledWith('keyboardhide', undefined));
+      assert.equal(manager._currentWindow, null);
 
-      manager._inputWindowToHide = false;
+      manager._currentWindow = inputWindow;
 
       Promise.resolve().then(function() {
-        assert.isTrue(!!manager._currentWindow);
         assert.isFalse(inputWindow.close.called);
       }).then(done, done);
     });
Attachment #8607930 - Flags: review?(alive)
Comment on attachment 8607930 [details] [review]
[gaia] timdream:system-wait-nextpaint-before-hide-keyboard2 > mozilla-b2g:master

r=me, thanks!
Attachment #8607930 - Flags: review?(alive) → review+
Attachment #8603161 - Attachment is obsolete: true
Attachment #8605522 - Attachment is obsolete: true
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Depends on: 1167654
You need to log in before you can comment on or make changes to this bug.