820057 - [Marketplace][Keyboard] Keyboard stays up after hitting return to sign in on persona

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect, P3)

Description

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

## Environment :
Unagi phone, build 2012-12-9

## Repro :
1. launch marketplace
2. hit the setttings button 
3. scroll down and click on sign in
4. type in the username in username field
5. type in the password for the password field and hit return to submit the form

## Expected :
1. valid username and password should allow for sign in and the keyboard dismisses

## Actual :
1. keyboard stays up (you do sign in though)

## Note :
1. bug 818575 for the repaint issue with the keyboard

Comment 1

[Jason Smith [:jsmith]]

Saw this also with the trusted UI with persona.

Comment 2

[David Scravaglieri [:scravag]]

The keyboard can be dismissed

Comment 3

[Jason Smith [:jsmith]]

(In reply to David Scravaglieri [:scravag] from comment #2)
> The keyboard can be dismissed

Sorry, but this is too common of a scenario to not block. Think about it this way - would you want every single persona sign-in experience including at marketplace to show this problem? This certainly won't make the persona guys happy.

This is the reason why as soon as we pref on trusted UI with persona that this will become a smoketest.

Comment 4

[David Scravaglieri [:scravag]]

Please if dogfooders complain renominate this.

Comment 5

[Ben Adida [:benadida]]

This seems pretty bad, and I don't see how this could be fixed on the server. I think we need to consider this a client-side blocker.

Note to David: dogfooders won't see this until we pref identity on. jsmith, I think we need to do that ASAP. With test automation still problematic, we really need user feedback now.

Comment 6

[Jason Smith [:jsmith]]

(In reply to Ben Adida [:benadida] from comment #5)
> This seems pretty bad, and I don't see how this could be fixed on the
> server. I think we need to consider this a client-side blocker.
> 
> Note to David: dogfooders won't see this until we pref identity on. jsmith,
> I think we need to do that ASAP. With test automation still problematic, we
> really need user feedback now.

Actually, it's reproducible with or without the pref. And yeah, I know we need to pref on asap, I'm actively waiting for marketplace prod to land the b2g shim support. When then lands, we'll do a quick check, and we should good to pref on.

Comment 7

[Jason Smith [:jsmith]]

(In reply to David Scravaglieri [:scravag] from comment #4)
> Please if dogfooders complain renominate this.

And now we know dogfooders are complaining. This was loud and clear on the feedback report. Back to nom.

Comment 8

[Jason Smith [:jsmith]]

Asking for input also from product on this one on whether they think this blocks.

Comment 9

[Peter Dolanjski [:pdol]]

After trying this myself on today's build, it is, indeed, very confusing for the user as it is not clear what happened. I just see a white screen until I click off of it.  
Despite arguing against it in triage, I think we should block on it.

Will this be fixed with bug 818575?

Comment 10

[Jason Smith [:jsmith]]

(In reply to Peter Dolanjski from comment #9)
> After trying this myself on today's build, it is, indeed, very confusing for
> the user as it is not clear what happened. I just see a white screen until I
> click off of it.  
> Despite arguing against it in triage, I think we should block on it.
> 
> Will this be fixed with bug 818575?

Don't know. We might want to retest after bug 818575 gets fixed though.

Comment 11

[:Margaret Leibovic]

This seems like a focus issue, since the keyboard hides itself when it gets a 'blur' event. I see in popup_manager.js, we call |window.focus()| when the popup closes, but maybe we need to be more specific about where we're moving focus. Or do something to explicitly send a blur event to the keyboard if it's open, so that we'll always properly hide the keyboard when a popup closes.

Comment 12

[:Margaret Leibovic]

Cc'ing Etienne, since I see he originally implemented this PopupManager logic:
https://github.com/mozilla-b2g/gaia/commit/f7167ae93ab32a657da484ba0aa4f6c0d7350e1b

Comment 13

[:Margaret Leibovic]

Actually, I'm wondering if the window is closing before the gecko keyboard logic has a change to actually fire the onfocuschange listener. Maybe this is some forms.js issue?

Comment 15

[:Margaret Leibovic]

I added some debugging code on the gecko side, and I found that what's happening is that we are getting a blur event on the input element as expected in forms.js and firing 'Forms:Input', but we're never getting this 'Forms:Input' message in Keyboard.jsm.

Could this be some issue with the forms.js process getting killed before that message makes its way back to the parent process?

Comment 16

[:Margaret Leibovic]

(In reply to Margaret Leibovic [:margaret] from comment #15)
> I added some debugging code on the gecko side, and I found that what's
> happening is that we are getting a blur event on the input element as
> expected in forms.js and firing 'Forms:Input', but we're never getting this
> 'Forms:Input' message in Keyboard.jsm.
> 
> Could this be some issue with the forms.js process getting killed before
> that message makes its way back to the parent process?

cjones, do you know what might be going on here?

I've found that when you close the popup with the "X" button in the header, things work as they should, but in that case the gaia PopupManager is just removing the frame from the DOM itself, as opposed to listening for a 'mozbrowserclose' event (which I assume gets fired if the popup closes itself, like what the persona login popup is doing).

Comment 17

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

That sounds plausible, though I don't fully understand the setup you're testing.

What happens if you bring up the keyboard in an app or browser tab and then |adb shell kill -9 [that process]|?  Does the keyboard go away or stay open?  We need to ensure we handle that case.

Comment 18

[:Margaret Leibovic]

Hrm, I think I was confused. I'm testing in the Marketplace app, opening the Person login popup you get when clicking the "Log in" button on the settings page. I thought that the popup window opened in a new process, but that appears to be wrong, since I don't see it when I do |adb shell b2g-ps| (killing a whole process does indeed hide the keyboard like we'd want).

However, I do see the "### forms.js loaded" message when the popup opens, so I still think something is probably killing whatever that script is doing before its 'Forms:Input' message makes its way to Keyboard.jsm.

(Forgive my rough understanding of how processes and frame scripts work, I'm just making assumptions based on what my logging is telling me)

Comment 19

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Margaret Leibovic [:margaret] from comment #18)
> Hrm, I think I was confused. I'm testing in the Marketplace app, opening the
> Person login popup you get when clicking the "Log in" button on the settings
> page. I thought that the popup window opened in a new process, but that
> appears to be wrong, since I don't see it when I do |adb shell b2g-ps|
> (killing a whole process does indeed hide the keyboard like we'd want).

OK, gotcha.  Popup windows (except in a special case) should always open in the same process.

> However, I do see the "### forms.js loaded" message when the popup opens, so
> I still think something is probably killing whatever that script is doing
> before its 'Forms:Input' message makes its way to Keyboard.jsm.

I guess it's possible that the popup is closed and the mm's destroyed before the mm message goes through.  That would be slightly surprising but believable.

I'm not sure how the "close-keyboard-on-crash" logic works, but for robustness we should be doing something similar for window closes, like "close-keyboard-on-focused-window-close".

Comment 20

[David Flanagan [:djf]]

cpeterson: you removed an event handler that no one understood from forms.js.  Is that still gone, or did it get reverted?  And could it have anything to do with handling this case?

Comment 21

[Chris Peterson [:cpeterson]]

The "ime-enabled-state-changed" event observer is still removed from form.js. It was firing at the wrong times because of the many problems with touch/mouse events.

Does the popup change the input focus? It sounds like something like ime-enabled-state-changed might be helpful if the keyboard should be closing when the popup closes if the input focus is not changing in a way that forms.js handles.

After bug 823619 is fixed, ime-enabled-state-changed will probably work (more) correctly, so we could reintroduce it to forms.js (though that has its own risks).

Comment 22

[:Margaret Leibovic]

(In reply to Chris Peterson (:cpeterson) from comment #21)

> Does the popup change the input focus? It sounds like something like
> ime-enabled-state-changed might be helpful if the keyboard should be closing
> when the popup closes if the input focus is not changing in a way that
> forms.js handles.

I experimented with adding back the ime-enabled-state-changed listener, and that didn't fix the problem.

The input is losing focus as we'd want, and hideKeyboard is getting called in forms.js; the problem is that the 'Forms:Input' message is never making its way to Keyboard.jsm. My suspicion is that we're running into some problem because the popup window frame is being destroyed immediately after we try to send this message.

I turned on debug logging in BrowserElementChild.js/BrowserElementParent.js, and this is what the log says after I hit the enter button on the keyboard to submit the Persona login form (I also added some of my own logging to forms.js/Keyboard.jsm):

I/Gecko   ( 1733): BrowserElementChild - scroll event [object Window]
I/Gecko   ( 1633): BrowserElementParent - fireEventFromMsg: scroll, [object Object]
I/Gecko   ( 1733): BrowserElementChild - Closing window [object Window]
I/Gecko   ( 1633): BrowserElementParent - fireEventFromMsg: close, undefined
I/Gecko   ( 1733): FormAssistant.handleEvent: blur, [object HTMLInputElement]
I/Gecko   ( 1733): FormAssistant.hideKeyboard - sent 'Forms:Input'
  --> we never receive this message in Keyboard.jsm
I/Gecko   ( 1733): FormAssistant.handleEvent: blur, [object HTMLDocument]
I/Gecko   ( 1733): FormAssistant.handleEvent: blur, [object Window]
I/Gecko   ( 1733): BrowserElementChild - Closing window [object Window]

Cc'ing jlebar here also, since it looks like he's worked in the relevant code.

Comment 23

[:Margaret Leibovic]

Attachment: Hide the keyboard if the window that opened it is closed

This patch feels a bit hacky, but it fixes the problem by sending along a blur message on mozbrowserclose. Looking at our gecko/gaia keyboard code, it doesn't seem like an occasional extra blur event would cause a problem, but maybe I'm missing something. I added some logging to the onfocuschange handler in keyboard.js, and it wasn't firing if I closed the popup with the keyboard closed, so maybe something else is even taking care of that for us?

I tried looking into the message manager code to see why the message is being dropped, but I got too confused before I could figure it out. We should get an expert in here (who would that be?) to see if this is expected behavior or if it's a bug.

Comment 24

[Tim Taubert [:ttaubert] (inactive)]

Stealing back :)

Comment 25

[Tim Taubert [:ttaubert] (inactive)]

Attachment: hide the keyboard on submit and pagehide events

Copied the behavior from the old XUL Fennec. We should/ need to hide the keyboard when the page is hidden or the form is submitted.

Comment 26

[:Margaret Leibovic]

Comment on attachment 698591 [details] [diff] [review]
hide the keyboard on submit and pagehide events

Team effort, but Tim can get the cred :)

Comment 27

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/40b1de54022a

Comment 28

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/b49d6c4f4973

Comment 29

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/40b1de54022a