1033964 - [NFC] Could not share the file which opened via notification

Status: VERIFIED FIXED

(Firefox OS Graveyard :: NFC, defect)

Description

[Alison Shiue]

Gaia      90605754e9bdbe20f3999522f9e1aec600c422a4
Gecko     https://hg.mozilla.org/releases/mozilla-aurora/rev/0bffc7e5d8a2
BuildID   20140702160207
Version   32.0a2

STR:
1. Phone A receive an image via bluetooth
2. Open the image via notification
3. Tap 2 phones together
4. Check the screen

Expected result:
Shrinking UI show correctly and the image can be shared

Actual result:
No shrinking UI so that the image cannot be shared

Comment 1

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Discussed with Greg, he will help to check why shrinking-ui isn't working here.

Comment 2

[Greg Weng [:snowmantw][:gweng][:λ]]

Leave NI until I can handle this bug. I'm now in LockScreen mode, and need some time to do the context switching.

Comment 3

[Greg Weng [:snowmantw][:gweng][:λ]]

I've tried this twice and get different results: the first time I tried it, it got failed as you described. However, the second time I tried it, it success as it should be. And I now suspected that it may be a Blutooth issue because I even encountered Blutooth failure without NFC, and you need to retry it to make the transferring works.

Comment 4

[Alison Shiue]

Hi, this bug is recorded for that we cannot share image which is opened via notification. 
About the BT transfer issue, I will try to reproduce it, thanks.

Comment 5

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Greg Weng [:snowmantw][:gweng][:λ] from comment #3)
> I've tried this twice and get different results: the first time I tried it,
> it got failed as you described.

Did you find anything wrong, or any information can help us to find the root cause or owner for this ?

Comment 6

[howie [:howie]]

Assign to Greg now, let's discussion offline if reassign needed, thanks.

Comment 7

[Greg Weng [:snowmantw][:gweng][:λ]]

The problem is when you click on the image it transferred, the image you opened is not real Gallery app, but a activity window, which would let Shrinking UI still think the current app is the app launched before your clicking. So, it can't be shared.

Comment 8

[Alison Shiue]

Some issues for this design:
1. The activity window screen would mislead user to think this is shareable since they can share image/video in apps.
2. If the current launched app before user click the notification is a contact (another image or browser) which can be shared, and then tap two phones together would show shrink UI (but the screenshot is the activity window screen); after swipe the screen to share, the receiver would feel confused that what he received was not what he saw on sender's screen.

ni? Juwei to look into further.

Comment 9

[Juwei Huang[:juwei]]

I guess in general users won't differentiate activity window or gallery window, they are simply want to share the image right in front of the screen. So I have 3 proposals:

1. Share the screen:
My first though is to make the image shareable, so either we make the activity window shareable or we make the notification access the detail view of gallery app. but I'm not quite sure if it's possible to share the activity window...I need more advices from developers. And, is there any reason that notification only calls out activity window rather than a true app? Is there any constraint that we cannot open an app through notification? 

2. Do not share the screen, but we pops out an error toast to inform users that the screen is not shareable:
I'll consider it as an alternate solution, and I'm wonder if we can implement it on schedule due to L10 translations.

3. Do not share the screen, but visually the activity window should look very different from gallery detail view:
It needs visual design inputs for this solution, but I guess the effort will be in gallery app side.



I think the first solution might be the best one in terms of a long stage experience. The 2nd & 3rd solutions are not fully satisfy the experience we expect....but we still can discuss the feasibility base on the schedule now.
Let me know your thought, thanks !

Comment 10

[Greg Weng [:snowmantw][:gweng][:λ]]

NI Alive because we've a offline discussion about activity.

Comment 11

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

A proposed fix would be:
[System] if we see the activity caller is system app, let's invoke it as window disposition activity.
otherwise we use the activity disposition in its manifest.
[Gallery] Put the system message handler of view in gallery main page also.

Possible issues:
This change in system will affect all inline activities.
Some inline activity may don't want be launched as an app. The system message handler may only lives in its inline page (for example, view.html).
Sometimes system is playing the role of launching activity by the current active app. For example, if we click an image anywhere to show the option menu (this feature is not opened yet), system app will call share activity for the app who has the image, but the caller indeed is the app itself.
https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/browser_context_menu.js#L195

In the open-app event handler of system app in this case we will see the caller is system app but actually it is not.

I dare not to say this is do-able but we need more investigation to the impact it brings us.

Comment 12

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

Hi, I'm setting target milestone to sprint6 in order to meet 0 NFC bugs before FC.
Let me know if you feel uncomfortable at this timeframe.

Comment 13

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

(In reply to Wesley Huang [:wesley_huang] from comment #12)
> Hi, I'm setting target milestone to sprint6 in order to meet 0 NFC bugs
> before FC.
> Let me know if you feel uncomfortable at this timeframe.

Hey, this is a feature request. Due to the risk I don't think it's blocking release.
Could we renom?

Comment 14

[Ken Chang[:kenkai]]

In my opinion, current situation is the second solution that Juwei mentioned. The only difference is that we indicate user the sharing function doesn't work through not shrinking UI. So I want to suggest keeping what we have now, and do long term improvement in future release.

Comment 15

[Greg Weng [:snowmantw][:gweng][:λ]]

What's the UX spec about the suggestion 2? A simple notification when user close it with other devices together?

Comment 16

[Ken Chang[:kenkai]]

I think it is a little late to have a new implementation. I wonder if UX could accept comment 14.

Comment 17

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

I just sat down with Juwei and Greg to play with current 2.0 on Flame.
This actually is a new feature/scenario which we didn't consider.
Based on 2.0 schedule, it's not practical to go for neither localization nor visualization at this moment.
Let's defer it to 2.1 and target on the proposal1 from comment 9. (I'm nominating 2.1? to triage).
There will be efforts on system app, contact app, and also gallery app.

Comment 18

[Ken Chang[:kenkai]]

According to comment 17, remove ni?.

Comment 19

[Ken Chang[:kenkai]]

This bug is no longer in 2.1.

Comment 20

[KTucker [:KTucker][Inactive 3/4/2016]]

Adam, you need to update the tracking flag.

Comment 21

[Sandip Kamat]

target 2.1

Comment 22

[Sandip Kamat]

Hit send too fast. NI for Bruce for inline activity discussion planning. Bruce, please check when this can be targeted?

Comment 23

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

I recall that UX made decision that it should launch contact app instead of inline activity.
ni? Juwei to reconfirm this.

Comment 24

[Juwei Huang[:juwei]]

As an agreement with Wesley & Greg in comment 9, we are now looking for the possibility of solution 1: launch contact app directly from notification.

Comment 25

[howie [:howie]]

feature not blocking, remove blocking flag.

Comment 26

[David Flanagan [:djf]]

If the issue in this bug is just that we need the gallery view activity to handle NFC, in the same way that the gallery handles it, I suspect that is easy to do.  Unless there is an issue where we can't initiate an NFC transfer from inside an inline activity...

Punam: since you've worked on other NFC-related gallery bugs, could you see if you can make Gallery's view activity handler respond to add an onpeerready callback and make it work with NFC? Or maybe even better... I wonder if we could add that directly to MediaFrame.  Then it would work in the camera app's preview mode, too. (I'm asking you to do this with the expectation that it is only a half day's work... If it is more complicated than that, please just report your findings here and we can discuss.)

Comment 27

[Punam Dahiya [:pdahiya]]

Hi David
Please find below PR with onpeerready callback added to open inline activity.
https://github.com/mozilla-b2g/gaia/pull/22296

Here are my findings (after testing the patch and synch up with alive on IRC)
1. The attached patch fixes NFC sharing for open inline activity, if the gallery app is opened before clicking notification to view image.  

2.  System app only sends the event to the foreground 'app'.  If gallery app is not the foreground app, open inline activity to view image doesn't trigger NFC share as it's not receiving peerready event. 

3. If foreground app is browser (or contacts app) and user clicks on notification to view image, on tapping two phones it shrinks the view image UI,  but shares the browser URL (#comment 8).
This behavior is replicable in both master and with the patch and misleading for the user.

In addition to the above PR, we need system app to be able to handle inline activity when pairing NFC.  
Setting NI flag for alive to share the findings and for his inputs on system app support to fix this issue.Thanks

Comment 28

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

Hi Punam, I quickly made a patch to support NFC pairing for inline activity,
https://github.com/mozilla-b2g/gaia/pull/22334

Lemme know if this works with your patch. Thanks.

Comment 29

[Punam Dahiya [:pdahiya]]

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #28)
> Hi Punam, I quickly made a patch to support NFC pairing for inline activity,
> https://github.com/mozilla-b2g/gaia/pull/22334
> 
> Lemme know if this works with your patch. Thanks.

Hi Alive, I tested 22334 PR with my patch on flame with both latest master build and buildid:20140720160202 and it didn't help. With 22334, NFC sharing was not working for gallery, video or browser app.
Thanks

Comment 30

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

(In reply to Punam Dahiya from comment #29)
> (In reply to Alive Kuo [:alive][NEEDINFO!] from comment #28)
> > Hi Punam, I quickly made a patch to support NFC pairing for inline activity,
> > https://github.com/mozilla-b2g/gaia/pull/22334
> > 
> > Lemme know if this works with your patch. Thanks.
> 
> Hi Alive, I tested 22334 PR with my patch on flame with both latest master
> build and buildid:20140720160202 and it didn't help. With 22334, NFC sharing
> was not working for gallery, video or browser app.
> Thanks

OK, will investigate what happens.. sorry for that.

Comment 31

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

Could you steal and finish the patch here? Thanks.

Comment 32

[(Inactive after June) George Duan [:gduan] [:喬智]]

we should let this.current = AppWindowManager.getActiveApp() , instead of listening to appcreated and get part of information from it.

Comment 33

[(Inactive after June) George Duan [:gduan] [:喬智]]

Attachment: PR to master

Hi Alive,
could I have you feedback on this patch before updating tests?
I've changed below items 
1. shrinkingUI.current will refer to AppWindowManager.activeApp()
2. wrapper will refer to activeApp's bottomMost window
3. shrinkingUI will listen activitiyopen & activityterminate to updateActiveApp
4. add onpeerready event in gallery's open.js

Comment 34

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

Comment on attachment 8473374 [details] [review]
PR to master

Works for me, please feedback=greg as well.
And I think we need gallery folks to review the gallery change.

Comment 35

[(Inactive after June) George Duan [:gduan] [:喬智]]

Comment on attachment 8473374 [details] [review]
PR to master

Hi Greg ,
could you have your feedback on this patch? (comment 33)
Thanks.

Comment 36

[(Inactive after June) George Duan [:gduan] [:喬智]]

Comment on attachment 8473374 [details] [review]
PR to master

Hi Punam,
I've added nfc onpeer method in open.js.
could I have your feedback on this patch? 
Thanks.

Comment 37

[Punam Dahiya [:pdahiya]]

Comment on attachment 8473374 [details] [review]
PR to master

Thanks for the patch. I have left comments in the PR on when to set NFC Sharing to false. With those updates, PR  has my feedback+ for gallery changes.

Comment 38

[Greg Weng [:snowmantw][:gweng][:λ]]

Comment on attachment 8473374 [details] [review]
PR to master

Here comes the 3rd f+. Who should review that?

Comment 39

[(Inactive after June) George Duan [:gduan] [:喬智]]

Comment on attachment 8473374 [details] [review]
PR to master

Hi Alive,
I've added tests and addressed issues. Please kindly review this patch, thanks.

Comment 40

[(Inactive after June) George Duan [:gduan] [:喬智]]

Comment on attachment 8473374 [details] [review]
PR to master

Hi Yoshi,
could I have your feedback in gallery/js/open.js ?
Thanks.

Comment 41

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

Comment on attachment 8473374 [details] [review]
PR to master

I would encourage you to use element everywhere instead of frame to be consistent but up to you.

Comment 42

[(Inactive after June) George Duan [:gduan] [:喬智]]

Thanks,
test looks fine,
master: https://github.com/mozilla-b2g/gaia/commit/0e9ccaa0d9edf6c212c9618c656c14f3d3455bc5

Comment 43

[Alison Shiue]

Verified on
Gaia      6e804a42ab90f4251c7fe8c68731dc1c6abd8006
Gecko     https://hg.mozilla.org/mozilla-central/rev/0753f7b93ab7
BuildID   20140826181551
Version   34.0a1