[Messages] We should not focus the composer after clicking a notification

VERIFIED FIXED in 2.2 S4 (23jan)

Status

defect
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: julienw, Assigned: azasypkin)

Tracking

({regression})

unspecified
2.2 S4 (23jan)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.2+, b2g-v2.2 verified, b2g-master verified)

Details

(Whiteboard: [p=1])

Attachments

(3 attachments, 2 obsolete attachments)

Reporter

Description

5 years ago
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
Reporter

Comment 1

5 years ago
Posted file github PR (obsolete) —
Tests are not working yet
Assignee: nobody → felash
It's a regression. Works as expected in 2.1 but not in 2.2.
Reporter

Comment 3

5 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)
triage: regression
blocking-b2g: 2.2? → 2.2+
Reporter

Comment 5

5 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

5 years ago
Assignee: felash → nobody
Reporter

Updated

5 years ago
Whiteboard: [p=1]
Reporter

Updated

5 years ago
Target Milestone: --- → 2.2 S4 (23jan)
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!
Flags: needinfo?(jelee)
Flags: needinfo?(fshih)
Posted file GitHub pull request URL (obsolete) —
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

5 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

5 years ago
Posted file alternate github PR
Attachment #8550394 - Flags: feedback+
Reporter

Comment 11

5 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-
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

5 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)
(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

5 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+
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: 5 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
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
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.