Closed Bug 1033334 Opened 10 years ago Closed 10 years ago

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

Categories

(Firefox OS Graveyard :: Gaia::SMS, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

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

RESOLVED FIXED
2.2 S3 (9jan)
Tracking Status
b2g-v1.3 --- affected
b2g-v1.4 --- affected
b2g-v2.0 --- affected
b2g-v2.1 --- affected

People

(Reporter: gerard-majax, Assigned: ythej, Mentored)

References

Details

(Keywords: dogfood, regression, Whiteboard: [lang=js][sms-papercuts][p=2])

Attachments

(3 files, 1 obsolete file)

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?]
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.
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)
(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)
No news on this one :( It's really bad from a UX point of view.
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)
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)
(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...
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
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
Whiteboard: [lang=js][sms-papercuts] → [lang=js][sms-papercuts][p=2]
Target Milestone: --- → 2.2 S3 (9jan)
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.
Hi kumar rishav, I have created pull request for all those bugs.I am waiting for the review. :) Thanks.
Attached patch bug_1033334.patch (obsolete) (deleted) — Splinter Review
Attachment #8539999 - Flags: review?(felash)
Attached patch bug_1033334.patch (deleted) — Splinter Review
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)
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 :)
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)
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)
Attachment #8541254 - Flags: review?(schung)
Comment on attachment 8541260 [details] 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)
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] 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.
Comment on attachment 8541260 [details] 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] 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+
Blocks: 1116978
No longer blocks: 1116978
Depends on: 1116978
Blocks: 1116978
No longer depends on: 1116978
(In reply to Steve Chung [:steveck] from comment #31) > Comment on attachment 8541260 [details] > 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 :)
Keywords: checkin-needed
In master: 4ceeff19086b2a2955f044ad923dcfa63a293de3
Status: NEW → RESOLVED
Closed: 10 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.

Attachment

General

Created:
Updated:
Size: