815093 - [sms] Compose "new" activities are unable to return to calling app once message is sent

Status: RESOLVED FIXED

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

Description

[Carlos Martínez Toral [:carlosmartinez]]

Tested in unagi device with:

Gaia - 56e45e8
Gecko - 4eaacd3

STR:
1-Open contacts app
2-Open contact details
3-Tap in the send SMS button in contact details
4-Compose the SMS and tap in the send button

Expected result --> You should return to contact details once SMS is sent
Actual result --> You stay on SMS app

Comment 1

[[:fabrice] Fabrice Desré]

I think the bug is in Gaia. Currently the sms app doesn't specify |"returnValue": true| on its activity declaration, so the platform immediatly fire onsuccess but never sends activity-done.

I set returnValue to "true" but then the app puts itself in the background as soon as it launches. This does not happen with other applications using returnValue (eg the wallpaper or the gallery) so I suspect a bug in the sms app.

Comment 2

[David Scravaglieri [:scravag]]

Not a smoketest blocker

Comment 3

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

The application use a regular activity instead of an inline activity afaik so this is expected and I'm unsure why it blocks the release. Re-noming.

Comment 4

[Daniel Coloma:dcoloma]

I've just talked to UX to check what is the current behaviour and the expected one.

CURRENT

1-Open contacts app
2-Open contact details
3-Tap in the send SMS button in contact details
4-Compose the SMS and tap in the send button
5-SMS is sent and the user is kept in the SMS conversation screen
6-The conversation screen shows a "back" button in the top-left corner. 
7-If I click on the back button I am redirected to the SMS thread list.

EXPECTED

1-Open contacts app
2-Open contact details
3-Tap in the send SMS button in contact details
4-Compose the SMS and tap in the send button
5-SMS is sent and the user is kept in the SMS conversation screen
6-The conversation screen shows a "close" button ("X") in the top-left corner and no back button.
7-If I click on the close button SMS is closed and go back to contacts.

It seems like SMS should handle inline activities properly.

Agree this is not a blocker but polish thing.

Comment 5

[Borja Salguero [:borjasalguero]]

More than a bug is a new feature that it's interesting to add. In the specs there is nothing related to the 'inline' activity in SMS, but is for sure a great improvement in the interaction. I would need more info from UX in order to get a design/specs for this screen.
Agree this is a 'nice to have', but not a blocker.

Comment 6

[Daniel Coloma:dcoloma]

(In reply to Borja Salguero [:borjasalguero] from comment #5)
> More than a bug is a new feature that it's interesting to add. In the specs
> there is nothing related to the 'inline' activity in SMS, but is for sure a
> great improvement in the interaction. I would need more info from UX in
> order to get a design/specs for this screen.
> Agree this is a 'nice to have', but not a blocker.

OK, let's forget about this for V1.

Comment 8

[Rafael Rebolleda]

Belatedly and FWIW, I agree.

FWDing this to Ayman, though, to follow up when the time comes.

Comment 9

[Rick Waldron [:rwaldron]]

Any update on this?

Comment 10

[Carlos Martínez Toral [:carlosmartinez]]

I have retested in unagi device with Gecko-3a432e0.Gaia-cb6d912 from v1-train and still the same behaviour that when I opened the bug.

Comment 11

[ayman maat :maat]

ok, so there are a couple of problems that i am seeing here when i test:

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

Both 3.1.1 and 3.1.2 are incorrect bug 78735 opened to deal with this

4) ensure valid content in 'To' field and composes message 
5) select send

**5.1 Expected**
5.1.1 message is sent and user is kept in the message conversation thread so that any message sending error can be observed or the user can continue the conversation
5.1.2 the message thread screen shows a 'back' button in the top-left corner 
5.1.3 upon selection of the back button the user is returned to the screen they were viewing in point 3) above

**5.1 actual**
5.1.1 <correct>
5.1.2 <correct>
5.1.3 <incorrect> upon selecting the back button the user is taken message app's inbox


reference:
build 20130603100120

Comment 13

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #3)
> The application use a regular activity instead of an inline activity afaik
> so this is expected and I'm unsure why it blocks the release. Re-noming.

FYI I had implemented the back function from so-called regular activity to caller in https://bugzilla.mozilla.org/show_bug.cgi?id=909255

Now any activity could go back to caller by postResult or postError. (With returnValue = ture specified in manifest.activity)

Comment 14

[Julien Wajsberg [:julienw]]

So, if I understand correctly Ayman in comment 11 and alive in comment 13:

* when the user presses the "back" arrow after sending the message, it needs to go back to the contacts app using the activity's postResult.

Of course, we need to take care to reset the "activity" in some events (like: the user clicks on a notification which will open another or the same thread. Other possible reseting events ?)

Comment 15

[Julien Wajsberg [:julienw]]

BTW I think that this is not a regression and that we can do this for 1.3 instead of 1.2.

Comment 16

[Caio Lima(:caiolima)]

(In reply to Julien Wajsberg [:julienw] from comment #14)
> So, if I understand correctly Ayman in comment 11 and alive in comment 13:
> 
> * when the user presses the "back" arrow after sending the message, it needs
> to go back to the contacts app using the activity's postResult.
> 
> Of course, we need to take care to reset the "activity" in some events
> (like: the user clicks on a notification which will open another or the same
> thread. Other possible reseting events ?)

Comment 17

[Caio Lima(:caiolima)]

Forget about my last comment It was sent accidentally

Comment 18

[Caio Lima(:caiolima)]

(In reply to Julien Wajsberg [:julienw] from comment #14)
> So, if I understand correctly Ayman in comment 11 and alive in comment 13:
> 
> * when the user presses the "back" arrow after sending the message, it needs
> to go back to the contacts app using the activity's postResult.
> 
> Of course, we need to take care to reset the "activity" in some events
> (like: the user clicks on a notification which will open another or the same
> thread. Other possible reseting events ?)

Julien, Did you test the cases like this?

Comment 19

[Julien Wajsberg [:julienw]]

Sorry Caio, I don't understand your question.

Comment 20

[Caio Lima(:caiolima)]

I was talking about the activity's reset that you mentioned in comment 14.

My ask is to know if you have already tested the case with these events. =)

Comment 21

[David Scravaglieri [:scravag]]

Need UX input to decide if the current behavior should redefined. Renominate if needed.

Comment 22

[ayman maat :maat]

(In reply to David Scravaglieri [:scravag] from comment #21)
> Need UX input to decide if the current behavior should redefined. Renominate
> if needed.

It has always been the intention that 'back' would take the user back to where they came from. Therefore the expected behaviour is as outlined in comment 6 (which Daniel posted this after discussion with me). I will reiterate it here (with a couple of text modification to bring it up to date as the app has evolved since then): 

1-Open contacts app
2-Open contact details
3-Tap in the send Message button in contact details
4-Compose the Message and tap in the send button
5-Message is sent and the user is kept in the Message thread screen
6-The thread screen shows a "close" button ("X") in the top-left corner and no back button.
7-If I click on the close button ("X") message thread is closed and go back to contact detail card.

Current behaviour should be aligned to this, particularly now with reference to Haida. Therefore renominating.

Comment 23

[Julien Wajsberg [:julienw]]

renominating for 1.3, I don't think we should do this in 1.2 as these changes are quite big.

Comment 24

[Caio Lima(:caiolima)]

Guys, as fabrice told on comment 1, I've tested to put "returnValue: true" either and the activity.postResult/postError didn't close the sms app.

As fabrice either reported, this activity opens a new SMS window when the MozActivity is called. This behavior is different from Camera's "pick" activity. I'm trying to understand why it's happening since saturday night, But I have no success.

Could alive maybe try to help us on that? In my investigation, I've noticed that the 'mozContentEvent' 'activity-choice' is trigged. I didn't find where this event is listened. I don't know if I'm going in the right direction...So, I've decided to ask for help =)

Comment 25

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Caio Lima(:caiolima) from comment #24)
> Guys, as fabrice told on comment 1, I've tested to put "returnValue: true"
> either and the activity.postResult/postError didn't close the sms app.

Could you give me your modification?

Comment 26

[(inactive after 6/18) Alive Kuo [:alive]]

Hi there, I am able to return to the contact with this:
https://gist.github.com/alivedise/6885769

Comment 27

[Julien Wajsberg [:julienw]]

Caio, can you handle this starting with Alive's code ?

Comment 28

[Caio Lima(:caiolima)]

Yes. (In reply to Julien Wajsberg [:julienw] (in MozSummit until next monday) from comment #27)
> Caio, can you handle this starting with Alive's code ?

Of course! I'm already working on it. =)

(In reply to Alive Kuo [:alive] from comment #26)
> Hi there, I am able to return to the contact with this:
> https://gist.github.com/alivedise/6885769

Alive, sorry for the delay, but I'm with some problems to run the FFOS these days. I'll check your code here, but I've already tested something like your sample. The difference is because I've used the "postError()" and it didn't work. Sorry to don't post my modification, but I've deleted it by accident.

Comment 29

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Caio Lima(:caiolima) from comment #28)
> Alive, sorry for the delay, but I'm with some problems to run the FFOS these
> days. I'll check your code here, but I've already tested something like your
> sample. The difference is because I've used the "postError()" and it didn't
> work. Sorry to don't post my modification, but I've deleted it by accident.

If postError doesn't work but postResult works, please file a gecko bug and I or fabrice would fix it.

Comment 30

[Caio Lima(:caiolima)]

(In reply to Alive Kuo [:alive] from comment #29)
> (In reply to Caio Lima(:caiolima) from comment #28)
> > Alive, sorry for the delay, but I'm with some problems to run the FFOS these
> > days. I'll check your code here, but I've already tested something like your
> > sample. The difference is because I've used the "postError()" and it didn't
> > work. Sorry to don't post my modification, but I've deleted it by accident.
> 
> If postError doesn't work but postResult works, please file a gecko bug and
> I or fabrice would fix it.

Hye Alive, I'm testing your code on emulator, And 'postResult' didn't work here. I'm using the emulator. 

If it's working in your build, could you send us some info about your build/gaia??

Comment 31

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Caio Lima(:caiolima) from comment #30)
> (In reply to Alive Kuo [:alive] from comment #29)
> > (In reply to Caio Lima(:caiolima) from comment #28)
> Hye Alive, I'm testing your code on emulator, And 'postResult' didn't work
> here. I'm using the emulator. 
> 
> If it's working in your build, could you send us some info about your
> build/gaia??

I'm not using emulator but latest unagi pvt build(gaia master + mozilla-central).

Comment 32

[Caio Lima(:caiolima)]

BTW, I've filed this bug. We need to confirm if it's a Gecko error.

https://bugzilla.mozilla.org/show_bug.cgi?id=925624

I don't understand why when the Messaging App handles the activity, it opens. This behavior is different from Camera 'pick' activity.

Comment 33

[Caio Lima(:caiolima)]

Attachment: bug-share-pr.txt

I couldn't test it on device, but following what Alive told me, the postResult and postError are working on device. So, could anyone confirm it to me?

This way I can keep working on this bug =D

Comment 34

[David Scravaglieri [:scravag]]

This app behavior will be reviewed with Haida

Comment 35

[Caio Lima(:caiolima)]

Ok, I'm waiting it =)

Comment 36

[David Scravaglieri [:scravag]]

per Comment 34, minused for now

Comment 37

[Julien Wajsberg [:julienw]]

Comment on attachment 816187 [details] [review]
bug-share-pr.txt

comments on github,

please ask review when you're ready :)

Comment 38

[Caio Lima(:caiolima)]

I've made some fixes in the code based on your comments. But before the review, I would like to you confirm another strange behavior.

The Activity is being closed if we try to attatch a media.

To reproduce this:

1. Open the Activity(via Contacts App);
2. Click to attatch a photo or video;
3. When the Camera/Galery Activity finish the work, the Messaging Activity redirect to caller application;

The expected behavior is:
When the Camera/Galery Activity finish the work, the Messaging Activity should stay in the front of view stack;

I didn't understand why it's happaning. Alive, could you give us a help on this?

Comment 39

[(inactive after 6/18) Alive Kuo [:alive]]

https://bugzilla.mozilla.org/show_bug.cgi?id=924971 ?

Comment 40

[Caio Lima(:caiolima)]

Hum... I'll verify if it's happening even without my patch. But this bug has a similar behavior.

My doubt is about how the Activity API supports a kind of "Stack of Activity".

Comment 41

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Caio Lima(:caiolima) from comment #40)
> Hum... I'll verify if it's happening even without my patch. But this bug has
> a similar behavior.
> 
> My doubt is about how the Activity API supports a kind of "Stack of
> Activity".

We support one chain + multiple inline activities now. AppA->InlineActivityB->InlineActivityC
It's undetermined what would happen if a WindowActivity is intermediate inserted
Keep InlineActivity when app is switched is ongoing.

No API facing change.

Comment 42

[Julien Wajsberg [:julienw]]

Then we should probably switch to an inline activity instead of a window activity here, what do you think alive ?

Comment 43

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Julien Wajsberg [:julienw] from comment #42)
> Then we should probably switch to an inline activity instead of a window
> activity here, what do you think alive ?

True. Email app uses window activity because it just cannot sync data.
Inline activity is supposed to live in a standalone page so it'd be lightweight and simple to load, also make us closer to in-app navigations.

Comment 44

[Julien Wajsberg [:julienw]]

We won't move to a standalone page in this bug, but hopefully, once I'll finish bug 881469, we'll be more flexible to do it.

Comment 45

[Caio Lima(:caiolima)]

I've changed to inline and tested it. It's working as we want.

I'm working on it yet, but I'll post it here when finish, and you can see how it is.

Comment 46

[Caio Lima(:caiolima)]

Comment on attachment 816187 [details] [review]
bug-share-pr.txt

Updated the manifest to put share/new activities' as inline, mainly because the Messaging Application can open other new inline apps.

Comment 47

[Julien Wajsberg [:julienw]]

(In reply to Caio Lima(:caiolima) from comment #46)

> 
> Updated the manifest to put share/new activities' as inline, mainly because
> the Messaging Application can open other new inline apps.

It's also that now the activity is more an inline (ie: a task "inside" the other app) than a window (ie: switch the context to another app) activity.

Comment 48

[Julien Wajsberg [:julienw]]

Comment on attachment 816187 [details] [review]
bug-share-pr.txt

some more simple comments on github
should be ready next time ;)

thanks !

Comment 49

[Caio Lima(:caiolima)]

Comment on attachment 816187 [details] [review]
bug-share-pr.txt

I've made the changes ;)

Comment 50

[Julien Wajsberg [:julienw]]

Comment on attachment 816187 [details] [review]
bug-share-pr.txt

r=me

thanks a lot !

Comment 51

[Julien Wajsberg [:julienw]]

master: 47b3bdf7a6e9396db6e09ac0453f782953ceda7a

Comment 52

[Borja Salguero [:borjasalguero]]

Julien, I was testing this feature and for me is not working as expected, and furthermore is adding more bugs to the App (actually some parts of the layout are broken with the patch, as the 'back' action in 'new' panel). I've uploaded a video with the STR http://youtu.be/3qQkWeCS5To
Probably we should revert this patch and fix all the issues before merging again, but I would like to check with you first. Wdyt? Thanks!

Comment 53

[Borja Salguero [:borjasalguero]]

Furthermore we need tests for ensuring that when the activity is closed, the UI is restored and everything works as expected. Currently https://github.com/mozilla-b2g/gaia/commit/9d0c6e143c01dc324d036e53d1dd6454e7f74ecd only add tests for changing the UI when opening the activity, but not when closing.

Comment 54

[Julien Wajsberg [:julienw]]

Hey Borja,

there are 2 solutions:
* since it's not on the branch and we have some time, it's probably safe to fix this in a follow-up bug
* we can also back out and land this again with a fixed patch.

In this bug, we didn't make the "share" activity "inline", and therefore we should not have the "cross" instead of the back button for this activity. That's the bug you're seeing. I felt that converting the share activity to an "inline" activity would be more difficult, and also maybe not desirable, and therefore I wanted to leave this for another bug for discussion.

Comment 55

[Borja Salguero [:borjasalguero]]

Hi Julien, 
In this case I think is even better to revert & land, because the solution is not complete, and we are merging a patch which generates more bugs...  IMHO it would be better to work for the entire solution in this bug, and land the entire fix once.

Comment 56

[Julien Wajsberg [:julienw]]

I really think we should not transform the "share" activity to an "inline" activity, and that it should stay a "window" activity. Therefore the solution is complete here imho, but I agree it's buggy.

I'll revert this because you really want it, but we'll fix only the fact that "share" activity should work like before, and we can open another bug to make the "share" activity "inline" (which I think we should not do).

revert master: 576a86aed4df763c6d52bdd9e710864752746c2c

Comment 57

[Caio Lima(:caiolima)]

Sorry guys. It was a lack of attention mine. I'll fix it. And of course it's a bug.

Comment 58

[Caio Lima(:caiolima)]

Attachment: bug-new-pr.txt

Fixe theerror reported by Borja Salguero on comment 52

Comment 59

[Caio Lima(:caiolima)]

Thank you so much for your report, Borja. And sorry agin guys. Now I guess taht it's ok

Comment 60

[Julien Wajsberg [:julienw]]

Comment on attachment 823395 [details] [review]
bug-new-pr.txt

r=me

looks good to me, let's squash and see if Travis is still green :)

Thanks for very quick fix, this is appreciated !

Comment 61

[Caio Lima(:caiolima)]

Julien, I've rebased it =)

Comment 62

[Julien Wajsberg [:julienw]]

master: 32b171e12d6e72598ddfa9137b401a27c0b66a2e

Oh, I thought it was done a long time ago ;)