Description: Cancelling share or successfully sending a message with shared media does not return user to original app. Repro Steps: 1) Updated Buri to Build ID: 20131108004004 2) Enter Gallery and select picture. 3) Choose Messages share option. 4) Either cancel the message or send the message to a recipient and observe. Actual: Both cases, the user stays within the message app and not returned to the original app. Expected: For both cases the user is returned to the original app. Environmental Variables Device: Buri v1.2 COM RIL Build ID: 20131108004004 Gecko: http://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/a886c641a306 Gaia: 4cf40fb30c7b8380ea83ed0d4efe043b8b81737f Platform Version: 26.0 RIL Version: 01.02.00.019.102 Notes: Repro frequency: 5/5, 100% See attached: Logcat This also occurs sharing multiple sources: Music app Video app Gallery- Video Gallery- Pictures Browser- Youtube

This is not a regression but how the SMS app works since the start. This is a "window" activity. In another bug we changed it for the "send sms" activity but I felt it was not as obvious for the "share" activity. I planed to file a bug for this but didn't, so I'm glad this bug was filed for discussion. Flagging Ayman about this.

Bug 802643 is the bug I was talking about in comment 1.

(In reply to Julien Wajsberg [:julienw] from comment #1) > This is not a regression but how the SMS app works since the start. This is > a "window" activity. > > In another bug we changed it for the "send sms" activity but I felt it was > not as obvious for the "share" activity. I planed to file a bug for this but > didn't, so I'm glad this bug was filed for discussion. > > Flagging Ayman about this. I don't think that aligns with the discussion going on in bug 936734. Readding regression flags and resetting component - I think Alive had a strong reason why this was set this way.

Specifically you should look at bug 909255.

I'm going to hold on the nomination however to get clarity on two areas: 1. QA - On a 9/5/2013 build, what's the behavior seen in this bug? 2. Alive - Why didn't bug 909255 change the behavior seen in this bug? What's the difference?

Because we don't use "postResult" in SMS. As I said before, this is how it behaves since the start. I don't mind changing this behavior but I want a discussion with Ayman before this. My view here is that a "share" activity is more a "changing context" activity rather than an "in context" activity, and that's why I'd like this activity to stay as is.

about bug 936734 Jason quotes in comment 3: this is quite different because it involves a "kill the activity app" step. Nothing related here.

(In reply to Julien Wajsberg [:julienw] from comment #6) > Because we don't use "postResult" in SMS. > > As I said before, this is how it behaves since the start. > > I don't mind changing this behavior but I want a discussion with Ayman > before this. My view here is that a "share" activity is more a "changing > context" activity rather than an "in context" activity, and that's why I'd > like this activity to stay as is. Okay - that clarifies the difference then. Anyways, that's not necessarily what other apps are doing with the share activity. In the email app, if you complete the share activity workflow, then you return back to the originating app. I'm guessing what motivated this bug to be filed was the inconsistency in the use of the share activity from the Gallery perspective - which I can agree that the UX is a bit confusing if there's a lack of consistency. I'm redirecting the UX request to the general alias - I think this isn't just a SMS specific discussion, this needs to be discussed across multiple UX team members to understand what makes sense if a user wants to share a picture across multiple Gaia apps.

Jason, comment #8 seems beyond the scope of this bug: this bug as filed is specific to SMS, and does not cover an audit of sharing behavior across apps. I suggest opening a new bug for that if that's what we'll be doing.

Hi Stephany, I think what we need to know is: should all "share" behaviors be "inline" activities, for all apps? That means that as soon as the "share" action is finished (here: click the "cross" that replaced the "back" button, or send the message), we should go back to the application that triggered the "share" behavior. My stance here is that I think a "share" activity sohuld not be inline, but we can definitely change it to achieve consistency first, and revisit it in another bug.

(In reply to Stephany Wilkes from comment #9) > Jason, comment #8 seems beyond the scope of this bug: this bug as filed is > specific to SMS, and does not cover an audit of sharing behavior across > apps. I suggest opening a new bug for that if that's what we'll be doing. I don't think so - that would affect whether we would decide on what the right behavior of the share activity for SMS. Consistency plays an important factor as it ensures that there isn't confusion when users use the same activity for a different app. A change of behavior in the email app on how sharing works could influence a change in workflow for the SMS app potentially. Anyways - the original UX request isn't really answered here - we need to figure out what's the right behavior here for the SMS sharing activity.

I've reviewed this with Francis for both Messages and Email. For consistency in this case, as Julien suggests, Messages should work like Email does today: when the "share" action is finished, the user should return to the application that triggered the "share" behavior (in the original bug filing, the Gallery). This is NOT true for the Back button, though: If the user is in Gallery, wants to share a photo, chooses Messages, and is then in the Message composition window, s/he is one level deep in the Messages app. In that case, the Back button would not be expected to go back to the Gallery. In our documentation, this is how UX expects share activities to work but it's not implemented across all apps. We should get work to change this into the appropriate app team backlogs for 1.3 and 1.4, though some of the Haida work will impact navigation, which would ideally just make everything consistent in the context of the new Haida direction.

Alive, just to be sure here, I think that an activity like what is defined in comment 12 should be "window", and use "postResult" when sending the message. I feel that for an "inline" activity, staying in the app and never returning is wrong. Is that your analysis too? Stephany, to be crystal clear, should the "send sms" activity behave like you're suggesting in comment 12? Currently, what we did in bug 802643 is to make that activity "inline" with the behavior I described in comment 10. Is that correct? Thanks!

Thanks, Julien. If "inline" literally means that as soon as the "share" action is finished, we should go back to the application that triggered the "share" behavior, then yes. And to this point, I believe bug #802643 is correct. But: Please note my comment on the Back button is comment #12, however. Back button is its own entity.

Just as a note as requested in the accompanying email thread that I agree with the specified behaviour in comment 12. Its spot on.

(In reply to Julien Wajsberg [:julienw] from comment #13) > Alive, just to be sure here, I think that an activity like what is defined > in comment 12 should be "window", and use "postResult" when sending the > message. I feel that for an "inline" activity, staying in the app and never > returning is wrong. Is that your analysis too? > > Stephany, to be crystal clear, should the "send sms" activity behave like > you're suggesting in comment 12? Currently, what we did in bug 802643 is to > make that activity "inline" with the behavior I described in comment 10. Is > that correct? > > Thanks! To me, whatever inline/window activity is done(by postResult or postError), we should just display the activity caller. This is my regression in bug 911053.

More detail: bug 911053 introduces ActivityWindow which is the inline activity window wrapper and it has an mozbrowseractivitydone event handler, however I mistakenly remove the handler in window manager for normal AppWindow which is window activity wrapper. But, Window Manager is going to be deprecated in 907013 and I also fixed this problem there in my patch. So that's why I keep this opened. If you guys still needs a quick fix I could provide, too. (One more conflict to myself...)

(In reply to Stephany Wilkes from comment #14) > Thanks, Julien. If "inline" literally means that as soon as the "share" > action is finished, we should go back to the application that triggered the > "share" behavior, then yes. And to this point, I believe bug #802643 is > correct. > > But: Please note my comment on the Back button is comment #12, however. Back > button is its own entity. Uh, I don't see any |postError| in apps/sms...I'd like to know "why not?" Why don't we do postError when back button is clicked?

Alive: does postError go back to the calling app ? I think the UX does not want to go back to the calling app when the user clicks on the back button. (but I'm currently confirming this by offline email). What happens if we never postError or postResult on an activity that has a returnValue? Do we leak objects?

(In reply to Julien Wajsberg [:julienw] from comment #19) > Alive: does postError go back to the calling app ? I think the UX does not > want to go back to the calling app when the user clicks on the back button. > (but I'm currently confirming this by offline email). I wonder why not...the activity is just finished even it's canceled/failed. Why back button cannot bring user "back"? > > What happens if we never postError or postResult on an activity that has a > returnValue? Do we leak objects? What do you mean by leaking objects?

(In reply to Alive Kuo [:alive][NEEDINFO] from comment #20) > (In reply to Julien Wajsberg [:julienw] from comment #19) > > Alive: does postError go back to the calling app ? I think the UX does not > > want to go back to the calling app when the user clicks on the back button. > > (but I'm currently confirming this by offline email). > > I wonder why not...the activity is just finished even it's canceled/failed. > Why back button cannot bring user "back"? Hey, I'm not the UX ;) That's what they want, I confirmed this with them (see comment 12) as this surprised me as well. > > > > > What happens if we never postError or postResult on an activity that has a > > returnValue? Do we leak objects? > > What do you mean by leaking objects? I mean, when there is an activity, I guess the code is keeping some data containing at least the originating app. So if we never "finish" the activity, what happens? Maybe we need a MozActivityRequestHandler.cancel function for this case, that would clean up the activity-related objects. Or maybe it is enough to put the object out of scope so that it's garbage collected?

(In reply to Julien Wajsberg [:julienw] from comment #21) > (In reply to Alive Kuo [:alive][NEEDINFO] from comment #20) > > (In reply to Julien Wajsberg [:julienw] from comment #19) > > > Alive: does postError go back to the calling app ? I think the UX does not > > > want to go back to the calling app when the user clicks on the back button. > > > (but I'm currently confirming this by offline email). > > > > I wonder why not...the activity is just finished even it's canceled/failed. > > Why back button cannot bring user "back"? > > Hey, I'm not the UX ;) That's what they want, I confirmed this with them > (see comment 12) as this surprised me as well. Email app does need this feature because they cannot launched as an inline activity but they want to go back to the sharing app. I really doubt about "what they want". After reading comment 12, I think they care about "back" which is "<" So what we could do, to me, is changing the back button to close button "X" whenever message is launched as an activity. How do you think? And UX? > > > > > > > > > What happens if we never postError or postResult on an activity that has a > > > returnValue? Do we leak objects? > > > > What do you mean by leaking objects? > > I mean, when there is an activity, I guess the code is keeping some data > containing at least the originating app. So if we never "finish" the > activity, what happens? For gaia there's a property linked two appWindow object. We remove the link when one of apps is killed. I don't think this is leaking.

Flagging Francis again. Francis, this one is high priority.

IMHO a Share activity should be "in-line" and the user should be returned to the originating app when it's completed, as Stephany mentioned comment #12. Essentially it's a sub-task and we shouldn't leave the user in the app that's called when the sub-task is finished. I do agree with Ayman (offline) and with Alive in comment #22 that we should swap the "Back" button out with a "Cancel" button when an email composer/SMS conversation view is shown. This is actually more appropriate for a Sub-task flow as it shows that it's separate from the main interaction flow. So basically the current situation we have is this: Share as Message from Gallery open a Message conversation view with "Back" and "Send" button -> "Back" returns the user to the Message list view -> "Send" keeps the user in the Message conversation view My proposal is: Share as Message from Gallery open a Message conversation view with "Cancel" and "Send" button, both of which should return the user to the Gallery app. For consistency's sake, we should do the same when Sharing as an Email - the email compose view should have a "Cancel" and "Send" button, both of which should return the user to the Gallery. With our new sheets model for 1.3, a "Back" button should take the user back within an application history or hierarchy, and should not take the user back between apps. This function is reserved for the app swipe gesture, so we want to make sure that we don't confuse the two, and that's what was meant in Comment #12 above. But if we change the "Back" button to a "Cancel" as Alive suggests, then we avoid this ambiguity.

Thanks, so I think this is in line with what we did for the "send sms" activity. Francis, to confirm this and so that we don't file new bugs later, I'd like you to test the "send sms" activity (pressing the "send sms" button in the Contacts app) and tell me if this is the expected behavior for both the "send sms" and the "share" activity. Thanks!

Redirecting the UX Needinfo Flag to Ayman, so that we can move forward. Ayman, please see comment 25.

(In reply to Julien Wajsberg [:julienw] (PTO until January 6th) from comment #26) > Redirecting the UX Needinfo Flag to Ayman, so that we can move forward. > > Ayman, please see comment 25. On the whole I agree with Francis. He is spot on as usual. I am just going to put the following here as i am going on holiday (YAY!) so will not be able to discuss this with Francis in good time to close this thread. One thing to keep our eye on in the long term is that there is a slight expectation difference between sending and email and sending a messages in that when a user sends an email they do not tend expect an answer back immediately (unless they are my crazy *crazy* ex wife who expects an answer before she has even pressed send) so therefore ‘send’ immediately taking the back to the Gallery makes total sense ..however with messages a response soon from the recipient is kinda expected and therefore we might want to keep the user in the message thread when they send the message in preparation for the ongoing conversation and have ‘cancel’ take them back to the gallery. I am not saying this is how it should be and i am cool also with having ‘send’ from the message app take the user directly back to the gallery, I am just popping the notion here for Francis’s consideration as he closes this conversation. ni? to francis to confirm which way he would like to go with ‘Send’ from the messages app.

Ayman, can you try to contact Francis offline about this?

(In reply to Julien Wajsberg [:julienw] from comment #25) > Thanks, so I think this is in line with what we did for the "send sms" > activity. > > Francis, to confirm this and so that we don't file new bugs later, I'd like > you to test the "send sms" activity (pressing the "send sms" button in the > Contacts app) and tell me if this is the expected behavior for both the > "send sms" and the "share" activity. > > Thanks! Hi Julien, I've finally checked out the SMS activity that you mentioned and confirm that it should follow the same behavior. Sorry it's taken me so long to get around to this. I think Ayman makes some good points - if the user presses "Send" in the case of Share as Message, we should keep the user in the Message thread. The message thread should have a "Close" button which, if pressed, would return the user back to Gallery. So in this regard, it follows the "Send SMS" behavior.

Just to clarify - my proposal for email remains the same. The email compose view should have a "Cancel" and "Send" button, both of which should return the user to the Gallery.

Thanks a lot, I think we'll be able to move forward now! Marking for mentoring right now, I'll try to add some more information later.

Please note that we might need some refactoring in ActivityHandler for this issue, see https://github.com/mozilla-b2g/gaia/pull/16798#discussion_r10388149

Hey Julien, This pull request is more about starting discussion that hopefully will clarify some things for me. I've tried to make minor refactoring inside ActivityHandler related to share\new activities to decouple ThreadUI from managing activities stuff. I still have some doubts about it that I've noted in the pull request.

Comment on attachment 8395542 [details] GitHub pull request URL This looks solid, I'm quite happy with all these changes. I added some comments on the PR, mostly nits.

Comment on attachment 8395542 [details] GitHub pull request URL Thanks for feedback! I've updated pull request with the changes you've suggested.

Hey Ayman, hope you'll be able to answer this. Currently, when sending a message with an activity (from contacts or from gallery), we don't show the draft saving panel if the user taps the cross. Do you think we should propose to save the draft, or should we just discard? Please answer for both sending from contacts and sending from gallery.

Comment on attachment 8395542 [details] GitHub pull request URL added some more comments, but I'm waiting for Ayman's answer to finish the review. I haven't tried on the device yet either.

Actually I've noticed some odd behavior for "share" activity inside browser and on my nexus. When I try to share something from gallery via SMS app sometimes it just switches to SMS thread list view or doesn't switch to SMS app at all - investigating that today, probably will figure out tomorrow. Anyway it will not be landed earlier than next week when I get real device to test :)

(In reply to Julien Wajsberg [:julienw] (away until March 24) from comment #37) > Hey Ayman, hope you'll be able to answer this. > > Currently, when sending a message with an activity (from contacts or from > gallery), we don't show the draft saving panel if the user taps the cross. > Do you think we should propose to save the draft, or should we just discard? > Please answer for both sending from contacts and sending from gallery. Hey Julien, sure i can answer this... We should most certainly provide functionality to save as draft irrespective of where the message composition dialogue has been triggered from be it from inside the message app itself, from the gallery, from contacts or from any other app that will provides a path for a message to be composed.

Comment on attachment 8395542 [details] GitHub pull request URL I left comments on github. Also, as discussed, please try to reproduce the issue you saw with hashchange events on another branch, because there might be a gecko issue here. Please put your new changes in a separate commit so that it's easier to review, and ask review again once you're ready. Thanks for this solid patch !

(In reply to Julien Wajsberg [:julienw] from comment #42) > Comment on attachment 8395542 [details] > GitHub pull request URL > > I left comments on github. > > Also, as discussed, please try to reproduce the issue you saw with > hashchange events on another branch, because there might be a gecko issue > here. > > Please put your new changes in a separate commit so that it's easier to > review, and ask review again once you're ready. > > Thanks for this solid patch ! Thanks for comprehensive review! I've updated my PR with changes you suggested and left few comments\proposition there. Regarding to issue with hashchange, I can't reproduce it anymore. Probably there was something wrong with my local environment, but anyway I'll keep eye on it and raise a red flag if it happens again. Sorry for pushing all changes into single commit, now it's separated, I'll squash it once r+ is achieved.

Comment on attachment 8395542 [details] GitHub pull request URL Asking for feedback first as there are a bunch of propositions to discuss in the PR.

Note: dupe alert (one for SMS app itself and one for the main app that initiated 'share' activity) that one may notice while sharing too big images via SMS is caused by bug 994537.

Comment on attachment 8395542 [details] GitHub pull request URL I've put comments on github, sorry again for the delay, I wanted to be in good disposition to read all this.

Comment on attachment 8395542 [details] GitHub pull request URL Removing the feedback request, please ask review once you're ready :)

Comment on attachment 8395542 [details] GitHub pull request URL (In reply to Julien Wajsberg [:julienw] from comment #47) > Comment on attachment 8395542 [details] > GitHub pull request URL > > Removing the feedback request, please ask review once you're ready :) Ok, I think it's ready for review now :)

Comment on attachment 8395542 [details] GitHub pull request URL Some comments on github, I'd like a new round before r+ but this is in good shape !

Comment on attachment 8395542 [details] GitHub pull request URL Ok, PR is updated :) Please, review new changes. Travis is red, but looks like it's caused by other tests.

Comment on attachment 8395542 [details] GitHub pull request URL r=me thanks ! please sqhash and rebase and then request checkin-needed :) Let's try to have a green travis too, you can restart jobs if necessary. Could be a good idea to file bugs for the issues that make travis red too.

(In reply to Julien Wajsberg [:julienw] from comment #51) > Comment on attachment 8395542 [details] > GitHub pull request URL > > r=me > > thanks ! > > please sqhash and rebase and then request checkin-needed :) > Let's try to have a green travis too, you can restart jobs if necessary. > Could be a good idea to file bugs for the issues that make travis red too. Thanks for review! Squashed and rebased - and now Travis is green!

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