NFCReadingEvent has an attribute with a type of dictionary?

[leonhsl]

http://w3c.github.io/web-nfc/#dom-nfcreadingevent defines

interface NFCReadingEvent : Event { readonly attribute NDEFMessage message; };

but https://heycam.github.io/webidl/#idl-dictionaries says

Dictionaries must not be used as the type of an attribute or constant.

[leonhsl]

@zolkis @kenchris

[zolkis]

This makes little sense to me in JavaScript. The type is object, with properties as described by NDEFMessage. Do we need to define an interface (constructible from the dictionary) just for the sake of satisfying WebIDL?

We could do it, but I don't think we have an ambiguity for implementation on this :).

The comment also applies to all places with attribute definitions using dictionaries.

Alternatively, we could just say object for type and mention in prose what properties/dictonary are accepted. It would make the WebIDL less intuitive, though.

@anssiko? @tobie? @marcoscaceres?

[leonhsl]

"3.1.2.4 Attribute, Constant, or Exception fields" at https://www.w3.org/TR/api-design/ suggests

To correct it, use type any or object and describe in prose that the attribute takes the type of the dictionary.

EXAMPLE 22

interface HasRightUsage { readonly attribute any dict; };

In your spec, write "dict is of type IAmDictionary dictionary as defined in [REFERENCE]".

Although I'm not sure the document is still valid or not as it seems being last updated in 2012.

[anssiko]

Using object type seems good, any would be also valid but as the union of all IDL types it'd be more ambiguous.

[zolkis]

OK, for now we can use object and on Anssi's advice we can put a comment in Web IDL what object this will be. I can do that change.

However, it would be nice to "fix" Web IDL in this respect.

[leonhsl]

We got some suggestions related to spec from V8 binding expert, " From the binding perspective, I'd suggest to have a spec discussion to change NFCReadingEvent#message to an operation that returns a NDEFMessage dictionary. " with more details of the discussion at https://chromium-review.googlesource.com/c/chromium/src/+/1655092/4/third_party/blink/web_tests/external/wpt/web-nfc/NFCReadingEvent_constructor.https.html#57.

We want to return a V8 object for NFCReadingEvent#message that is actually an IDL dictionary NDEFMessage, the unusual thing is that this V8 object may internally refer to some V8 objects for arbitrary dictionaries (not IDL dictionary, json record data), which triggers V8 crash and is considered kind of discouraging pattern. readonly attribute object message; // NDEFMessage

@zolkis @kenchris @reillyeon

[kenchris]

Maybe we can just do

NDEFMessage ndef();

Similar to https://developer.mozilla.org/en-US/docs/Web/API/Body/json etc

I don't know if there is anything implementation wise that makes this make more sense as a Promise - like can we delay work?

[zolkis]

We could use a similar pattern we used for Web of Thing discovery:

• keep an internal slot/queue in NFCReader to store the read NFCMessage objects until they are consumed (or reach a max size)
• fire an onreading event when we get an NFCMessage
• add a next() function to NFCReader that returns either an NFCMessage or null if the internal queue is empty.

[zolkis]

Alternatively we can replace next(), start() and stop() with a read() function that returns a Promise that resolves with NFCMessage, then we don't need the onreading event, either. We could cancel with an AbortController.

[reillyeon]

read() vs. onreading seems like not too big a difference and the ergonomics of one vs. the other seem to hinge on whether a page will typically enable a reader for multiple tag reads or only one.

To compare further to the Body object. Could we rewrite the IDL like this?

[Constructor(NDEFMessageInit messageInit)]
interface NDEFMessage {
  readonly attribute USVString url;
  readonly attribute FrozenArray<NDEFRecord> records;
}

[Constructor(NDEFRecordInit recordInit)]
interface NDEFRecord {
  readonly attribute NDEFRecordType recordType;
  readonly attribute USVString mediaType;
  [NewObject] DOMString string();
  [NewObject] any json();
  [NewObject] ArrayBuffer data();
}

The disadvantage of using interfaces rather than dictionaries for these types seems to be that it makes constructing messages more difficult as you have to say new NDEFMessage({ ... }) rather than passing a dictionary directly. Then again, maybe we could support passing NDEFMessageInit (renamed from NDEFMessage) directly to methods like push().

[kenchris]

@reillyeon how would you set the data for each record with this API?

[kenchris]

@zolkis > a read() function that returns a Promise that resolves with NFCMessage

Is pretty much like my Promise<NDEFMessage> ndef() idea above

[reillyeon]

@reillyeon how would you set the data for each record with this API?

The new interface types would take the renamed NDEFMessageInit and NDEFRecordInit dictionaries as constructor parameters,

dictionary NDEFMessageInit {
  USVString url;
  sequence<NDEFRecordInit> records;
};

dictionary NDEFRecordInit {
  NDEFRecordType recordType;
  USVString mediaType;
  NDEFRecordData data;
};

Alternatively we could reuse NDEFMessageSource as the constructor parameter type for NDEFMessage because it has already been designed to be a little easier to use with the push() method. In fact, we could have push() optionally take an NDEFMessageInit directly.

All that said it seems like replacing NFCReadingEvent with a method like Promise<NDEFMessage> read() is a much simpler solution.

[kenchris]

Yes, though the event has additional non-NDEF things like potential serial number and might have additional methods in the future to read additional tag specific data regions

[marcoscaceres]

Super late to the discussion here (sorry @zolkis!), but please avoid "object" and "any" for security reasons... I see you already are, but making a general statement (@kenchris, something you should add to the TAG API design guidelines). If you need something "map-like" use a maplike IDL declaration. Otherwise, if the thing looks like a dictionary (where the members are all known), then please use a dictionary.... similarly to what the V8 Folks said, Gecko also prefers well defined Dictionaries and/or Interfaces over object.

[leonhsl]

read() vs. onreading seems like not too big a difference and the ergonomics of one vs. the other seem to hinge on whether a page will typically enable a reader for multiple tag reads or only one.

Do we have any knowledge of which pattern is typically expected from POV of web developers? i.e. read once at a time OR start then just wait for events. I think usability is the most important/basic thing for decision.

If we go with the 'read once a time' pattern, we can just put the serial number back into NDEFMessage? And for any additional tag specific data regions, if they do not necessarily need to be tied with NDEFMessage, we can introduce new methods on NFCReader for getting them?

If we go with the reading event pattern, I'd like to +1 to Reilly's proposal above of changing NDEF{Message, Record} to interface type.

[zolkis]

Thanks @reillyeon, the proposal makes sense to me, though it's a bigger change. So if I get the wishes right, we'd modify NDEFMessage and NDEFRecord, add a data fetching method to NFCReader, and simplify the read event (strip off data which will be read elsewhere). Thanks @marcoscaceres, I saved the comment for future reference :).

[kenchris]

@zolkis I suggest you try to create some code examples of how this would be used, to evaluate if this makes sense in practice. Is this easier for a developer to use, or more complicated?

[zolkis]

Yes, I've been looking at the examples. @reillyeon 's suggestions actually cover a gap in the API, they definitely make it easier to use and I like the synergy between the Fetch API and this proposal (though Fetch is asynchronous for text() and json()). But if if take this in, the spec will be bigger and implementation will need to catch up. Need to make a branch for this exploration.

[kenchris]

Great, examples would help make the spec better as well as serve for MDN later on.

Yes, slightly bigger, so it makes sense to adopt some of the new respec features (look at wake-lock source for examples) - that makes it easier to spec. Also, make sure we don't loose the "serialnumber" feature, or a place to hang future extensions (read the tag specific regions)

[kenchris]

If we go with the 'read once a time' pattern, we can just put the serial number back into NDEFMessage?

No, that would not be good as it is actually not part of NDEF and we would want a place to add future extensions. NFC Tags have different tech so they can have tech specific data regions. We don't support this yet, but would want to in the future.

Some exploration on how to do that would be good - at least for one type so that we know we can add it after MVP. @zolkis ?

Do we have any knowledge of which pattern is typically expected from POV of web developers?

Generic Sensors receive an event, but then have to access the data manually on the object, which is somewhat similar

[kenchris]

(Editable comment) Feel free to update this as we create examples and agree on new API

dictionary NDEFMessageInit {
  USVString url;
  sequence<NDEFRecordInit> records;
};

[Constructor(NDEFMessageInit messageInit)]
interface NDEFMessage {
  readonly attribute USVString url;
  readonly attribute FrozenArray<NDEFRecord> records;
}

dictionary NDEFRecordInit {
  NDEFRecordType recordType;
  USVString mediaType;
  NDEFRecordData data;
};

[Constructor(NDEFRecordInit recordInit)]
interface NDEFRecord {
  readonly attribute NDEFRecordType recordType;
  readonly attribute USVString mediaType;
  [NewObject] DOMString string();
  [NewObject] any json();
  [NewObject] ArrayBuffer data();
}

[Constructor(optional NFCReaderOptions options), SecureContext, Exposed=Window]
interface NFCReader : EventTarget {
  attribute EventHandler onreading;
  attribute EventHandler onerror;

  void start();
  void stop();
};

[Constructor(DOMString type, NFCReadingEventInit readingEventInitDict), SecureContext, Exposed=Window]
interface NFCReadingEvent : Event {
  readonly attribute DOMString serialNumber;
  readonly attribute NDEFMessage ndef; // or message
};

typedef (DOMString or ArrayBuffer or NDEFMessage) NDEFMessageSource;

[Constructor(), SecureContext, Exposed=Window]
interface NFCWriter {
  Promise<void> push(NDEFMessageSource message, optional NFCPushOptions options);
};

[zolkis]

Just for completeness, here is an alternative solution adapted from @reillyeon's suggestion. (editable comment, feel free to update) Note that there is no multiple reads from the same adapter anyway. Subsequential reads from the same adapter could be handled by a loop.

[Constructor(optional NFCReaderOptions options), SecureContext, Exposed=Window]
interface NFCReader : EventTarget {
  attribute EventHandler onerror;
  Promise<NDEFMessage> ndef();
};

Or,

[Constructor(optional NFCReaderOptions options), SecureContext, Exposed=Window]
interface NFCReader : EventTarget {
  attribute EventHandler onerror;
  attribute EventHandler onreading;
  void start();
  void stop();
  NDEFMessage ndef();
};

On a second and third thought, I think the first proposal with the event containing the carried data is just more clear and easier to use. A reader can be constructed, started, stopped, then reused/rewired, started again etc. It's clearer if data is encapsulated, otherwise we'll complicate things with reader internal queue/slots and what to do when rewiring (with the last proposal). With the second proposal, looks like it's not scalable enough to include more NFC tech later, so it's dropped.

[zolkis]

We discussed with @kenchris and would prefer the first proposal. He supports renaming 'message' to 'ndef', it's more natural/clear. For me it's the opposite, since in our terminology we define message and record; this makes clear you expect a message (that contains records). @kenchris argues the same is true with 'ndef'. The floor is open, please help us decide :) @reillyeon ? For me it's almost the same to say event.message.records[0].string() or event.ndef.records[0].string()

[kenchris]

In the future we want to allow lower level I/O - some exploration

dictionary TagMetadata {
  DOMString tagType;
  number transceiveMaxByteLength;
}

dictionary TagFMetadata : TagMetadata {
  UInt8Array manufacturer;
  UInt8Array systemCode;
} 
...

interface NFCReadingEvent : Event {
  readonly attribute DOMString serialNumber;
  readonly attribute NDEFMessage ndef;
  readonly attribute TagMetadata metadata;

  void open();
  void close();
  Promise<Uint8Array> transceive(Uint8Array data);
};

Moved to https://github.com/w3c/web-nfc/issues/238 for people interested

[zolkis]

Looks good. Still related to this issue, would it make sense to rename start() to open() and stop() to close() already now so the only thing we'd need to add is transceive()? [edit] Misread the above, thinking that open() and close() were being added to NFCReader. Disregard the comment. Even if we'd move open() and close() to NFCReader instead of start() and stop(), it would not reflect what happens as faithfully as this (more complex) design.

However, I am starting to be uncomfortable with having so many methods: start() and stop() for NFCReader, then the read methods for NDEFMessage, then open() and close() on the event. Feels overengineered, we need to take a step back and rethink.

[kenchris]

No, these are different. Start/stop is whether to receive read events. When you use transceive (for tags that have their own protocol) then you need to close at the end.

[zolkis]

So when you get a read event for a tech that requires comms, what would event.ndef contain initially? null? Would it make sense to include serialNumber to tagMetadata and include tagType already now?

[kenchris]

I believe that tags may contain NDEF in additional to its own protocol, but it may also not contain ndef at all

[zolkis]

That would be one tag with two tech, app chooses one of them. Android will always choose NDEF. Then it goes with the tech discovered intent. Anyway you're saying that having the ndef property won't harm. But my question is should that be null by default? Then, again, would it make sense to include serialNumber to tagMetadata and include tagType already now? Also to @leonhsl @DonnaWuDongxia

[reillyeon]

While I appreciate the desire for a forward-looking design it seems like this discussion is getting off topic. Let's keep the discussion of low-level I/O on #238 and focus on resolving the problem of the current specification's invalid WebIDL definition of the message property.

The remaining question I have is whether simply defining a json() method on an NDEFRecord interface is sufficient to resolve the problem @leonhsl is having with the Blink implementation. It seems like it may be invalid to round-trip an arbitrary Javascript object through an interface like this. We may have to explicitly define how the data field of input NDEFRecordInit is cloned to create the return value of the proposed json() method.

[leonhsl]

For cloning the json data provided by NDEFRecordInit, I'd have NDEFRecord ctor stringify the json and save the result string, then make json() generate/return a new json object each time by parsing the saved string.

But, the above stringify/parse operation for json data helps me think more about this issue, actually, it can help us still keep the original readonly attribute DEFRecordData data for NDEFRecord interface, if we want. By my understanding, Body provides json(), arrayBuffer(), text(), blob() etc. for getting multiple forms of data from only one given data source, all of them could return meaningful things, while our NDEFRecord seems a different case that the data type for a given NDEFRecord instance is fixed, it can only be one type, either String, or ArrayBuffer, or JSON, i.e. calling json() on a record with array buffer data will just get null. So maybe we'd better still keep readonly attribute DEFRecordData data?

[leonhsl]

To make it clear, changing NDEF{Record,Message} to interface type is still necessary to solve this issue, it enables us to do those code customization mentioned above, i.e. stringify/parse json data.

[reillyeon]

The NDEFRecordData should still be returned by a getter method rather than being an attribute. The reason for that is that it prevents script from trying to modify the object. Each call to data() (or whatever we call it) returns a new object.

[leonhsl]

NDEFRecordData is an IDL union type, which can not be marked as [NewObject](https://heycam.github.io/webidl/#NewObject) or [SameObject](https://heycam.github.io/webidl/#SameObject). In implementation we just return a copy of NDEFRecordData each time, just like handling a String attribute. I'd prepare a CL for review soon.

[leonhsl]

I got 2 CLs that can pass tests now.

https://chromium-review.googlesource.com/c/chromium/src/+/1684664 changes NDEF{Record,Message} to interface type.

dictionary NDEFMessageInit {
  USVString url;
  sequence<NDEFRecordInit> records;
};

[Constructor(NDEFMessageInit messageInit)]
interface NDEFMessage {
  readonly attribute USVString url;
  readonly attribute FrozenArray<NDEFRecord> records;
}

dictionary NDEFRecordInit {
  NDEFRecordType recordType;
  USVString mediaType;
  NDEFRecordData data;
};

[Constructor(NDEFRecordInit recordInit)]
interface NDEFRecord {
  readonly attribute NDEFRecordType recordType;
  readonly attribute USVString mediaType;
  [CallWith=ScriptState] NDEFRecordData data();
};

typedef (DOMString or ArrayBuffer or NDEFMessageInit) NDEFMessageSource;

https://chromium-review.googlesource.com/c/chromium/src/+/1655092 introduces NFCReadingEvent(Init) based on the above CL.

dictionary NFCReadingEventInit : EventInit {
  DOMString? serialNumber = "";
  required NDEFMessageInit message;
};  

[Constructor(DOMString type, NFCReadingEventInit eventInitDict)]
interface NFCReadingEvent : Event {
  readonly attribute DOMString serialNumber;
  [SameObject] readonly attribute NDEFMessage message;
};     

[reillyeon]

This looks good except as commented on your patch it is still invalid for the data attribute to be an NDEFRecordData. This needs to turn into a method.

[leonhsl]

Why do we need to have the NDEFMessageSource union?

typedef (DOMString or ArrayBuffer or NDEFMessage) NDEFMessageSource;

Can we just remove it and use NDEFMessage instead as the parameter to NFCWriter#push()? If users want to push a DOMString or ArrayBuffer, they can just build an NDEFMessage containing a record for the DOMString or ArrayBuffer then feed the NDEFMessage to push(), actually this is just like how our current implementation is internally handling them.

This makes the IDL simpler and easier for users to understand. The current NDEFMessageSource makes people wonder why only DOMString and ArrayBuffer can be fed to push(), but why other objects cannot? like Dictionary etc.

By the way, the current description in the spec http://w3c.github.io/web-nfc/#dom-nfcwriter-push lacks an explanation of how to handle the NDEFMessageSource when it's DOMString or ArrayBuffer.

[zolkis]

The NDEFMessageSource union was used for developer convenience (e.g. push text in a simple one-line code). However, we don't have much input on typically what kind of data would be pushed.

You are right the "convenience" steps are missing from the create Web NFC message steps, IIRC it was there at some point, might somehow been lost between edits.

[leonhsl]

I've updated my previous comment to record the IDLs exactly being implemented by the 2 CLs mentioned there. With great help from @reillyeon the 2 CLs have already got LGTM, Thanks! Now I think it's time for us to decide how to settle down our spec here, any insights/comments/suggestions are appreciated :100:

[leonhsl]

If no objections I'm going to update the spec and land the CLs as described above.

[zolkis]

Let's do that! Thanks!

[kenchris]

By my understanding, Body provides json(), arrayBuffer(), text(), blob() etc. for getting multiple forms of data from only one given data source, all of them could return meaningful things, while our NDEFRecord seems a different case that the data type for a given NDEFRecord instance is fixed, it can only be one type, either String, or ArrayBuffer, or JSON, i.e. calling json() on a record with array buffer data will just get null.

That is pretty much the same case here. JSON is also stored as text and can be read as text() or arrayBuffer() with no problem.

Look at the definitions

JSON

• Return the result of running parse JSON from bytes on bytes.

text

• Return the result of running UTF-8 decode on bytes.

arrayBuffer

• Return a new ArrayBuffer whose contents are bytes.

Blob

• Return a Blob whose contents are bytes and type attribute is mimeType.

You do have some point though, as we attempt to store things in an organized fashion. https://w3c.github.io/web-nfc/#data-mapping

• text and url are stored in TNF 1, so everything else than text() should probably just return null.
• data (bytes) with mime type is stored in TNF 2, and it should be fine to make text(), json(), arrayBuffer() and blob() work.
• data (bytes) with no mime type is stored in TNF 4, so everything but blob() should work as blob requires mime type.

[leonhsl]

Thank you @kenchris ! I feel this is a great improvement related to data mapping between our WebIDL NDEFRecord(Init) and NDEF record, I'd like to propose to do it as the next step, in the respects of both spec and impl, i.e. merge #240 (focused on introduction of NDEF{Message,Record} interface types) first as a basis of the further improvement, to have things get done incrementally.

[kenchris]

Sure, can be another patch