914100 - [Messages] Prevent keyboard from disappearing when recipients are rendered after user input

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

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

Gaia   94e5f269874b02ac0ea796b64ab995fce9efa4b3
SourceStamp ab5f29823236
BuildID 20130906040204
Version 26.0a1
Buri

1. launch SMS
2. type in a phone number and type ;

Expected: the keyboard does not go away after typing ;
Actual: keyboard disappears and you have to tap into the "To" field in order to bring back the keyboard

Comment 1

[Julien Wajsberg [:julienw]]

asking koi because this is annoying and probably a regression from bug 906581.

Comment 2

[Rick Waldron [:rwaldron]]

Julien, were you able to reproduce?

Comment 3

[Julien Wajsberg [:julienw]]

mmm strangely I thought I could reproduce, but right now I can't.

Comment 4

[Joe Cheng [:jcheng] (please needinfo)]

qawanted to confirm reproducibility

Comment 5

[Chris Kreinbring [:CKreinbring]]

Able to repro with the steps given on Buri 1.2 mozilla RIL.  After hitting the ; key the phone number turns into a contact bubble and the keyboard closes, forcing the user to tap the To field in order to enter additional numbers.

Build ID: 20130927004015
Gecko: http://hg.mozilla.org/releases/mozilla-aurora/rev/7eebf2d65429
Gaia: 1e9470b9b6df630eddf1c4c8b25b3170ee786b0e
Platform Version: 26.0a2

Comment 6

[Joe Cheng [:jcheng] (please needinfo)]

triage: koi+ for regression

Comment 7

[Evelyn Eastmond [:evhan55]]

Taking this from Rick for now.  A good chance to investigate all this plumbing and learn how sms and keyboard function.

Comment 8

[Rick Waldron [:rwaldron]]

(In reply to Evelyn Eastmond [:evhan55] from comment #7)
> Taking this from Rick for now.  A good chance to investigate all this
> plumbing and learn how sms and keyboard function.

Please r? me when ready :)

Comment 9

[Evelyn Eastmond [:evhan55]]

(In reply to Rick Waldron from comment #8)
> (In reply to Evelyn Eastmond [:evhan55] from comment #7)
> > Taking this from Rick for now.  A good chance to investigate all this
> > plumbing and learn how sms and keyboard function.
> 
> Please r? me when ready :)

Absolutely!  I'll have questions soon about how this works, probably.

Comment 10

[Evelyn Eastmond [:evhan55]]

Attachment: https://github.com/mozilla-b2g/gaia/pull/12713

- Recipients.View
    - Had a race condition between render() and focus() because of calls within render() which were asynchronous.  Separated out the calls after a ';' is typed to wait for render() to finish before calling focus(), which causes the keyboard to reappear (or not disappear).
- Tests:
    - Integration test needed?

Comment 11

[Rick Waldron [:rwaldron]]

Combining bug 924158

Comment 13

[Evelyn Eastmond [:evhan55]]

Comment on attachment 814131 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/12713

- Recipients.View
    - Had a race condition between render() and focus() because of calls to innerHTML within render() (and target.isPlaceholder?) which were asynchronous
    - Used setTimeout in 3 places to defer setting focus to the next execution turn to allow recipients list rendering to complete
- Tests:
    - Integration test needed?

- Questions:
    - This now fixes three cases of the keyboard hiding:
        1) when adding recipients by typing ';'
        2) when deleting manually entered contacts
        3) when deleting contacts entered from the contacts app.
      However, the keyboard *still* hides when you are adding recipients from the contacts app, because that gets handled by Recipients.prototype.add, which calls render() at the end, but not focus(). This may be desired behavior when you are generally adding from contacts app, or maybe not.  I suggest adding a focus here as well.  Waiting for review to decide.

Comment 14

[Rick Waldron [:rwaldron]]

(In reply to Evelyn Eastmond [:evhan55] from comment #13)

>       However, the keyboard *still* hides when you are adding recipients
> from the contacts app, because that gets handled by
> Recipients.prototype.add, which calls render() at the end, but not focus().
> This may be desired behavior when you are generally adding from contacts
> app, or maybe not.  I suggest adding a focus here as well.  Waiting for
> review to decide.

This is correct, but I can't locate the bug thread for it.

Comment 15

[Evelyn Eastmond [:evhan55]]

(In reply to Rick Waldron from comment #14)
> (In reply to Evelyn Eastmond [:evhan55] from comment #13)
> 
> >       However, the keyboard *still* hides when you are adding recipients
> > from the contacts app, because that gets handled by
> > Recipients.prototype.add, which calls render() at the end, but not focus().
> > This may be desired behavior when you are generally adding from contacts
> > app, or maybe not.  I suggest adding a focus here as well.  Waiting for
> > review to decide.
> 
> This is correct, but I can't locate the bug thread for it.

Ok! As long as it has been covered and not seen as a missing case in this bug.

Comment 16

[Julien Wajsberg [:julienw]]

(In reply to Evelyn Eastmond [:evhan55] from comment #13)
> Comment on attachment 814131 [details] [review]
> https://github.com/mozilla-b2g/gaia/pull/12713
> 
> - Recipients.View
>     - Had a race condition between render() and focus() because of calls to
> innerHTML within render() (and target.isPlaceholder?) which were asynchronous

Mmm assigning to innerHTML is not asynchronous afaik ?


>     - Used setTimeout in 3 places to defer setting focus to the next
> execution turn to allow recipients list rendering to complete

I'd like to avoid using setTimeout as much as possible, this is a road to unpredictable behavior :)

Could you please roughly describe what happens when we press ';' to terminate a contact ?

Comment 17

[Rick Waldron [:rwaldron]]

(In reply to Julien Wajsberg [:julienw] (in MozSummit until next monday) from comment #16)
> (In reply to Evelyn Eastmond [:evhan55] from comment #13)
> > Comment on attachment 814131 [details] [review]
> > https://github.com/mozilla-b2g/gaia/pull/12713
> > 
> > - Recipients.View
> >     - Had a race condition between render() and focus() because of calls to
> > innerHTML within render() (and target.isPlaceholder?) which were asynchronous
> 
> Mmm assigning to innerHTML is not asynchronous afaik ?

There does seem to be some special handling when setting innerHTML of a contenteditable: http://mxr.mozilla.org/mozilla-central/source/content/base/src/Element.cpp#3227

I worked on this with Evelyn and it appears the updates to innerHTML aren't complete (and ready to accept focus) until at least the end of the turn. 


FWIW, I'm seeing maybe what you're seeing, re: ";", but I get different behaviour in two different devices. Unagi works correctly, inari works incorrectly.

Comment 18

[Rick Waldron [:rwaldron]]

This: 

> FWIW, I'm seeing maybe what you're seeing, re: ";", but I get different behaviour in two different devices. Unagi works correctly, inari works incorrectly.

...is wrong.

Comment 19

[Rick Waldron [:rwaldron]]

Going back to the beginning of multi-recipient support, I branched from 2e6a20d256cd79a3cf806abe01605c6446a8629c and ran the same tests and the results are the same as they are today:

1. type "asdf;" (keyboard hides)
2. type "asdf" <enter> (keyboard hides)


Removing "regression" keyword as this never behaved the way the OP expected.

Comment 20

[Evelyn Eastmond [:evhan55]]

(In reply to Julien Wajsberg [:julienw] (in Paris-Web conference thursday and friday) from comment #16)
> Could you please roughly describe what happens when we press ';' to
> terminate a contact ?

Roughly, when we press ';' to terminate a contact:
1) the newly-typed recipient is checked for validity
2) if it is a valid recipient, it is added to the list of recipients and the recipients list is re-rendered from scratch in 'render' method
3) also in 'render' method, a new placeholder recipient is added to the end of the list
4) finally, 'focus' is called to put focus on that new placeholder recipient

The render and focus methods necessarily need to happen in that order, since we want to focus on the new placeholder recipient, but that placeholder is added in the render method.

For some reason, the innerHTML call in render method is not finishing synchronously, it seems.
If it's not exactly innerHTML, *something* in the render method is not finishing synchronously.
That is why we are delaying the call to focus in this way.

Let me know what you think.

Comment 21

[Joe Cheng [:jcheng] (please needinfo)]

adding Sprint 3 to target milestone. please let us know if this a problem then we can re-discuss

Comment 22

[Evelyn Eastmond [:evhan55]]

Please refer to
https://bugzilla.mozilla.org/show_bug.cgi?id=914100#c16

Comment 23

[Evelyn Eastmond [:evhan55]]

(In reply to Evelyn Eastmond [:evhan55] from comment #22)
> Please refer to
> https://bugzilla.mozilla.org/show_bug.cgi?id=914100#c16

I meant
https://bugzilla.mozilla.org/show_bug.cgi?id=914100#c20
copy error

Comment 24

[Julien Wajsberg [:julienw]]

QA, does that happen in 1.1 ? Comment 19 says it's doesn't.

Comment 25

[Julien Wajsberg [:julienw]]

It looks like a keyboard bug to me, but Evelyn will try to do a reduced testcase so that we can maybe reassign it.

Comment 26

[Chris Kreinbring [:CKreinbring]]

I checked both ways from comment 19 on the Leo 1.1 mozilla RIL and was unable to repro the defect.  After tapping the ; or enter key, the text becomes enclosed in a bubble and the keyboard stays up.

Build ID: 20131015041203
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/4bfd6a51cd05
Gaia: 680f3b86b1e4ff1411ece6ba397b8b0e56b4b31c
Platform Version: 18.1

Comment 27

[Rick Waldron [:rwaldron]]

gnarf, evhan55 and I worked on this bug together today, the following is STR for our findings:

1. there are two "inputType" events that are both `event.detail.type = "inputmethod-contextchange"`
2. event.detail.inputType = "blur", event.detail.inputType = "textarea"
3. recipient entry 1: type "ssss"<enter>; a event.detail.inputType = "blur" is dispatched
4. recipient entry 2: type "aaaa"<enter>; a event.detail.inputType = "textarea" is dispatched
5. recipient entry 3: type "dddd"<enter>; a event.detail.inputType = "blur" is dispatched
6. recipient entry 4: type "ffff"<enter>; a event.detail.inputType = "textarea" is dispatched

- on all: "focus" is dispatched on an element created after <enter>
- on the odds: the keyboard closes
- on the evens: the keyboard stays open
- now we've traced it into Keyboard.jsm

Comment 28

[Corey Frang [:gnarf]]

There definitely seems to be a bug somewhere in the keyboard related gecko code changes between 1.1 and 1.2 that cause this problem.  Changing focus from one contenteditable to another somehow gets "lost" as a blur event with no following "textarea" event coming through to keyboard_manager.

Fabrice, any chance you know who might be able to help us track this issue down?

Comment 29

[[:fabrice] Fabrice Desré]

Yuan, can you take a look?

Comment 30

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

It seems a duplicated bug of 917048.

Comment 31

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

This bug is caused by the side effect of bug 902942, which will generate a blur event if the focused element is removed.

When adding recipients by typing ';', we move the focus from the old element to a new element, but we don't get a blur event when the old element loses its focus. So we assume the focus event from the second is duplicate and still regard the old element as the focused one. When the old element is removed from DOM tree, the patch of bug 902942 generates a blur event mistakenly and makes the keyboard hide.

Comment 32

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

Attachment: gecko patch v1

Please refer to the above comment(Comment 31) about the cause of the bug. The patch fixes the bug by keeping the focused element up to date so that we won't generate a blur event mistakenly if a previously focused element has been removed.

Comment 33

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

https://tbpl.mozilla.org/?tree=Try&rev=5b928757ccbb

Comment 34

[[:fabrice] Fabrice Desré]

Yuan, do we have tests covering that? If not, we need to get some.

Comment 35

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

It seems not. I need gaia guy to help create a test on gaia side.
RudyL, could you help?

Comment 36

[[:fabrice] Fabrice Desré]

(In reply to Yuan Xulei [:yxl] from comment #35)
> It seems not. I need gaia guy to help create a test on gaia side.
> RudyL, could you help?

Can't we use a mochitest here?

Comment 37

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

Yes, but not easy and kind of hacking. If using mochitest, we need to mimic an unexpected behavior of the SMS input field that is changing the focused element without firing a blur event. The test case is too limited to cover nothing if we change the implementation of the SMS input field in the future and cannot cover other cases that cause keyboard failure either.

So I suggest a test on gaia side for the SMS app.

Comment 39

[Julien Wajsberg [:julienw]]

Yuan, I think a mochitest is useful here, to cover your new code.

Sadly we don't do integration tests for the SMS app yet. The first test is always the longest to do, I'd be glad if someone write it, but I won't request it from you. That said, I agree with you it would be welcome.

Comment 40

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

We could add integration test for testing keyboard and SMS app coop with this bug: Bug 927139.

Comment 42

[[:fabrice] Fabrice Desré]

Comment on attachment 820186 [details] [diff] [review]
gecko patch v1

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

r=me once a bug is filed to add tests.

Comment 43

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

Fabrice, thanks for the review. Sorry for late response to the mochitest and bug 932145 is filed to add tests.

Comment 44

[Ryan VanderMeulen [:RyanVM]]

IIUC, the Gaia PR linked above does not need to land. Feel free to needinfo me if I've got that wrong.

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

Comment 45

[Ryan VanderMeulen [:RyanVM]]

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

Comment 46

[Julien Wajsberg [:julienw]]

Comment on attachment 814131 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/12713

Ryan, you're right, sorry for not obsoleting this before.

Thanks!

Comment 48

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/95f476363455