1082443 - B2G NFC: event fallback to System app if the foreground app cannot handle it

Status: RESOLVED FIXED

(Firefox OS Graveyard :: NFC, defect)

Description

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

In Bug 1082440 we will dispatch the NFC event to the foregound app(s).

However if the app(s) couldn't handle the NFC event (for example. ontagfound), we should re-dispatch the NFC event to the default handler, i.e. System app through System message or DOM callback, then System app will process the NFC content accordingly.(process Handover or dispatch it to other apps through MozActivity).

Comment 1

[Krzysztof Mioduszewski[:tauzen]]

I'm not sure if this fallback is need. As a user, if I have some NFC app running in the foreground and I scan a tag, I would expect that this tag is handled only by this app. If the app can't handle the tag, it should show some error notification. If I scan a tag with NFC app open and don't see any results I just switch to a different one. I think it might be confusing for the user if he opens one app, tries to scan a tag and some other app will open instead.

Comment 2

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

(In reply to Krzysztof Mioduszewski[:kmioduszewski][:tauzen] from comment #1)
> I'm not sure if this fallback is need. As a user, if I have some NFC app
> running in the foreground and I scan a tag, I would expect that this tag is
> handled only by this app. 
You don't know what's inside the tag so normally you don't know which app to run.

> If the app can't handle the tag, it should show
> some error notification. 
> If I scan a tag with NFC app open and don't see any
> results I just switch to a different one. I think it might be confusing for
> the user if he opens one app, tries to scan a tag and some other app will
> open instead.

That might hurt UX, again user doesn't know what's inside the tag.
So how does user know which app to open?

Comment 3

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

Also the idea of this bug is we do some event bubbling.

We first dispatch to foreground app, and if the app could handle this, it should call event.stopPropagation(), or event.preventDefault(), or return false, something (detail to be discussed though),
otherwise gecko would bubble the event to System app.

Comment 4

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

Hi, smaug
In bug 1082440 we plan to deliever the ontagfound/onpeerfound event to the foreground app.

However if the foreground app cannot handle the content of the tag, for example, the app could only handle type Contacts(stored in vcard), however the NFC Tag stores 'URL' inside, we hope we could re-dispatch the event/System message to System app, so System app could launch Browser to display this URL, without user pressing HOME key to back to Homescreen, and re-tap the tag again.

So I'd propose to implement this in the following way:

1. When we tap a tag, then the foreground app will receive the ontagfound callback. (Bug 1082440).

2. If the app could process the content of the tag, it should call 'evt.stopPropagation()' or 'evt.stopImmediateProgation()' or 'return false in the ontagfound'.

3. After the event handler is called, NFC DOM part will check if 'evt.stopPropagation()' or 'evt.stopImmediateProgation()' has been called or 'ontagfound returns false', then notify the parent process for the result.

4. Parent process receives the result, if app cannot handle this, parent will notify ontaglost to the app, and then re-dispatch System Message (nfc-manager-tech-discovered) or ontagfound to System app.

How do you think about this approach?

Thanks

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Yoshi Huang[:allstars.chh] from comment #4)

> 2. If the app could process the content of the tag, it should call
> 'evt.stopPropagation()' or 'evt.stopImmediateProgation()' or 'return false
> in the ontagfound'.
stop*Propagation() shouldn't affect to the default action of the event.
calling event.preventDefault() (or return false in EventHandler) could prevent default actiona


> 
> 3. After the event handler is called, NFC DOM part will check if
> 'evt.stopPropagation()' or 'evt.stopImmediateProgation()' 
No to these.


has been called or
> 'ontagfound returns false', then notify the parent process for the result.
> 
Just check event.defaultPrevented.
Not, the event must be cancelable.


> How do you think about this approach?
Could work.

Comment 6

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

Hi, smaug
How do I know app returns false in the event handler?

or return false = stopPropagation() + preventDefault(), so I could just check event.defaultPrevented?

Thanks

Comment 7

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

(In reply to Yoshi Huang[:allstars.chh] from comment #6)
> Hi, smaug
> How do I know app returns false in the event handler?
> 
> or return false = stopPropagation() + preventDefault(), so I could just
> check event.defaultPrevented?
> 
smaug replied me on irc
http://mxr.mozilla.org/mozilla-central/source/dom/events/JSEventHandler.cpp#219

Comment 8

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

Attachment: WIP - Patch

Comment 9

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

Comment on attachment 8535354 [details] [diff] [review]
WIP - Patch

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

::: dom/nfc/nsNfc.js
@@ +360,5 @@
>      let appId = this._window.document.nodePrincipal.appId;
>      this._nfcContentHelper.unregisterTargetForPeerReady(appId);
>    },
>  
>    notifyTagFound: function notifyTagFound(sessionToken, event, records) {

I only handle tag case in this version, will also handle peer next time.

@@ +405,5 @@
>      debug("fire ontagfound " + sessionToken);
>      let tagEvent = new this._window.MozNFCTagEvent("tagfound", eventData);
>      this.__DOM_IMPL__.dispatchEvent(tagEvent);
> +
> +    if (!tagEvent.defaultPrevented) {

I should call ontaglost first before calling default handler.

Comment 10

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

triage: feature-b2g:2.2 for it impacts app behavior after we make API privileged.

Comment 11

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

Hi Yoshi,
Can you set the target milestone?

Comment 12

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

Attachment: Patch.

Comment 13

[Dimi Lee [:dimi]]

Comment on attachment 8554343 [details] [diff] [review]
Patch.

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

::: dom/nfc/nsNfc.js
@@ +440,5 @@
> +   * Handles Tag Found event.
> +   *
> +   * returns true if the app could process this event, false otherwise.
> +   */
> +  canHandleTagFound: function canHandleTagFound(sessionToken, tagInfo, ndefInfo, records) {

We can just use |handleTagFound| as function name because |canHandleTagFound| looks like we will
only check if it could handle tag found but actually we do a lot of things here.
same as canHandlePeerFound.

Comment 14

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

Attachment: Patch. v2.

addressed dimi's comment.

Comment 15

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

Comment on attachment 8554373 [details] [diff] [review]
Patch. v2.

add r? to smaug for 
1. Make the event cancellable.
2. Call the default handler for NFC event if defaultPrevented is false.

Comment 16

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

Attachment: Patch. v3.

add comments in the WebIDL.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8554542 [details] [diff] [review]
Patch. v3.


>+    if (isPeerReady) {
>+      eventData.cancelable = true;
>+      eventType = "peerready";
>+    } else {
>+      eventType = "peerfound";
>+    }
Per .webidl documentation peerfound should be cancelable.



>   /**
>    * This event will be fired when a NFCPeer is detected.
>+   * The default action of this event will be re-dispatched to System app
Surely you don't send the default action anywhere.
So please improve the comment. What does System app actually do if event isn't cancelled?

>   /**
>    * Ths event will be fired when a NFCTag is detected.
>+   * The default action of this event will be re-dispatched to System app,
Same here

Comment 18

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

(In reply to Olli Pettay [:smaug] from comment #17)
> Comment on attachment 8554542 [details] [diff] [review]
> Patch. v3.
> 
> 
> >+    if (isPeerReady) {
> >+      eventData.cancelable = true;
> >+      eventType = "peerready";
> >+    } else {
> >+      eventType = "peerfound";
> >+    }
> Per .webidl documentation peerfound should be cancelable.
> 
Yeah, thanks for catching this.

Comment 19

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

Attachment: Patch. v4.

Comment 20

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

Attachment: Patch inter-diff (v3 - v4)

Comment 21

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

Comment on attachment 8555005 [details] [diff] [review]
Patch. v4.

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

Hi Smaug
Thanks for the previous review.
I have corrected the 'cancelable' typo and updated the comments,
could you help to review this again?

Thanks

Comment 22

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

Comment on attachment 8555005 [details] [diff] [review]
Patch. v4.

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

::: dom/nfc/nsINfcContentHelper.idl
@@ +288,5 @@
> +   *        Is this a P2P Session.
> +   * @param records
> +   *        NDEF Records.
> +   */
> +  void callDefaultEventHandler(in DOMString sessionToken,

I'll rename this to
callDefaultFoundHandler.

Comment 23

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

Attachment: Patch. v5.

Comment 24

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

Attachment: Patch v5.

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8555134 [details] [diff] [review]
Patch v5.


>   /**
>-   * Ths event will be fired when a NFCTag is detected.
>+   * Ths event will be fired when a NFCTag is detected.
Ths? Perhaps This ?

 The application has to
>+   * be running on the foreground (Decided by System app) to receive this event.
s/Decided/decided

>+   *
>+   * The default action of this event will be re-dispatched to System app again,
The default action is not something you re-dispatch. You dispatch events, not actions.
"The default action of this event is to dispatch the event in System app and it will run.."
Something like that, in both places.

Comment 26

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

Attachment: Patch. v6.

Thanks smaug for the review and providing comments.

Comment 27

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

https://hg.mozilla.org/integration/b2g-inbound/rev/37aec6d48423

Comment 28

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

Comment on attachment 8555637 [details] [diff] [review]
Patch. v6.

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
This is feature-b2g:2.2+)

User impact if declined:
User will possibly be unable to read the NFC tag.

Testing completed: 
Manually.

Risk to taking this patch (and alternatives if risky): 
No

String or UUID changes made by this patch:
Change UUID in nsINfcContentHelper.idl which is an internal interface.

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/37aec6d48423

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/eac3747cd8a1