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

VERIFIED FIXED

Status

defect
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: ashiue, Assigned: gduan)

Tracking

unspecified
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(tracking-b2g:backlog, b2g-v2.0 affected, b2g-v2.1 verified)

Details

(Whiteboard: [p=1], [2.0-flame-test-run-3] [priority] [2.1-flame-test-run-1])

Attachments

(1 attachment)

46 bytes, text/x-github-pull-request
alive
: review+
alive
: feedback+
gweng
: feedback+
pdahiya
: feedback+
allstars.chh
: feedback+
Details | Review
Reporter

Description

5 years ago
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
Reporter

Updated

5 years ago
blocking-b2g: --- → 2.0?
Discussed with Greg, he will help to check why shrinking-ui isn't working here.
Flags: needinfo?(gweng)
Leave NI until I can handle this bug. I'm now in LockScreen mode, and need some time to do the context switching.
Flags: needinfo?(gweng)
Flags: needinfo?(gweng)
blocking-b2g: 2.0? → 2.0+
See Also: → 1034405
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.
Flags: needinfo?(gweng) → needinfo?(ashiue)
Reporter

Comment 4

5 years ago
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.
Flags: needinfo?(ashiue)
(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

5 years ago
Assign to Greg now, let's discussion offline if reassign needed, thanks.
Assignee: nobody → 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.
Reporter

Comment 8

5 years ago
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.
Flags: needinfo?(jhuang)
Flags: needinfo?
Reporter

Updated

5 years ago
Flags: needinfo?
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 !
Flags: needinfo?(jhuang)
NI Alive because we've a offline discussion about activity.
Flags: needinfo?(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.
Flags: needinfo?(alive)
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.
Flags: needinfo?(gweng)
Target Milestone: --- → 2.0 S6 (18july)
(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?
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.
What's the UX spec about the suggestion 2? A simple notification when user close it with other devices together?
Flags: needinfo?(gweng)
I think it is a little late to have a new implementation. I wonder if UX could accept comment 14.
Flags: needinfo?(jhuang)
Whiteboard: [p=1]
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.
blocking-b2g: 2.0+ → 2.1?
According to comment 17, remove ni?.
Flags: needinfo?(jhuang)
This bug is no longer in 2.1.
No longer blocks: b2g-NFC-2.0
Reporter

Updated

5 years ago
QA Whiteboard: [COM=NFC]
QA Whiteboard: [COM=NFC] → [COM=NFC], [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Whiteboard: [p=1] → [p=1], [2.0-flame-test-run-3]
Adam, you need to update the tracking flag.
QA Whiteboard: [COM=NFC], [QAnalyst-Triage?] → [COM=NFC], [QAnalyst-Triage-]
Flags: needinfo?(ktucker) → needinfo?(aalldredge)
Flags: needinfo?(aalldredge) → needinfo?(ktucker)
QA Whiteboard: [COM=NFC], [QAnalyst-Triage-] → [COM=NFC], [QAnalyst-Triage+]
Flags: needinfo?(ktucker)

Comment 21

5 years ago
target 2.1

Comment 22

5 years ago
Hit send too fast. NI for Bruce for inline activity discussion planning. Bruce, please check when this can be targeted?
Flags: needinfo?(bhuang)
I recall that UX made decision that it should launch contact app instead of inline activity.
ni? Juwei to reconfirm this.
Flags: needinfo?(jhuang)
QA Whiteboard: [COM=NFC], [QAnalyst-Triage+] → [COM=NFC], [QAnalyst-Triage+][lead-review+]
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.
Flags: needinfo?(jhuang)

Comment 25

5 years ago
feature not blocking, remove blocking flag.
blocking-b2g: 2.1? → ---
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.)
Flags: needinfo?(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
Flags: needinfo?(alive)
Flags: needinfo?(pdahiya)
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.
Flags: needinfo?(alive) → needinfo?(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
Flags: needinfo?(pdahiya)
Blocks: b2g-NFC-2.1
blocking-b2g: --- → backlog
Target Milestone: 2.0 S6 (18july) → ---
(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.
Assignee: gweng → alive
Whiteboard: [p=1], [2.0-flame-test-run-3] → [p=1], [2.0-flame-test-run-3] [priority]
Could you steal and finish the patch here? Thanks.
Flags: needinfo?(gduan)
Assignee: alive → gduan
Flags: needinfo?(gduan)
we should let this.current = AppWindowManager.getActiveApp() , instead of listening to appcreated and get part of information from it.
Posted file 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
Attachment #8473374 - Flags: feedback?(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.
Attachment #8473374 - Flags: feedback?(alive) → feedback+
Comment on attachment 8473374 [details] [review]
PR to master

Hi Greg ,
could you have your feedback on this patch? (comment 33)
Thanks.
Attachment #8473374 - Flags: feedback?(gweng)
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.
Attachment #8473374 - Flags: feedback?(pdahiya)
QA Whiteboard: [COM=NFC], [QAnalyst-Triage+][lead-review+] → [COM=NFC], [QAnalyst-Triage?][lead-review+]
Flags: needinfo?(dharris)
Whiteboard: [p=1], [2.0-flame-test-run-3] [priority] → [p=1], [2.0-flame-test-run-3] [priority] [2.1-flame-test-run-1]
QA Whiteboard: [COM=NFC], [QAnalyst-Triage?][lead-review+] → [COM=NFC], [QAnalyst-Triage+][lead-review+]
Flags: needinfo?(dharris)
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.
Attachment #8473374 - Flags: feedback?(pdahiya) → feedback+
Comment on attachment 8473374 [details] [review]
PR to master

Here comes the 3rd f+. Who should review that?
Attachment #8473374 - Flags: feedback?(gweng) → feedback+
Comment on attachment 8473374 [details] [review]
PR to master

Hi Alive,
I've added tests and addressed issues. Please kindly review this patch, thanks.
Attachment #8473374 - Flags: review?(alive)
Comment on attachment 8473374 [details] [review]
PR to master

Hi Yoshi,
could I have your feedback in gallery/js/open.js ?
Thanks.
Attachment #8473374 - Flags: feedback?(allstars.chh)
Attachment #8473374 - Flags: feedback?(allstars.chh) → feedback+
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.
Attachment #8473374 - Flags: review?(alive) → review+
Thanks,
test looks fine,
master: https://github.com/mozilla-b2g/gaia/commit/0e9ccaa0d9edf6c212c9618c656c14f3d3455bc5
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Reporter

Comment 43

5 years ago
Verified on
Gaia      6e804a42ab90f4251c7fe8c68731dc1c6abd8006
Gecko     https://hg.mozilla.org/mozilla-central/rev/0753f7b93ab7
BuildID   20140826181551
Version   34.0a1
Status: RESOLVED → VERIFIED
Flags: needinfo?(bhuang)
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.