878735 - [SMS / MMS] entering new message screen not rendering correctly when accessed via contacts app

Status: VERIFIED FIXED

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

Description

[ayman maat :maat]

When the user enters new message mode via the contact detail card in the contacts app the new message screen is not rendered correctly:

**PATH** 

1) open contacts app
2) select contact that has a mobile number associated to it
3) select the 'message' icon

**3.1 Expected**
message app opens on new message screen with name of contact whose detail was being viewed in set 3) in 'To' field and curser focus in message field  

**3.1 Actual**
either: 

3.1.1 message app opens on new message screen with 'To' field empty and cursor focus in 'To' field, or
3.1.2 message app opens on new message screen with name of contact whose detail was being viewed in set 3) in 'To' field and cursor focus within the module containing the contacts name in 'To' field

Comment 1

[John Hu [:johnhu][:johu][:醬糊小弟]]

Attachment: patch for this bug.

Move focus to input field while using new activity from contacts app.

Comment 2

[Corey Frang [:gnarf]]

I'm still really concerned about 3.1.1 from the actual section. I haven't been able to reproduce this, but have you seen it loading up to an "empty" recipients entry area?

John, can you do some extra testing to make sure you can't reproduce 3.1.1?

Ayman, I haven't been able to get it to open a new message without a recipient on v1-train, is there some trickery I need to do to get this to happen?

This patch is tiny, and I will r+ it, but I want to make sure we resolve 3.1.1 before we land it, alright john?

Comment 3

[John Hu [:johnhu][:johu][:醬糊小弟]]

Corey, 

I can't reproduce it. I thought that was a bug before, because with code tracking, it should not be happened.

I agreed with you. I can wait until we can make sure 3.1.1 part. Thanks.

Comment 4

[John Hu [:johnhu][:johu][:醬糊小弟]]

Comment on attachment 758406 [details]
patch for this bug.

Remove reviewing state for waiting information from reporter.

Comment 5

[Steve Chung [:steveck]]

Hi all,
After some investigation, the reproduce step for 3.1.1 is :
1) Open the message app and enter the new message page,
2) Switch to contacts app, and view a contact with mobile number,
3) Enter the message app via contact number.

The main reason is message app doesn't handle the open activity properly when app is already opened, it will only stay in the originated page without any action(like adding the number if user is already in the compose page or switch to the compose page if user is in other page). John , you can create another bug for this issue if you have no spare time for that, but it's welcome if you prefer to solve both issues in this bug.

Comment 6

[John Hu [:johnhu][:johu][:醬糊小弟]]

Thanks Steve, I provide more investigation about this bug:

Besides STR 1(provided by Steve), I found another STR 2 which is caused by different reason.

STR 2:
1. open message app in thread list view
2. press home
3. open contacts app and message someone
4. message app shows like 3.1.2
5. back to thread list and create a new empty message
6. press home
7. open contacts app and message someone
8. message app shows like 3.1.1

And more, if we continue doing the following, it has more situation:
9. back to thread list
10. press home
11. open contacts app and message someone
12. message app still shows thread list view.


I had checked the code about this part. The causes of these two STR:
1) STR 1 case => [1] changes window.location.hash to "#new" to open thread view(new message) but the window.location.hash is already equal to "#new". So, the hashchange event will not be triggered and the auto filling part doesn't execute.

2) STR 2 case => There is a lock in activity_handler, MessageManager.activity.isLocked[2]. When message app enters thread view(new message), it is not changed back to false[1]. If there is not theadId in MessageManager.activity, it will not be unlocked[3]. All other activities will not be triggered because of locked state[2]. And more,  STR 9~12 is also caused with the same locked state. In [2], there is a github issue code, but that page is already disappeared.


I think the STR 2 is about activity handling of message app. So, it is nice to create another bug for it.


[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/js/activity_handler.js#L164
[2] https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/js/activity_handler.js#L38
[3] https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/js/message_manager.js#L256

Comment 7

[John Hu [:johnhu][:johu][:醬糊小弟]]

Bug 880115 has been created for STR 2.

I will fix the STR 1 in this patch.

Comment 8

[John Hu [:johnhu][:johu][:醬糊小弟]]

ayman, 

While testing the patch of this bug, I got a situation like this: 
1. open message app and write a new message with recipient chosen and text typed.
2. press home to back home screen
3. use contact app to send a message to somebody

What should we do in this kind of case?? Because we don't support draft, the message and recipients will be overrode, in current implementation.

Comment 9

[ayman maat :maat]

(In reply to John Hu [:johnhu] from comment #8)
> ayman, 
> 
> While testing the patch of this bug, I got a situation like this: 
> 1. open message app and write a new message with recipient chosen and text
> typed.
> 2. press home to back home screen
> 3. use contact app to send a message to somebody
> 
> What should we do in this kind of case?? Because we don't support draft, the
> message and recipients will be overrode, in current implementation.

Good spot. Yes this is one of the big handicaps to not supporting Dafts. 
We are going to do this:

1) open message app
2) select new message
3) enter valid content in the 'to' field
4) enter content in the message field
5) select the home button to navigate to the home screen.
6) open the contacts app
7) select a contact that has a mobile number associated with it
8) select the message CTA in the contact detail card

 **FIX**
9) Upon returning to the messaging app we will display an alert message to the user stating something along the lines of: 'there is an unsent message in the message composer. do you want to send this message or discard it?' ...I will get the copywriter work his magic on this

I am going to produce a set of wireframes for this now as the functionality of back stepping needs to be thought through and articulated clearly.

Comment 10

[John Hu [:johnhu][:johu][:醬糊小弟]]

Comment on attachment 758406 [details]
patch for this bug.

Corey, 

I had added the code to support case 3.1.1 and the prompt message for overriding existing message. Please review it. Thanks.

Comment 11

[Corey Frang [:gnarf]]

I left a few comments on the pull request.  I'd also like to know if bug 880115 is still needed?

Comment 12

[John Hu [:johnhu][:johu][:醬糊小弟]]

Corey, 

Thanks for your comments. I had replied some and will make a little change according to your comments.

I think Bug 880115 is still needed.  This bug is only care about the behavior of new activity of Message app. Because Bug 880115 is dealing with another case about MessageManager.activity.isLocked variable. There is a case which this variable will be true and never goes back to false, see comment 6 or STR of that bug. So, all other new activity will not be executed.

Comment 13

[Corey Frang [:gnarf]]

Comment on attachment 758406 [details]
patch for this bug.

This is good now, just some final tweaks requested for the unit tests.

r=me assuming all the tests still pass!

Comment 14

[ayman maat :maat]

Attachment: HTML5_SMS-MMSUserStorySpecifications_unsent message_20130610_V0.1.pdf

wireframes. reference comment 9

Comment 15

[John Hu [:johnhu][:johu][:醬糊小弟]]

Corey, 

Should I reset the review and request review again after implementing this spec??

Comment 16

[John Hu [:johnhu][:johu][:醬糊小弟]]

ayman, 

I got two problems about this wireframes:

1) the alert menu looks like action menu[1] in building block. It only has a title as message. So, the text will be truncated. If I use confirm[2], it only has two buttons layouted horizontally.

2) the "new message" activity supported by sms app is using window mode without return value[3], it can't go back to the caller app which is the cancel behavior. If I change it to have return value, it may need to be always back to caller or closed.


[1] http://buildingfirefoxos.com/building-blocks/action-menu/
[2] http://buildingfirefoxos.com/building-blocks/confirm/
[3] https://developer.mozilla.org/en-US/docs/WebAPI/Web_Activities

Comment 17

[ayman maat :maat]

John

Ok, well if the situation is that we are constrained by whats in the building blacks we will have to adjust the UX proposal around it.

The key factors influencing the design decision I am taking are:

1) This as being a very temporary solution until we get Draft functionality implemented.
2) In our current situation the message that we show the end user detailing why the alert being shown is King. This is because if the user proceeds down the 'discard message' path they loose information they have entered into the messaging app. This is uncommon behavior compared to our benchmarks and we need to reduce the risk of human error.

In view of this and your point two we can sacrifice the 'Cancel' button and we shall use the Confirm Dialogue Building Block:
http://buildingfirefoxos.com/building-blocks/confirm/

1) The placeholder body copy will be:

	Unsent message
	------------------------- 
	Unsent message in the composer.
	Do you want to send or discard this message?

(I will get the copywriter to wordsmith this as its not great)

2) The two CTAs will be

	| Send |    | discard |

I will update the wireframes accordingly. In the mean time that should be enough for you to proceed.

NeedInfo me if you require further input.

Comment 18

[ayman maat :maat]

NeedInfo to Tyler: Tyler please can you work your magic on the copy detailed in the dialogue on page one of the wireframes attached to this bug.

Ping me if you need to discuss.

Comment 19

[tyler.altes]

Hi,

Here is my suggestion for the alert:

-----------------

Unsent Message

Your previous message was not sent. Do you want to edit the unsent message or discard it and continue?



|  Edit  |  Discard  |

Comment 21

[ayman maat :maat]

Attachment: HTML5_SMS-MMSUserStorySpecifications_unsent message_20130612_V0.2

Attached with reference to comment 17 and 19

this document will be part of wireframe pack HTML5_SMS-MMSUserStorySpecifications V9.0 when published

Comment 22

[John Hu [:johnhu][:johu][:醬糊小弟]]

Comment on attachment 758406 [details]
patch for this bug.

I had followed the ayman's wireframe to use confirm dialog of building block. This changes lots of codes. Please review it again and give me some comments. Thanks in advance.

Comment 23

[Corey Frang [:gnarf]]

Comment on attachment 758406 [details]
patch for this bug.

some comments on the pull - flag again when updated

Comment 24

[John Hu [:johnhu][:johu][:醬糊小弟]]

Comment on attachment 758406 [details]
patch for this bug.

Corey,

I had changed the code. Thanks the review.

Comment 25

[Corey Frang [:gnarf]]

Comment on attachment 758406 [details]
patch for this bug.

r=me

Comment 26

[Corey Frang [:gnarf]]

master: https://github.com/mozilla-b2g/gaia/commit/78b678d8c7e75d343b7127a21ada6baa9b875c1e

Comment 27

[John Ford [:jhford] CET/CEST Berlin Time]

Uplifted 78b678d8c7e75d343b7127a21ada6baa9b875c1e to:
v1-train: f160a09754d46b0c0c1905bb02430dc7c5780b7d

Comment 28

[John Ford [:jhford] CET/CEST Berlin Time]

1.1hd: f160a09754d46b0c0c1905bb02430dc7c5780b7d

Comment 29

[Deepa Subramanian]

When the user taps the "Message icon" on a contact in the Contacts app, SMS app opens up with the "To:" field filled with the selected recipient.

This issue does not repro as per steps given in description. Verifying as fixed. 

Tested on:
Leo Build ID: 20130717070237
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/582e3a7018b0
Gaia: c506c50adaaebcf729ac3c27887ba2931ab79040
Platform Version: 18.1