Closed
Bug 1116780
Opened 10 years ago
Closed 10 years ago
[Messages] We should not focus the composer after clicking a notification
Categories
(Firefox OS Graveyard :: Gaia::SMS, defect)
Tracking
(blocking-b2g:2.2+, b2g-v2.2 verified, b2g-master verified)
People
(Reporter: julienw, Assigned: azasypkin)
References
Details
(Keywords: regression, Whiteboard: [p=1])
Attachments
(3 files, 2 obsolete files)
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
Comment 2•10 years ago
|
||
It's a regression. Works as expected in 2.1 but not in 2.2.
Reporter | ||
Comment 3•10 years ago
|
||
Comment on attachment 8542983 [details] [review]
github PR
Hey Oleg, I'd like a first feedback before moving forward.
Attachment #8542983 -
Flags: review?(azasypkin)
Reporter | ||
Comment 5•10 years ago
|
||
Comment on attachment 8542983 [details] [review]
github PR
Now I remember you gave me a feedback already :)
Attachment #8542983 -
Flags: review?(azasypkin)
Reporter | ||
Updated•10 years ago
|
Assignee: felash → nobody
Reporter | ||
Updated•10 years ago
|
Blocks: sms-sprint-2.2S4
Whiteboard: [p=1]
Reporter | ||
Updated•10 years ago
|
Target Milestone: --- → 2.2 S4 (23jan)
Assignee | ||
Comment 6•10 years ago
|
||
Will take it from Julien's PR.
Assignee: nobody → azasypkin
Status: NEW → ASSIGNED
OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → ARM
Assignee | ||
Comment 7•10 years ago
|
||
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!
Flags: needinfo?(jelee)
Flags: needinfo?(fshih)
Assignee | ||
Comment 8•10 years ago
|
||
Hey Julien,
Here's updated PR with support of number-less "new" activity, small refactoring and unit tests.
Attachment #8542983 -
Attachment is obsolete: true
Attachment #8548204 -
Flags: review?(felash)
Reporter | ||
Comment 9•10 years ago
|
||
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.
Reporter | ||
Comment 10•10 years ago
|
||
Attachment #8550394 -
Flags: feedback+
Reporter | ||
Comment 11•10 years ago
|
||
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-
Assignee | ||
Comment 12•10 years ago
|
||
Comment on attachment 8550394 [details] [review]
alternate github PR
Thanks for feedback, Julien!
Could you please review updated PR?
Attachment #8550394 -
Flags: review?(felash)
Comment 13•10 years ago
|
||
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!
Flags: needinfo?(jelee)
Flags: needinfo?(fshih)
Assignee | ||
Comment 14•10 years ago
|
||
(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
Reporter | ||
Comment 15•10 years ago
|
||
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+
Assignee | ||
Comment 16•10 years ago
|
||
Compatible PR for v2.2 branch
Attachment #8548204 -
Attachment is obsolete: true
Assignee | ||
Comment 17•10 years ago
|
||
Thanks for review! Treeherder is finally green.
Master: https://github.com/mozilla-b2g/gaia/commit/decbe1e087594b968cd00600a75d964dcdeae815
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 18•10 years ago
|
||
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?
Updated•10 years ago
|
Attachment #8553593 -
Flags: approval-gaia-v2.2? → approval-gaia-v2.2+
Comment 19•10 years ago
|
||
status-b2g-v2.2:
--- → fixed
status-b2g-master:
--- → fixed
Comment 20•10 years ago
|
||
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
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Updated•10 years ago
|
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
You need to log in
before you can comment on or make changes to this bug.
Description
•