Closed Bug 815093 Opened 7 years ago Closed 6 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

(blocking-b2g:-)

RESOLVED FIXED
blocking-b2g -

People

(Reporter: carlosmartinez, Assigned: caiolima)

References

Details

(Keywords: polish)

Attachments

(1 file, 1 obsolete file)

46 bytes, text/x-github-pull-request
julienw
: review+
Details | Review
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
blocking-basecamp: --- → ?
Assignee: nobody → fabrice
blocking-basecamp: ? → +
Priority: -- → P2
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.
Assignee: fabrice → nobody
Not a smoketest blocker
Assignee: nobody → fbsc
Keywords: smoketest
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.
blocking-basecamp: + → ?
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.
blocking-basecamp: ? → ---
Keywords: polish
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.
Flags: needinfo?(hello)
OS: Windows 7 → Gonk (Firefox OS)
Hardware: x86_64 → ARM
(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.
Duplicate of this bug: 816930
Belatedly and FWIW, I agree.

FWDing this to Ayman, though, to follow up when the time comes.
Flags: needinfo?(hello) → needinfo?(aymanmaat)
Any update on this?
Flags: needinfo?(carlos.martinez)
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.
Flags: needinfo?(carlos.martinez)
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
Flags: needinfo?(aymanmaat)
Duplicate of this bug: 893770
blocking-b2g: --- → koi?
(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)
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 ?)
BTW I think that this is not a regression and that we can do this for 1.3 instead of 1.2.
(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 ?)
Forget about my last comment It was sent accidentally
(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?
Sorry Caio, I don't understand your question.
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. =)
Need UX input to decide if the current behavior should redefined. Renominate if needed.
blocking-b2g: koi? → -
Flags: needinfo?(aymanmaat)
(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.
Flags: needinfo?(aymanmaat)
renominating for 1.3, I don't think we should do this in 1.2 as these changes are quite big.
blocking-b2g: - → 1.3?
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 =)
(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?
Hi there, I am able to return to the contact with this:
https://gist.github.com/alivedise/6885769
Caio, can you handle this starting with Alive's code ?
Assignee: borja.bugzilla → ticaiolima
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.
(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.
(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??
(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).
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.
Attached file bug-share-pr.txt (obsolete) —
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
Attachment #816187 - Flags: feedback?(felash)
This app behavior will be reviewed with Haida
Ok, I'm waiting it =)
Attachment mime type: text/plain → text/x-github-pull-request
per Comment 34, minused for now
blocking-b2g: 1.3? → -
Comment on attachment 816187 [details] [review]
bug-share-pr.txt

comments on github,

please ask review when you're ready :)
Attachment #816187 - Flags: feedback?(felash) → feedback+
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?
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".
(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.
Then we should probably switch to an inline activity instead of a window activity here, what do you think 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.
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.
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 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.
Attachment #816187 - Flags: review?(felash)
(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 on attachment 816187 [details] [review]
bug-share-pr.txt

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

thanks !
Attachment #816187 - Flags: review?(felash)
Comment on attachment 816187 [details] [review]
bug-share-pr.txt

I've made the changes ;)
Attachment #816187 - Flags: review?(felash)
Summary: [sms] Compose/share activities are unable to return to calling app once message is sent → [sms] Compose "new" activities are unable to return to calling app once message is sent
Comment on attachment 816187 [details] [review]
bug-share-pr.txt

r=me

thanks a lot !
Attachment #816187 - Flags: review?(felash) → review+
master: 47b3bdf7a6e9396db6e09ac0453f782953ceda7a
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
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!
Status: RESOLVED → REOPENED
Flags: needinfo?(felash)
Resolution: FIXED → ---
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.
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.
Flags: needinfo?(felash)
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.
Flags: needinfo?(felash)
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
Flags: needinfo?(felash)
Sorry guys. It was a lack of attention mine. I'll fix it. And of course it's a bug.
Attached file bug-new-pr.txt
Fixe theerror reported by Borja Salguero on comment 52
Attachment #816187 - Attachment is obsolete: true
Attachment #823395 - Flags: review?(felash)
Thank you so much for your report, Borja. And sorry agin guys. Now I guess taht it's ok
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 !
Attachment #823395 - Flags: review?(felash) → review+
Julien, I've rebased it =)
master: 32b171e12d6e72598ddfa9137b401a27c0b66a2e

Oh, I thought it was done a long time ago ;)
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Depends on: 936729
Depends on: 946678
Depends on: 999286
Depends on: 1011654
You need to log in before you can comment on or make changes to this bug.