Closed Bug 820057 Opened 7 years ago Closed 7 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

(blocking-basecamp:+, firefox19 wontfix, firefox20 fixed, firefox21 fixed, b2g18+ fixed)

RESOLVED FIXED
B2G C4 (2jan on)
blocking-basecamp +
Tracking Status
firefox19 --- wontfix
firefox20 --- fixed
firefox21 --- fixed
b2g18 + fixed

People

(Reporter: nhirata, Assigned: ttaubert)

References

Details

Attachments

(1 file, 1 obsolete file)

## 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
Saw this also with the trusted UI with persona.
blocking-basecamp: --- → ?
Whiteboard: DUPEME
The keyboard can be dismissed
blocking-basecamp: ? → ---
blocking-kilimanjaro: --- → +
Priority: -- → P3
Whiteboard: DUPEME
blocking-kilimanjaro: + → ---
tracking-b2g18: --- → +
(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.
blocking-basecamp: --- → ?
Please if dogfooders complain renominate this.
blocking-basecamp: ? → -
tracking-b2g18: --- → +
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.
(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.
(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.
blocking-basecamp: - → ?
Asking for input also from product on this one on whether they think this blocks.
Flags: needinfo?(pdolanjski)
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?
blocking-basecamp: ? → +
Flags: needinfo?(pdolanjski)
(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.
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.
Cc'ing Etienne, since I see he originally implemented this PopupManager logic:
https://github.com/mozilla-b2g/gaia/commit/f7167ae93ab32a657da484ba0aa4f6c0d7350e1b
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?
Assignee: nobody → ttaubert
Target Milestone: --- → B2G C4 (2jan on)
Duplicate of this bug: 826365
Assignee: ttaubert → 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?
(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).
Flags: needinfo?(jones.chris.g)
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.
Flags: needinfo?(jones.chris.g)
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)
(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".
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?
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).
(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.
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.
Attachment #698143 - Flags: review?(ttaubert)
Stealing back :)
Assignee: margaret.leibovic → ttaubert
Status: NEW → ASSIGNED
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.
Attachment #698143 - Attachment is obsolete: true
Attachment #698143 - Flags: review?(ttaubert)
Attachment #698591 - Flags: review?(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 :)
Attachment #698591 - Flags: review?(margaret.leibovic) → review+
https://hg.mozilla.org/releases/mozilla-b2g18/rev/b49d6c4f4973
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Component: Gaia::Keyboard → General
QA Contact: wachen
You need to log in before you can comment on or make changes to this bug.