1079728 - Taking focus away from <select> will not send out blur message

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

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

The user visible bug is fixed at the cloned bug, but it looks like after bug 1057898 the inputcontextchange event will fire again when the focus is move away from the focused frame?

Let's use this bug to fix it in Gecko as well.

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

Description:

Repro Steps:
1: Update a Flame to 20141006040204
2: Open Settings > Device Information > More Information > Enable Developer Menu
3: From the developer menu, enable the software home button > open the Debugging via USB menu
4: Press the software home button 

Actual: The menu remains, navigation takes place in the background.

Expected: The user is brought to the homescreen.

Flame 2.2 (319mb/fullflash)
Device: Flame 2.2
BuildID: Device: Flame 2.2
BuildID: 20141006040204
Gaia: 470826d13ae130a5c3d572d1029e595105485fb0
Gecko: e0d714f43edc
Gonk: 2c909e821d107d414f851e267dedcd7aae2cebf
Version: 35.0a1 (2.2)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0

Notes:
This issue does NOT occur using the normal home button.

Repro frequency: 100%

See attached: logcat

Video clip: http://youtu.be/-LRTeQH_tbc

Comment 1

[(inactive after 6/18) Alive Kuo [:alive]]

Not sure what does bug 1057898 doing. But this smells like the same root cause as:
* Open UITest > Keyboard.
* Focus an input and the keyboard will show.
* Press home button, the keyboard will hide.
* Open UITest again.
Actual:
The keyboard displays again once the app is opened. That means the focus outside the iframe does not lose the focusElement in it.

So the real problem of comment 0 seems to me:
The settings app is focused "again" while the settings app is closing/homescreen is opening.

Comment 2

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

Bug 1078822 did not fix user facing issue from Gaia -- need to find the root cause in Gecko.

Comment 3

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

Attachment: WIP

Here is my WIP fix and the log after applying the fix. Note that this proposed fix only take align the behavior of <select> to <input>; when the user hits software home button, the System app will still receive three inputcontextchange events (blur-focus-blur), instead of one (blur). Explaining below:

The log confirm the root cause of this bug. In bug 1057898 I am overloading |isKeyboardOpened| which will not be set to true for <select> elements, therefore a blur message will never send from froms.js if <select> loose focus.

However the real cause of this bug is the bogus blur-and-focus element happens when the user press the software home button. With the hardware home button press, log marked * does not happen, which is expected (System app [pid 4748] receiving focus and Settings [pid 5275] lost focus).

Alive, is it possible we try to regain focus of the closing frame when the user hit software home button? If so we should remove that and fix the bug -- if not we need to dig deeper.

===============

I/Gecko   ( 5275): XXXXX 390 blur *
I/Gecko   ( 5275): XXXXX 685 hideKeyboard *
I/Gecko   ( 5275): XXXXX 689 fa_hideKeyboardSync *
I/Gecko   ( 5275): XXXXX 695 fa_hideKeyboardSync sendAsyncMessage *
I/Gecko   ( 5275): XXXXX 358 focus  *
I/Gecko   ( 5275): XXXXX 362 focus [object HTMLBodyElement] *
I/Gecko   ( 5275): XXXXX 358 focus  *
I/Gecko   ( 5275): XXXXX 362 focus [object Window] *
I/Gecko   ( 5275): XXXXX 358 focus  *
I/Gecko   ( 5275): XXXXX 362 focus [object HTMLSelectElement] *
I/Gecko   ( 5275): XXXXX 668 showKeyboard *
I/Gecko   ( 5275): XXXXX 674 fa_showKeyboardSync *
I/Gecko   ( 5275): XXXXX 737 sendKeyboardState *
I/Gecko   ( 4748): XXXXX 358 focus 
I/Gecko   ( 4748): XXXXX 362 focus [object HTMLBodyElement]
I/Gecko   ( 4748): XXXXX 358 focus 
I/Gecko   ( 4748): XXXXX 362 focus [object Window]
I/Gecko   ( 5275): XXXXX 390 blur
I/Gecko   ( 5275): XXXXX 685 hideKeyboard
I/Gecko   ( 5275): XXXXX 689 fa_hideKeyboardSync
I/Gecko   ( 5275): XXXXX 695 fa_hideKeyboardSync sendAsyncMessage

Comment 4

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

Comment on attachment 8506838 [details] [diff] [review]
WIP

This patch is incomplete.

Comment 5

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

Alive and I have identified the additional focus() call originates.

https://github.com/mozilla-b2g/gaia/blob/78fe65635bff2b312ac1/apps/system/js/value_selector/value_selector.js#L242-L244

Alive, 

After thinking more about it, how about remove the app.focus(); here? Since value selector are not suppose to take away focus anyway, it shouldn't need to give the focus back regardless.

Comment 6

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

Bug 1078822 did fix the user-facing issue.

Comment 7

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

Attachment: Patch v1.0

forms.js has involved to a point where |isKeyboardOpened| serves only two purpose:

1. Allow resize event handler to decide whether or not to really handle the event.
2. Tell fa_hideKeyboardSync() whether or not to really send out the blur message

This patch fixes (2) with a side effect allowing resize event handler to handle not only for <input> but for <select>. I think the side effect is justified because Input Method API should not depend on current Gaia interface design (which covers the frame when the <select> is focused). We should always handle resize event regardless.

The patch also rename all instances of the word "Keyboard" so the meaning can be more generic.

(Test patch and try run URL to come)

Comment 8

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

Attachment: Test for Patch v1

The test is simply the <select> version of test_two_inputs.html. I have verified the test is valid simply by running the test w/o the patch and w/ the patch.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=df49e40acdfd

Comment 9

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #5)
> Alive and I have identified the additional focus() call originates.
> 
> https://github.com/mozilla-b2g/gaia/blob/78fe65635bff2b312ac1/apps/system/js/
> value_selector/value_selector.js#L242-L244
> 
> Alive, 
> 
> After thinking more about it, how about remove the app.focus(); here? Since
> value selector are not suppose to take away focus anyway, it shouldn't need
> to give the focus back regardless.

Sure if value selector is good at that.

Comment 10

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

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #9)
> Sure if value selector is good at that.

Bug 1085233 is filed for this.

Comment 11

[Yuan Xulei [:yxl] (Leave from May 18)]

Comment on attachment 8507619 [details] [diff] [review]
Patch v1.0

Review of attachment 8507619 [details] [diff] [review]:
-----------------------------------------------------------------

The patch is good to go if you fix `scrollSelectionOrElementIntoView()` issue.

::: dom/inputmethod/forms.js
@@ +417,2 @@
>            return;
>  

The following `scrollSelectionOrElementIntoView()` operation doesn't apply to <selection> and only valid for text input and content editable.
So you should check the focusedElement by calling isTextInputElement() before doing `scrollSelectionOrElementIntoView()`.

Comment 12

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

(In reply to Yuan Xulei [:yxl] from comment #11)
> The following `scrollSelectionOrElementIntoView()` operation doesn't apply
> to <selection> and only valid for text input and content editable.
> So you should check the focusedElement by calling isTextInputElement()
> before doing `scrollSelectionOrElementIntoView()`.

I don't understand why this code is not valid for <select>

http://dxr.mozilla.org/mozilla-central/source/dom/inputmethod/forms.js#1121-1155

|getPlaintextEditor()| would return null and it would make |scrollSelectionOrElementIntoView()| simply call |element.scrollIntoView(false)|. Is that not enough?

Comment 13

[Yuan Xulei [:yxl] (Leave from May 18)]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #12)
> (In reply to Yuan Xulei [:yxl] from comment #11)
> > The following `scrollSelectionOrElementIntoView()` operation doesn't apply
> > to <selection> and only valid for text input and content editable.
> > So you should check the focusedElement by calling isTextInputElement()
> > before doing `scrollSelectionOrElementIntoView()`.
> 
> I don't understand why this code is not valid for <select>
> 
> http://dxr.mozilla.org/mozilla-central/source/dom/inputmethod/forms.js#1121-
> 1155
> 
> |getPlaintextEditor()| would return null and it would make
> |scrollSelectionOrElementIntoView()| simply call
> |element.scrollIntoView(false)|. Is that not enough?

|getPlaintextEditor()| is designed for <input>, <textarea> and contenteditable, but it surprisingly works for <select>...
So just as your explanation, scrollSelectionOrElementIntoView() does work for <select>

Comment 14

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

(In reply to Yuan Xulei [:yxl] from comment #13)
> |getPlaintextEditor()| is designed for <input>, <textarea> and
> contenteditable, but it surprisingly works for <select>...
> So just as your explanation, scrollSelectionOrElementIntoView() does work
> for <select>

Thanks.

Comment 15

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

Attachment: Patch for commit, part 1

Comment 16

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

Attachment: Patch for commit, part 2

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/f5d6d05320c5

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f5d6d05320c5

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f5d6d05320c5