[Message] We should keep the focus in composer after sending a message

RESOLVED FIXED in 2.2 S3 (9jan)

Status

Firefox OS
Gaia::SMS
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: gerard, Assigned: ythej, Mentored)

Tracking

({dogfood, regression})

unspecified
2.2 S3 (9jan)
ARM
Gonk (Firefox OS)
dogfood, regression
Dependency tree / graph

Firefox Tracking Flags

(b2g-v1.3 affected, b2g-v1.4 affected, b2g-v2.0 affected, b2g-v2.1 affected)

Details

(Whiteboard: [lang=js][sms-papercuts][p=2])

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
Recent master builds regressed regarding how we handle the focus when sending messages.

As far as I can say, this dates back to a couple of days at most. On current Gecko/Gaia, tapping the send button in the Messages app makes the keyboard losing focus and thus disappearing.

Previously, when tapping the send button we would keep the focus and the keyboard on to easily send another message after.
QA Wanted to get a video.
Keywords: qawanted
Video Link
http://youtu.be/812pxrm2yKg

Keyboard disappearing after send button is tapped.

Environmental Variables
Device: Flame 2.1
Build ID: 20140703045555
Gecko: https://hg.mozilla.org/mozilla-central/rev/ac6960197eb6
Gaia: d7a517f0bde32072f1799e4a47ea34c6d786c287
Platform Version: 33.0a1
Firmware Version: V122
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(jmitchell)
Keywords: qawanted
QA Contact: croesch
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(jmitchell)
QA Wanted to confirm this is working on 2.0.
QA Whiteboard: [QAnalyst-Triage+]
Keywords: qawanted
This bug repro's on: Flame 2.1 Master, Flame 2.0, Flame 1.4, Buri 2.1

Actual Results: Each time a sms is sent, pressing the send button will cause the keyboard to disappear.

Environmental Variables:
Device: Flame Master
Build ID: 20140706091720
Gaia: 93daa354671a698634a3dc661c8c9dcb7d824c31
Gecko: 1dc6b294800d
Version: 33.0a1 (Master)
Firmware Version: v122
-----------------------------------------------
Environmental Variables:
Device: Flame 2.0
Build ID: 20140707041118
Gaia: 60b800bcf758a7a668218634a3957907051b2f80
Gecko: 16545d52beb1
Version: 32.0a2 (2.0)
Firmware Version: v122
-----------------------------------------------
Environmental Variables:
Device: Flame 1.4
Build ID: 20140707075127
Gaia: f2c118767aa26981386570e5d0bed95bab593de5
Gecko: 30ea9808e7d3
Version: 30.0 (1.4)
Firmware Version: v122
-----------------------------------------------
Environmental Variables:
Device: Buri Master
Build ID: 20140707060819
Gaia: 99f56d9db3cd37c684b01de6fed786421f47e2b7
Gecko: 085eea991bb9
Version: 33.0a1 (Master)
Firmware Version: v1.2device.cfg
QA Whiteboard: [QAnalyst-Triage?]
status-b2g-v1.3: --- → affected
status-b2g-v1.4: --- → affected
status-b2g-v2.0: --- → affected
status-b2g-v2.1: --- → affected
Flags: needinfo?(jmitchell)
Keywords: qawanted
(In reply to Alexandre LISSY :gerard-majax from comment #0)
> Recent master builds regressed regarding how we handle the focus when
> sending messages.
> 
> As far as I can say, this dates back to a couple of days at most. On current
> Gecko/Gaia, tapping the send button in the Messages app makes the keyboard
> losing focus and thus disappearing.
> 
> Previously, when tapping the send button we would keep the focus and the
> keyboard on to easily send another message after.


Based on Comment 0, it sounds like this should have been working at some point where the keyboard would stay on screen after tapping send for SMS.

I checked the base build v122 on Flame just to see if i could get the issue to occur way back in early June. However the bug still occurs.

Environmental Variables:
Device: Flame 1.3
Build ID: 20140616171114
Gaia: e1b7152715072d27e0880cdc6b637f82fa42bf4e
Gecko: e181a36ebafaa24e5390db9f597313406edfc794
Version: 28.0 (1.3)
Firmware Version: v122


Jason, if you would like me to check another build, please let me know.
(Reporter)

Comment 6

4 years ago
Cody, this was working previously but I have no idea whether the fix made it in time for v1.3
Flags: needinfo?(croesch)
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(jmitchell)
(In reply to Alexandre LISSY :gerard-majax from comment #6)
> Cody, this was working previously but I have no idea whether the fix made it
> in time for v1.3

Alexandre - What are you asking for here?
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage+][lead-review+]
Flags: needinfo?(croesch) → needinfo?(lissyx+mozillians)
(Reporter)

Comment 8

4 years ago
(In reply to Jason Smith [:jsmith] from comment #7)
> (In reply to Alexandre LISSY :gerard-majax from comment #6)
> > Cody, this was working previously but I have no idea whether the fix made it
> > in time for v1.3
> 
> Alexandre - What are you asking for here?

I'm just documenting that testing an old 1.3 build may not expose the regression, as I think it has landed after. But I don't even know in which bug this was handled.
Flags: needinfo?(lissyx+mozillians)
(Reporter)

Comment 9

3 years ago
No news on this one :( It's really bad from a UX point of view.
Blocks: 949551
See Also: → bug 1097539
Hey Jenny, just want to confirm with you what the wanted behavior is here?

(don't know why this bug falled out of my sight..)
Flags: needinfo?(jelee)
See Also: bug 1097539

Comment 11

3 years ago
Hmm I actually think it's fine to lose keyboard focus once user hits send button, because user's attention is probably fixated on the "sending.." "sent" status shown in the bubble at this moment, and once confirmed sent, leave the app or leave the thread. Compare to "send another message right after", I think the previous behavior is more likely to happen.  So current behavior works for me. Thanks!
Flags: needinfo?(jelee)
(Reporter)

Comment 12

3 years ago
(In reply to Jenny Lee from comment #11)
> Hmm I actually think it's fine to lose keyboard focus once user hits send
> button, because user's attention is probably fixated on the "sending.."
> "sent" status shown in the bubble at this moment, and once confirmed sent,
> leave the app or leave the thread. Compare to "send another message right
> after", I think the previous behavior is more likely to happen.  So current
> behavior works for me. Thanks!

Jenny, you are just making it painful to use the Messages app. I do use it to send a lot of SMS in a row, and I know a lot of people doing so. Loosing focus and this having keyboard hiding/showing constantly is painful and makes me highly relunctant to dogfood seriously: I have been using the SMS app a lot less since this regression happened.

Besides, all other devices I can use (Android ranging from 2.1 to 4.4) do NOT have this behavior, and keep you focused and with keyboard visible.
Flags: needinfo?(jelee)
Moreover, keeping the keyboard does not prevent the user to leave the thread...

Comment 14

3 years ago
Hi Alexandre, 

”Sending SMS in a row“ is most likely to happen *only* when the cost is fairly low to user. But I agree "not losing focus" would benefit users like you, for those who doesn't send multiple SMS one after another, it matters little whether the keyboard stays or not. So let's do what you suggested. Thanks

Hi Julien,

See above for the final decision, thanks!
Flags: needinfo?(jelee)
Thanks Jenny, let's fix this then :)

Adding this bug to list of mentored bugs. I'm not making it a "good first bug" bug because the code for Compose is quite complicated.
Mentor: felash@gmail.com
Whiteboard: [lang=js][sms-papercuts]
Summary: Keyboard focus regression → [Message] We should keep the focus after sending a message
Summary: [Message] We should keep the focus after sending a message → [Message] We should keep the focus in composer after sending a message
Blocks: 1112127
Whiteboard: [lang=js][sms-papercuts] → [lang=js][sms-papercuts][p=2]
Target Milestone: --- → 2.2 S3 (9jan)
(Assignee)

Comment 16

3 years ago
Hi Julien, 

I want to work on this bug.Can you assign it to me?

Thanks :)
Assignee: nobody → yvtheja
I have seen three-fourbugs already assigned to you, which is not fixed yet. Before approaching other bugs, fix these first. Don't worry lots are there. 
Anyway i assigned this to you.
(Assignee)

Comment 18

3 years ago
Hi kumar rishav,

I have created pull request for all those bugs.I am waiting for the review. :)

Thanks.
(Assignee)

Comment 19

3 years ago
Created attachment 8539999 [details] [diff] [review]
bug_1033334.patch

Pull request:
https://github.com/mozilla-b2g/gaia/pull/26938
Attachment #8539999 - Flags: review?(felash)
(Assignee)

Comment 20

3 years ago
Created attachment 8540101 [details] [diff] [review]
bug_1033334.patch

Hi Julien,
sorry that I used wrong command by mistake.Here is the new pull request.
https://github.com/mozilla-b2g/gaia/pull/26946
Attachment #8540101 - Flags: review?(felash)
Attachment #8539999 - Attachment is obsolete: true
Attachment #8539999 - Flags: review?(felash)
Comment on attachment 8540101 [details] [diff] [review]
bug_1033334.patch

Added comments on github; basically, I think the change should be in sendMessage, and I'd like a unit test.

Thanks :)
Attachment #8540101 - Flags: review?(felash)
(Assignee)

Comment 22

3 years ago
Hi Julien,
As you suggested I would make two pull request's, one with change in sendMessagge and other in onSendClick, and will add unit test's too!

Thanks you :)
(Assignee)

Comment 23

3 years ago
Created attachment 8541254 [details] [review]
Pull request when edited in onSendClick

Hi julien,
Here I handled the "Composer empty" case also and wrote the test's in a simple manner.I would edit after we decide editing which function would be better.
Attachment #8541254 - Flags: review?(felash)
(Assignee)

Comment 24

3 years ago
Created attachment 8541260 [details] [review]
Pull request when edited in sendMessage

Hi Julien,
Here I couldn't handle the "Composer empty" case.
Thank you :)
Attachment #8541260 - Flags: review?(felash)
Actually both patches have the exact same behavior on both Buri and Flame, so my preference would go to add the line in sendMessage.

The "Composer empty" case will be done in bug 935060 so maybe this makes no sense to handle it here.

I'm gonna redirect the review requests to Steve because I'm now going in PTO :) Steve, please do what you think is better, I have a preference but it's maybe not the best idea.
Attachment #8541254 - Flags: review?(felash) → review?(schung)
Attachment #8541260 - Flags: review?(felash) → review?(schung)

Updated

3 years ago
Attachment #8541254 - Flags: review?(schung)
Comment on attachment 8541260 [details] [review]
Pull request when edited in sendMessage

I agree with Julien that handling the focus in sendMessage might be better, but there is another scenario that might need focus as well:

1. Entering compose new message , and type some words for sending
2. Send the message, you can see the message sent and seitch to thread view. But the keyboard dismissed after message sent.

Hi Jenny, do you think we should handle this case? The idea might be a little bit different from sending in the same thread, and I'm not sure it's suitable to keep the keyboard while swithing to thread view.
Flags: needinfo?(jelee)

Comment 27

3 years ago
Hi Steve,

I think it's better to keep the behavior consistent, so this case should also be handled. Thanks ;)!
Flags: needinfo?(jelee)
Steve, I think it should be a separate bug:
* because it's likely more complex
* because it's less frequent

What do you think?
Comment on attachment 8541260 [details] [review]
Pull request when edited in sendMessage

Only some suggestion in unit test, so ping me when finished.

About the composer to thread case, it is more complicated because changing hte view will reset the focus. I'm fine to leave this in another follow up, and we might need a integration test for this because simply calling focus could not guarantee the keyboard is displayed in this scenario.
(Assignee)

Comment 30

3 years ago
Comment on attachment 8541260 [details] [review]
Pull request when edited in sendMessage

Hi Steve,
I have added the line in sendMessage and moved the assertion to clickButtonAndSelectSim.

Thank you :)
Flags: needinfo?(schung)
Comment on attachment 8541260 [details] [review]
Pull request when edited in sendMessage

Hi Teja, the fixing is good but you might need git rebase and squash your 2 commits into one for merging(don't be hesitated to ask us if you're not sure how to do). And please add keyword 'checkin-needed' once you finished, thanks!
Flags: needinfo?(schung)
Attachment #8541260 - Flags: review?(schung) → review+

Updated

3 years ago
Blocks: 1116978
Create Bug 1116978 for comment 26
No longer blocks: 1116978

Updated

3 years ago
Depends on: 1116978

Updated

3 years ago
Blocks: 1116978
No longer depends on: 1116978
(Assignee)

Comment 33

3 years ago
(In reply to Steve Chung [:steveck] from comment #31)
> Comment on attachment 8541260 [details] [review]
> Pull request when edited in sendMessage
> 
> Hi Teja, the fixing is good but you might need git rebase and squash your 2
> commits into one for merging(don't be hesitated to ask us if you're not sure
> how to do). And please add keyword 'checkin-needed' once you finished,
> thanks!

Hi steve,
No problem, I have done it before :)
Thanks :)
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
In master: 4ceeff19086b2a2955f044ad923dcfa63a293de3
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Depends on: 1118963
You need to log in before you can comment on or make changes to this bug.