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

RESOLVED FIXED in Firefox 28, Firefox OS v1.2

Status

()

Core
Event Handling
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: nhirata, Assigned: yxl)

Tracking

({regression})

unspecified
1.2 C3(Oct25)
ARM
Gonk (Firefox OS)
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:koi+, firefox26 wontfix, firefox27 wontfix, firefox28 fixed, b2g-v1.2 fixed)

Details

(Whiteboard: [comms-triaged])

Attachments

(1 attachment, 1 obsolete attachment)

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
asking koi because this is annoying and probably a regression from bug 906581.
blocking-b2g: --- → koi?
Keywords: regression
Julien, were you able to reproduce?
mmm strangely I thought I could reproduce, but right now I can't.
qawanted to confirm reproducibility
Keywords: qawanted
Whiteboard: [comms-triaged]
QA Contact: 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
Keywords: qawanted
Assignee: nobody → waldron.rick
triage: koi+ for regression
blocking-b2g: koi? → koi+
status-b2g-v1.2: --- → affected
Assignee: waldron.rick → evelyn
Taking this from Rick for now.  A good chance to investigate all this plumbing and learn how sms and keyboard function.
(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 :)
(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.
Created 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 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?
Attachment #814131 - Flags: review?(waldron.rick)
Combining bug 924158
Summary: [SMS][keyboard] keyboard disappears after typing ; for the contacts when typing multiple contacts → [Messages] Prevent keyboard from disappearing when recipients are rendered after user input
Duplicate of this bug: 924158
Attachment #814131 - Flags: review?(waldron.rick)
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.
Attachment #814131 - Flags: review?(waldron.rick)
(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.
(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.
(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 ?
(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.
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.
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.
Keywords: regression
(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.
adding Sprint 3 to target milestone. please let us know if this a problem then we can re-discuss
Target Milestone: --- → 1.2 C3(Oct25)
(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
QA, does that happen in 1.1 ? Comment 19 says it's doesn't.
Keywords: qawanted
It looks like a keyboard bug to me, but Evelyn will try to do a reduced testcase so that we can maybe reassign it.
Flags: needinfo?(felash)
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
Keywords: qawanted
Attachment mime type: text/plain → text/x-github-pull-request
Keywords: regression
Target Milestone: 1.2 C3(Oct25) → ---
Target Milestone: --- → 1.2 C3(Oct25)
Flags: needinfo?(gnarf37)
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
Assignee: evelyn → nobody
Component: Gaia::SMS → Event Handling
Product: Boot2Gecko → Core
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?
Flags: needinfo?(gnarf37) → needinfo?(fabrice)
Yuan, can you take a look?
Flags: needinfo?(fabrice) → needinfo?(xyuan)
It seems a duplicated bug of 917048.
Assignee: nobody → xyuan
Status: NEW → ASSIGNED
Flags: needinfo?(xyuan)
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.
Blocks: 902942
Created attachment 820186 [details] [diff] [review]
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.
Attachment #820186 - Flags: review?(fabrice)
Yuan, do we have tests covering that? If not, we need to get some.
It seems not. I need gaia guy to help create a test on gaia side.
RudyL, could you help?
Flags: needinfo?(rlu)
(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?
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.
(Assignee)

Updated

5 years ago
Duplicate of this bug: 917048
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.
We could add integration test for testing keyboard and SMS app coop with this bug: Bug 927139.
Flags: needinfo?(rlu)
Duplicate of this bug: 923262
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.
Attachment #820186 - Flags: review?(fabrice) → review+
Attachment #814131 - Flags: review?(waldron.rick)
(Assignee)

Updated

5 years ago
Depends on: 932145
Fabrice, thanks for the review. Sorry for late response to the mochitest and bug 932145 is filed to add tests.
Keywords: checkin-needed
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e1e249588a1c
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
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!
Attachment #814131 - Attachment is obsolete: true
Duplicate of this bug: 917191
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/95f476363455
status-b2g-v1.2: affected → fixed
status-firefox26: --- → wontfix
status-firefox27: --- → wontfix
status-firefox28: --- → fixed
(Assignee)

Updated

5 years ago
Blocks: 952741
You need to log in before you can comment on or make changes to this bug.