1051719 - [SMS] Unable to open the links in SMS application

Status: RESOLVED WORKSFORME

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

Description

[sasikala]

1. Title: Unable to open the links from SMS application while sharing from browser
2. Precondition: Connect wi-fi and should be able to send and receive SMS
3. Tester's Action: 1.Open Browser application
                    2.Add some link 
                    3.select '->' in browser -> while browsing click on share
                    4.Select Messages -> Send the message
                    5.While sending,sent,received -> click on the link

4. Actual Symptom (ENG.) : When user click on the link it is not going to browser. When click on Home button again open the SMS application able to open the link
5. Expected: User should go to the link from SMS at any time

Comment 1

[sasikala]

When we click on the link -> it is going to Browser in background.
If we change the existing code from 
    "share": {
      "href": "/index.html#activity-share",
      "filters": {
        "type": ["image/*", "audio/*", "video/*", "url"],
        "number": {
          "max": 5
        }
       },
      "disposition": "inline",
      "returnValue": true
    }
  to 
    "share": {
      "href": "/index.html#activity-share",
      "filters": {
        "type": ["image/*", "audio/*", "video/*", "url"],
        "number": {
          "max": 5
        }
       },
      "disposition": "window"
    }
this is working fine. But as per discussion with julien in IRC we should not change the manifest.webapp

Comment 2

[Julien Wajsberg [:julienw]]

[Blocking Requested - why for this release]:

What Sasikala did not precise: the link is actually opening in the Browser app, but in background.

[Blocking Requested - why for this release]: seems to be a regression.
I'm not sure whether the regression comes from the fact we now have an inline activity (bug 936729) or if there is something else happening in the System app.


It looks to be an issue in the Window Management subsystem. Here is what happens:

Browser -> ("share": inline activity) -> SMS app -> ("view": ? activity) -> Browser

The "view" activity is not specifying any dispotiion, so I guess the default is "window".

What happens is that the activity works, but the Browser app should be somehow be put in foreground, but I don't know if the inline activity (happening as part of the Browser) should be stopped...


Maybe we can use window.open instead of the "view" activity. But this wouldn't solve the bigger picture problem.

Alive, any advice?

Comment 3

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

Holly!
This is something called circular activity which we don't and couldn't have.

Browser-inline->SMS-window->Browser
The SMS inline activity is rendered inside the container of Browser app,
so it's impossible to show browser app again on top of the sms activity unless the view activity is inline but this is a big change to browser app.

For 2.0,
we could try removing the sms activity once browser app is asked to be opened. Bad UX I know.

Comment 4

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

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #3)
> Holly!
> This is something called circular activity which we don't and couldn't have.
> 
> Browser-inline->SMS-window->Browser
> The SMS inline activity is rendered inside the container of Browser app,
> so it's impossible to show browser app again on top of the sms activity
> unless the view activity is inline but this is a big change to browser app.
> 
> For 2.0,
> we could try removing the sms activity once browser app is asked to be
> opened. Bad UX I know.

Once 2.1 we have the system browser, we could open a new browserWindow for the view request in system app so this won't happen.

Comment 5

[Julien Wajsberg [:julienw]]

For v2.0> I was thinking of it too; we can do it entirely in the SMS app: if we're in an activity, show the same menu than when we close on the "cross".

Jenny, what do you think?

Comment 6

[Jenny Lee]

(In reply to Julien Wajsberg [:julienw] from comment #5)
> For v2.0> I was thinking of it too; we can do it entirely in the SMS app: if
> we're in an activity, show the same menu than when we close on the "cross".
> 
> Jenny, what do you think?

For 2.0 I think it's fine to close the sms activity when user taps on the link that's shared, just make sure we show the fade out transition (like when close on the "cross") so that user would know the sms window is closed. Thanks!

Comment 7

[Julien Wajsberg [:julienw]]

Jenny, just want to make sure: should we show the "save draft/discard draft/cancel" menu if there is some content in the composer?

Comment 8

[Jenny Lee]

I think it's weird to show the dialog right after user tapped on the link, can we save as draft automatically here if there's some content in composer? tks!

Comment 9

[Julien Wajsberg [:julienw]]

Yes we can :)
(in my head, I think we should _always_ save automatically, but make it easier to delete it :p)

Comment 10

[sasikala]

Attachment: Bug_1051719.patch

Hi Julien,

Please find the proposed patch for the issue and give the feedback on this.

As per the analysis,we no need to save the draft ->  because when we input text and click on the link it is automatically saving as draft.

Tested scenarios:
1. a) Share any link from browser to Messaging app -> send the message
   b) while sending click on the link -> the link will be opened

2. a) Open SMS thread -> Input some text
   b) click on the existing link in the thread
   c) this will open the browser with link
   d) Go back to SMS application -> the message entered is shown as draft

Let me know if any information is needed.

Thanks!

Comment 11

[Julien Wajsberg [:julienw]]

Comment on attachment 8471471 [details] [diff] [review]
Bug_1051719.patch

Review of attachment 8471471 [details] [diff] [review]:
-----------------------------------------------------------------

This scenario does not save the draft:

3. a) Share any link from browser to Messaging app -> send the message
   b) input some things in the composer.
   c) click the link

Also, please do a patch for the master branch first.

::: apps/sms/js/link_action_handler.js
@@ +39,4 @@
>            return;
>          }
>  
> +        if (ActivityHandler.isInActivity) {

actually, we don't need this because "leaveActivity" does the check too.

Comment 12

[Jason Smith [:jsmith]]

QA Wanted for branch checks.

Comment 13

[Julien Wajsberg [:julienw]]

Note that this is likely yet another regression from bug 936729 (I should invent an acronym for this).

Comment 14

[Dietrich Ayala (:dietrich)]

Functional regression resulting in un-shippable behavior (a Web operating system that can't open links!), so triage concluded blocking+ for 2.0.

Comment 15

[Duane Dixon [:ddixon]]

Branch Check

(Flame devices tested with 319 MB memory.)

Issue DOES occur in Flame 2.0, Buri 2.0

Actual Results: Tapping on link in Messages App does not open link and take user to Browser app.

Device: Flame 2.0
Build ID: 20140812092813
Gaia: 88e774c606520c9ce349b467866e200e8db31af4
Gecko: d0f5b392dc93
Version: 32.0 (2.0)
Firmware Version: v123
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0
Flame 2.1 Repro? No  (tested with wifi and data strains)
------------------------------------------------------------------------------------
Device: Buri 2.0
Build ID: 20140811110610
Gaia: 1144cdc3a544f81c9bf71598aba1cb67d6c95a29
Gecko: c4768df8f483
Version: 32.0 (2.0)
Firmware Version: v1.2device.cfg
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0
------------------------------------------------------------------------------------
------------------------------------------------------------------------------------
Issue DOES NOT occur in Flame 2.1, 1.4, Buri 2.1, Open_C 2.1

Device: Flame Master
Build ID: 20140812093515
Gaia: 02f778766521b84da0ca3e575bfe35c37f46803d
Gecko: 0c7eb00d3ef6
Version: 34.0a1 (Master)
Firmware Version: v123
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0
------------------------------------------------------------------------------------
-Not Applicable-
User cannot share web address via "Messages".

Device: Flame 1.4
Build ID: 20140811081112
Gaia: d47fecdcaa4aefb754561114edd22fb23a92b98d
Gecko: fbee340d0830
Version: 30.0 (1.4)
Firmware Version: v123
User Agent: Mozilla/5.0 (Mobile; rv:30.0) Gecko/30.0 Firefox/30.0
------------------------------------------------------------------------------------
Device: Buri Master
Build ID: 20140810234109
Gaia: 19ed3c9e78eaf234cc08484bde6998ae21552ba5
Gecko: a9b43778f0c2
Version: 34.0a1 (Master)
Firmware Version: v1.2device.cfg
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0
-----------------------------------------------------------------------------------
Device: Open_C Master
Build ID: 20140812132618
Gaia: 9f35fca9d818b26c06aa6b7e5c0bef25886f8f20
Gecko: 7fc96293ada8
Version: 34.0a1 (Master)
Firmware Version: P821A10V1.0.0B06_LOG_DL
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Comment 16

[Julien Wajsberg [:julienw]]

(In reply to Dietrich Ayala (:dietrich) from comment #14)
> Functional regression resulting in un-shippable behavior (a Web operating
> system that can't open links!), so triage concluded blocking+ for 2.0.

This is not exactly true. It generally works. It just doesn't work when we are in an activity that originates from the Browser (so we shared an URL, then we sent the message, and then we clicked on the URL).

I still think it should block, but I wanted to stress it's not as serious than the title implies.

Comment 17

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Alive, are you active working on this or do you need someone else? This showed up on triage because it's unassigned.

Comment 18

[Julien Wajsberg [:julienw]]

No, it's not something that will be fixed in System. This will be picked up by the SMS team in the next sprint (and possibly earlier if we have time).

Comment 19

[Julien Wajsberg [:julienw]]

IMO, the fix here should be:
* move LinkActionHandler code to ThreadUI (the code used to be bigger but now is much simpler and really belongs to ThreadUI)
* when handling the click on a link, save the draft if there is content in the composer, and call "leaveActivity()". Actually I wonder if we should not do it each time we call leaveActivity too, up to the assignee :)

Comment 20

[Steve Chung [:steveck]]

Hi Sasikala, I set the assignee to you since you've created the the patch for it. Feel free to contact us if you have any question about the feedback, thanks.

Comment 21

[sasikala]

Hello Steve & Julien,

The following are the below observations for the issue in below mentioned scenario.

1. From handleEvent() in thread_ui.js the LinkActionHandler.onClick(evt);function is called where we are calling the leaveActivity. But when i am trying to debug the issue -> in the existing fail case it is not entering into handleEvent case at all. :(
2. Even if we add save in draft condition before leaving the activity this is failing as this is not entering into onClick in link_action_handler.

I have already checked adding the below code in 
  if (action === 'url-link') {
    if (!Compose.isEmpty()) {
      ThreadUI.saveDraft({autoSave: true});
      ActivityHandler.leaveActivity();
    }

Please let me know how to proceed in this case.

Thanks!

(In reply to Julien Wajsberg [:julienw] (PTO 08/20 -> 09/15; contact schung instead) from comment #11)
> Comment on attachment 8471471 [details] [diff] [review]
> Bug_1051719.patch
> 
> Review of attachment 8471471 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This scenario does not save the draft:
> 
> 3. a) Share any link from browser to Messaging app -> send the message
>    b) input some things in the composer.
>    c) click the link
> 
> Also, please do a patch for the master branch first.
> 
> ::: apps/sms/js/link_action_handler.js
> @@ +39,4 @@
> >            return;
> >          }
> >  
> > +        if (ActivityHandler.isInActivity) {
> 
> actually, we don't need this because "leaveActivity" does the check too.

Comment 22

[sasikala]

Steve, as discussed on IRC, I am sharing the Work in progress patch. Please check and let me know how to solve the issue mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=1051719#c21(Already discussed the issue in detail with steve on IRC).

Pull request for WIP patch at https://github.com/mozilla-b2g/gaia/pull/23033.

Thanks!

Comment 23

[Duane Dixon [:ddixon]]

Unable to provide Regression Window. 

Issue occurs in the earliest build that the share via Messages feature is implemented. 

First Broken (also the first build that the feature is implemented in). 

Environmental Variables:
Device: Flame Master
BuildID: 20140514163007
Gaia: 3a1d67246a79e3632c3b3f2460a25291e7e2714c
Gecko: e4843f4f08a7
Version: 32.0a1 (Master) 
Firmware Version: v123
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Comment 24

[Steve Chung [:steveck]]

(In reply to sasikala from comment #22)
> Steve, as discussed on IRC, I am sharing the Work in progress patch. Please
> check and let me know how to solve the issue mentioned in
> https://bugzilla.mozilla.org/show_bug.cgi?id=1051719#c21(Already discussed
> the issue in detail with steve on IRC).
> 
> Pull request for WIP patch at https://github.com/mozilla-b2g/gaia/pull/23033.
> 
> Thanks!

I left some comments on your WIP, I tried it and it basically work, not sure why your patch could not triggered the link correctly... And if it's only 2.0 issue since current master could open the new browser page without any problem, I'm fine that keep the link action helper as it just like your first patch. But please also remember to save the draft while leaving activity, thanks.

Comment 25

[sasikala]

Hi steve,

Thank you for the comment. As this is only for 2.0, we will keep the link action helper as it is as per your suggestion. I will update the WIP as per your comments.
Will you plaese let me did the below scenario working fine with the above changes.

3. a) Share any link from browser to Messaging app -> send the message
   b) input some things in the composer.
   c) click the link

Thanks!

Comment 26

[sasikala]

Steve, Modified the existing PR - https://github.com/mozilla-b2g/gaia/pull/23033 and squash is done.
Please verify and kindly provide your feedback on the comments.

Thanks!

Comment 28

[Steve Chung [:steveck]]

Hi sasikala, please see 1039955 and we will let system app to solve the circular activity issue, but in this patch we will still need to save the draft because app does not know when system will kill activity. So please simply save to draft after bug 1039955 landed, thanks.

Comment 29

[sasikala]

Hi Steve, Thank you for response. I have checked the issue along with the patch from bug 1039955 but even after these changes able to reproduce the issue.

(In reply to Steve Chung [:steveck] from comment #28)
> Hi sasikala, please see 1039955 and we will let system app to solve the
> circular activity issue, but in this patch we will still need to save the
> draft because app does not know when system will kill activity. So please
> simply save to draft after bug 1039955 landed, thanks.

Comment 30

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

Hi Sasikala,
Just let you know that this 2.0+ needs to be landed before end of this week.
Do you think it can be done?

Comment 31

[sasikala]

Hello Wesley,

There is some scenario which is failing.If the other scenario works then only we can fix this issue.
Requesting steve, please provide opinion on this and on comment 29.

(In reply to Wesley Huang [:wesley_huang] from comment #30)
> Hi Sasikala,
> Just let you know that this 2.0+ needs to be landed before end of this week.
> Do you think it can be done?

Comment 32

[Steve Chung [:steveck]]

Hi sasikala, after 1039955 landed, message app will be close by system if target activity is same as callee app, but we will still need to save draft like I said in comment 28, so please save the draft before calling dial/browser activity(because they are window activity), and it would be easier to implement that moving the link action handler into ThreadUI.

Comment 33

[sasikala]

Hi Steve,

I have taken the patch and verified the issue but the below scenario is still reproduced even with the 1039955 patch.

3. a) Share any link from browser to Messaging app -> send the message
   b) input some things in the composer.
   c) click the link

I have done the changes for this issue in link_action_handler.js as

  if (!Compose.isEmpty()) {
    ThreadUI.saveDraft({preserve: true, autoSave: true});
    Drafts.store();
  }
  ActivityHandler.leaveActivity();

Here before leaving the activity, if the composer is not empty -> saving the draft and storing the data.

I am not sure about the scenario 3.As this is not fixed even with patch 1039955 need to know how to fix these scenarios.

Please let me know the way to fix this scenario.

Thanks!

Comment 34

[Steve Chung [:steveck]]

Hi Alive, after patch in bug 1039955 applied, we still found some issue in browser case in comment 33, it seems the system only treat the same url as the same callee, so if we are opening a different url in step 3.c , message app will still in the foreground without killed.(Please note it's 2.0 only issue because in master browser will create another instance in 3.c, instead of opening in the original browser app). Not sure if you will solve it on 2.0, but just letting you know it first.

Hi sasikala, since it's still not fixed completely, could you please still create a small patch inly for 2.0 like you said in comment 33? Sorry for the inconvenience...

Comment 35

[sasikala]

Steve, Sorry for late response i will create a patch and share.

Comment 36

[sasikala]

Steve, https://bugzilla.mozilla.org/show_bug.cgi?id=1051719#c10 these scenarios are fixed in latest 2.0 code. I am not sure which changes fixed this but the below mentioned scenario is not fixed even in latest code
3. a) Share any link from browser to Messaging app -> send the message
   b) input some things in the composer.
   c) click the link

So, hope patch which i have done is not needed now.
Please check and let me know if anything needed.
Thanks!

Comment 37

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

I cannot repro it in latest v2.0:

* Open browser app and type http://aa.b.ccc
* Click share and choose message
* type some number in sendee
* Click send
* Click the link

The message activity disappears correctly.

I could append a video.

Comment 38

[Francisco Jordano [:arcturus] [:francisco]]

Asking for QAWANTED in 2.0 to verify last comment from Alive.

Comment 39

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

Duane, 
Can you verify this?

Comment 40

[Duane Dixon [:ddixon]]

(In reply to Wesley Huang [:wesley_huang] from comment #39)
> Duane, 
> Can you verify this?

Confirming that this issue DOES NOT occur in latest 2.0 builds. 

Actual Results: Pressing on shared link in Messages app opens it correctly and dismisses the Messages app correctly. 
 
(Tested with the v123 and v165 base images.)

Device: Flame 2.0
BuildID: 20140912000202
Gaia: 91dd0e596aa7c124dd968e1474b23e7992dc35a1
Gecko: a66168598533
Version: 32.0 (2.0) 
Firmware Version: v123
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Device: Flame 2.0 (KK build)
BuildID: 20140911220254
Gaia: 91dd0e596aa7c124dd968e1474b23e7992dc35a1
Gecko: a66168598533
Version: 32.0 (2.0) 
Firmware Version: v165
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Comment 41

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

Hi Sasikala -

Seems like we cannot reproduce this issue any more with newest 2.0(Comment#40), could you help to verify as well to see if you also cannot reproduce this issue at your end?

Thanks

Vance

Comment 42

[sasikala]

Hi Vance,
I will test and provide the result today.
Thanks!

(In reply to Vance Chen [:vchen][[e-mail]] from comment #41)
> Hi Sasikala -
> 
> Seems like we cannot reproduce this issue any more with newest
> 2.0(Comment#40), could you help to verify as well to see if you also cannot
> reproduce this issue at your end?
> 
> Thanks
> 
> Vance

Comment 43

[Julien Wajsberg [:julienw]]

Yeah, it seems to work fine now. I tried various cases and all work.

Comment 44

[sasikala]

Julien, Did the scenario mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=1051719#c11 also working?

Comment 45

[sasikala]

Vance, Did the scenario mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=1051719#c11 also working?

(In reply to Vance Chen [:vchen][[e-mail]] from comment #41)
> Hi Sasikala -
> 
> Seems like we cannot reproduce this issue any more with newest
> 2.0(Comment#40), could you help to verify as well to see if you also cannot
> reproduce this issue at your end?
> 
> Thanks
> 
> Vance

Comment 46

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

(In reply to sasikala from comment #45)
> Vance, Did the scenario mentioned in
> https://bugzilla.mozilla.org/show_bug.cgi?id=1051719#c11 also working?
> 
> (In reply to Vance Chen [:vchen][[e-mail]] from comment #41)
> > Hi Sasikala -
> > 
> > Seems like we cannot reproduce this issue any more with newest
> > 2.0(Comment#40), could you help to verify as well to see if you also cannot
> > reproduce this issue at your end?
> > 
> > Thanks
> > 
> > Vance

Hi Sasikala -

Cannot reproduce since in the following scenario, cannot launch browser by clicking the link in the message composer


3. a) Share any link from browser to Messaging app -> send the message
   b) input some things in the composer.
   c) click the link


Since the phone will not leave message composer and launch the browser, it will not have the discard message problem mentioned in this bug

Comment 47

[sasikala]

Vance, This issue is reproducible if you are sending a message to new number(thread should not exist for the number which we are sending). 
3. a) Share any link from browser to Messaging app -> send the message
   b) input some things in the composer.
   c) click the link
If open the same thread again add text -> click on the link -> the message is saved in this case.

Will you please mention the steps you are trying for reproducing this issue?
Thanks!

Comment 48

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

(In reply to sasikala from comment #47)
> Vance, This issue is reproducible if you are sending a message to new
> number(thread should not exist for the number which we are sending). 
> 3. a) Share any link from browser to Messaging app -> send the message
>    b) input some things in the composer.
>    c) click the link
> If open the same thread again add text -> click on the link -> the message
> is saved in this case.
> 
> Will you please mention the steps you are trying for reproducing this issue?
> Thanks!

Sorry, Sasikala, my bad, and yes still can reproduce this issue with the above scenario.

Could you help to raise another issue for that since it is a different issue from this one?

Thanks

Vance

Comment 49

[Julien Wajsberg [:julienw]]

I think we may need to implement the same solution as Tarako and save drafts continuously instead of doing it at specific moments.

Comment 50

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

(In reply to Julien Wajsberg [:julienw] from comment #49)
> I think we may need to implement the same solution as Tarako and save drafts
> continuously instead of doing it at specific moments.

Hi Julien -

Since our partner find this bug during their IOT test, could we just uplift the Tarako mechanism to 2.0? Or you can provide the bug number, maybe partner can just cheery-pick into their  branch

Thanks

Vance

Comment 51

[Julien Wajsberg [:julienw]]

Nope, the tarako mechanism was completely ad-hoc because we had no draft support at all in v1.3. It's impossible to apply it in any code from v1.4.

FTR Tarako implementation is bug 989600.

If we need it, please file a separate bug and nominate accordingly.

Comment 52

[sasikala]

Hi Julien and Vance,

I have filed a separate Bug for the issue in comment48 https://bugzilla.mozilla.org/show_bug.cgi?id=1051719#c48
Bugzilla URL: https://bugzilla.mozilla.org/show_bug.cgi?id=1069783

Thanks!