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

Status: VERIFIED FIXED

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

Description

[Julien Wajsberg [:julienw]]

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 1

[Julien Wajsberg [:julienw]]

Attachment: github PR

Tests are not working yet

Comment 2

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

It's a regression. Works as expected in 2.1 but not in 2.2.

Comment 3

[Julien Wajsberg [:julienw]]

Comment on attachment 8542983 [details] [review]
github PR

Hey Oleg, I'd like a first feedback before moving forward.

Comment 4

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

triage: regression

Comment 5

[Julien Wajsberg [:julienw]]

Comment on attachment 8542983 [details] [review]
github PR

Now I remember you gave me a feedback already :)

Comment 6

[Oleg Zasypkin [:azasypkin]]

Will take it from Julien's PR.

Comment 7

[Oleg Zasypkin [:azasypkin]]

Attachment: discard_draft_dialogs.zip

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!

Comment 8

[Oleg Zasypkin [:azasypkin]]

Attachment: GitHub pull request URL

Hey Julien,

Here's updated PR with support of number-less "new" activity, small refactoring and unit tests.

Comment 9

[Julien Wajsberg [:julienw]]

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 10

[Julien Wajsberg [:julienw]]

Attachment: alternate github PR

Comment 11

[Julien Wajsberg [:julienw]]

Comment on attachment 8548204 [details] [review]
GitHub pull request URL

I prefer the other approach :)

I left some comments in github. Thanks !

Comment 12

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8550394 [details] [review]
alternate github PR

Thanks for feedback, Julien!

Could you please review updated PR?

Comment 13

[Jenny Lee]

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!

Comment 14

[Oleg Zasypkin [:azasypkin]]

(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 15

[Julien Wajsberg [:julienw]]

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 :)

Comment 16

[Oleg Zasypkin [:azasypkin]]

Attachment: GitHub pull request URL (v2.2)

Compatible PR for v2.2 branch

Comment 17

[Oleg Zasypkin [:azasypkin]]

Thanks for review! Treeherder is finally green.

Master: https://github.com/mozilla-b2g/gaia/commit/decbe1e087594b968cd00600a75d964dcdeae815

Comment 18

[Oleg Zasypkin [:azasypkin]]

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

Comment 19

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/3737ce78512a3a43aaf060039296919bde204409

Comment 20

[Pi Wei Cheng [:piwei] (inactive)]

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