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).

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.

(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?

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.

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

(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.

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

(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 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.

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

Hi Yoshi, Can you set the target milestone?

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.

addressed dimi's comment.

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.

add comments in the WebIDL.

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

(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 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 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 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.

Thanks smaug for the review and providing comments.

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.