Closed Bug 1116780 Opened 6 years ago Closed 6 years ago
[Messages] We should not focus the composer after clicking a notification
99.63 KB, application/zip
46 bytes, text/x-github-pull-request
|Details | Review|
46 bytes, text/x-github-pull-request
|Details | Review|
STR: 1. Receive a SMS 2. Tap the notification Expected: * the thread for the contact is open, but the composer is not focused Actual: * the composer is focused This is a regression from bug 918970
Tests are not working yet
Assignee: nobody → felash
It's a regression. Works as expected in 2.1 but not in 2.2.
Comment on attachment 8542983 [details] [review] github PR Hey Oleg, I'd like a first feedback before moving forward.
blocking-b2g: 2.2? → 2.2+
Comment on attachment 8542983 [details] [review] github PR Now I remember you gave me a feedback already :)
Will take it from Julien's PR.
Assignee: nobody → azasypkin
Status: NEW → ASSIGNED
OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → ARM
Hey Jenny and Fang, While working on this bug I noticed one UI thing that IMO can be polished\unified, so I need your advice on this. We have two cases when user is prompted to discard existing draft (if any) before user is redirected to another panel: * When user taps on new message notification _and_ messages app is already run and has unsent message we display confirmation about discarding it and opening the one from notification (see "current_discard_draft_on_message_from_notification.png" in the attachment); * When user is in a Thread panel and has unsent message, it's possible to tap on any phone number located inside existing message and choose to send message to this number. In this case we display another confirmation message (see "current.png" in the attachment); Regarding the second case I've attached draft "proposition#1.png" and "proposition#2.png" to make confirmation dialog look similar to other confirmation dialogs we have (changed main text a bit to eliminate "Edit" button, "Cancel" sounds safer and more understandable for me). So do you think we can somehow unify dialogs for both cases like in "proposition#1.png" and "proposition#2.png"? If yes, I'll handle it in a separate bug, just tell me what title and buttons (+ appropriate styles) these dialogs should have? The main text should probably be left untouched though. Thanks!
Hey Julien, Here's updated PR with support of number-less "new" activity, small refactoring and unit tests.
I'm keeping the r? because I haven't tried on a device yet. I'm not really happy with all the "return promise" changes you made for a blocker bug, I'm afraid of regressions. As far as I looked I don't think this has any effect but I'll try to test various cases on a device. I'd rather remove them if they're not needed though.
Comment on attachment 8548204 [details] [review] GitHub pull request URL I prefer the other approach :) I left some comments in github. Thanks !
Attachment #8548204 - Flags: review?(felash) → feedback-
Comment on attachment 8550394 [details] [review] alternate github PR Thanks for feedback, Julien! Could you please review updated PR?
Attachment #8550394 - Flags: review?(felash)
Hi Oleg, I've discussed with Fang and we both think it's a good idea to align the two dialogs. Proposition #2 is better as "Discard" is a destructive move and we should use color red for the button; title and text body both looks good, so feel free to move forward with #2. Thanks for suggesting and sorry for the late response!
(In reply to Jenny Lee from comment #13) > Hi Oleg, > > I've discussed with Fang and we both think it's a good idea to align the two > dialogs. Proposition #2 is better as "Discard" is a destructive move and we > should use color red for the button; title and text body both looks good, so > feel free to move forward with #2. Thanks for suggesting and sorry for the > late response! Thanks for reply, Jenny! Will be probably handled in bug 1122501
Comment on attachment 8550394 [details] [review] alternate github PR r=me, let's land this :) You'll need a rebased patch for master, but maybe you can keep this one to uplift to v2.2. Don't forget to ask approval as well :)
Attachment #8550394 - Flags: review?(felash) → review+
Compatible PR for v2.2 branch
Attachment #8548204 - Attachment is obsolete: true
Thanks for review! Treeherder is finally green. Master: https://github.com/mozilla-b2g/gaia/commit/decbe1e087594b968cd00600a75d964dcdeae815
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Comment on attachment 8553593 [details] [review] GitHub pull request URL (v2.2) [Approval Request Comment] [Bug caused by] (feature/regressing bug #): bug 918970 [User impact] if declined: user will always see keyboard showing when open message from notification + phone-number-less "new message" activities won't work with Messages app [Testing completed]: yes manual and unit [Risk to taking this patch] (and alternatives if risky): relatively low [String changes made]: no
Attachment #8553593 - Flags: approval-gaia-v2.2?
Attachment #8553593 - Flags: approval-gaia-v2.2? → approval-gaia-v2.2+
This issue is verified fixed on Flame 3.0 and on Flame 2.2. Observed behavior: Tapping on an SMS notification brings user to the corresponding thread WITHOUT focusing on the composer and does not bring up the keyboard. Device: Flame 3.0 Master BuildID: 20150211010216 Gaia: 8c7865486a1b11076b849bbf8f7fccbaffbfafe7 Gecko: ee093ca70666 Gonk: e7c90613521145db090dd24147afd5ceb5703190 Version: 38.0a1 (3.0 Master) Firmware Version: v18D-1 User Agent: Mozilla/5.0 (Mobile; rv:38.0) Gecko/38.0 Firefox/38.0 Device: Flame 2.2 BuildID: 20150211002505 Gaia: 943be6fd146017dcd9d4c9d1027be1e43bad13eb Gecko: e614443583e7 Gonk: e7c90613521145db090dd24147afd5ceb5703190 Version: 37.0a2 (2.2) Firmware Version: v18D-1 User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
You need to log in before you can comment on or make changes to this bug.