Spec feedback

[jakearchibald]

Sorry for dumping all this in one issue, I'm being lazy.

Don't be put off by the size of this list. Most of this is stuff I've learned by getting stuff wrong and having others correct me. I wish we had better guides to writing specs.

---

Contact pickers are frequently seen in native mobile applications for a variety of use cases

It might be worth listing the use cases here.

---

The contact picker model also gives websites one-off access to a user’s contacts

"also" makes it sound like an additional behaviour, as in, it might also give something other than one-off access.

---

Contact information is composed of the following parts.

"Contact information" is defined but never referenced. I would be tempted to roll this section into your "user contact" definition.

So instead of

names, a [=list=] of {{USVString}}s, intially empty, each [=list/item=] representing a
[=contact information/name=] of the contact.

It could be:

names, a [=list=] of {{USVString}}s, intially empty, each [=list/item=] representing a unique name corresponding to the user.

---

A user contact consists of:

It might be worth clarifying:

• The data relates to a single user.
• The first entry in names isn't associated with the first entry in emails.
• The lists may be of different lengths.
• Is names[0] more important than names[1]?

---

intially

Should be 'initially'. I didn't even notice until I pasted it above and the spell checker complained 😀.

---

names, a list of USVStrings

I think these should be DOMStrings. I'm not 100% sure when one should be used over the other, but https://heycam.github.io/webidl/#idl-USVString recommends DOMString if in doubt.

---

A user contact consists of:

Are the items in names, emails, numbers expected to be ordered in any way?

Are any of the fields validated? Could an email be an invalid email address like "whatever"?

---

[Exposed=(Window,SecureContext)]
partial interface Navigator {

Navigator is already exposed in non-secure contexts. You can choose to expose particular properties though. See https://w3c.github.io/ServiceWorker/#navigator-serviceworker.

---

You need to define how the navigator.contacts getter works. https://wicg.github.io/background-fetch/#extensions-to-service-worker-registration is a good reference.

---

dictionary ContactInfo {
  sequence<USVString> name;
  sequence<USVString> email;
  sequence<USVString> tel;
};

We generally avoid returning dictionaries as they don't have any type definition in JavaScript. This would be better as an interface with defined getters, like https://wicg.github.io/background-fetch/#background-fetch-record-interface. Ignore me.

Interfaces can't have attributes that are sequences, because they're passed by value. As an alternative, you can use FrozenArray.

I think the values should be DOMStrings.

---

dictionary ContactsSelectOptions {
  required sequence<ContactProperty> properties;
  boolean multiple = false;
};

We generally avoid putting required items into option objects. I'd split this into:

navigator.contacts.select(['name', 'email'], { multiple: true });

---

navigator.contacts only has one method. I wouldn't be surprised if TAG asked why it isn't navigator.selectContacts. I guess we expect to add other things there in future? Worth having an answer ready.

---

1. Let |promise| be a new {{Promise}}.

I think this should be:

1. Let |promise| be [=a new promise=].

Which references https://www.w3.org/2001/tag/doc/promises-guide#a-new-promise, where the other promise manipulation stuff happens.

Also, totally nit-picky, but I'd move this line after the early returns, otherwise a promise is created but never used.

---

2. If the browsing context is not a…

You need to define which browsing context, since it'd be different if the caller was in a different realm. I'd go for:

1. Let |relevantBrowsingContext| be the [=context object=]'s [=relevant settings object=]'s [=environment settings object/responsible browsing context=].
1. If |relevantBrowsingContext| is not a…

Bit of a mouthful, but covers off cases like:

anIframe.contentWindow.navigator.contacts.select(…);
anIframe.contentWindow.navigator.contacts.select.call(navigator.contacts, …);

---

1. If [=contact picker is showing flag=] is set…

Should be:

1. If |relevantBrowsingContext|'s [=contact picker is showing flag=] is set…

Same goes for the "Set contact picker is showing flag" line.

---

To launch a contact picker

Nit: This section uses "share", "pick" and "select" to mean (I think) the same thing.

Some ideas to simplify this section (feel free to ignore):

You could have a single algorithm:

To <dfn lt="launching a contact picker">launch a contact picker</dfn> with |allowMultiple| (a boolean)…

Then, define the constraints as you have now, but also define when this 'algorithm' returns.

* The UI must provide an option to cancel/return without sharing any contacts, in which case remove the UI and return an empty [=/list=].
* The UI must provide an a way for users to indicate that they are done selecting, in which case remove the UI and return a [=/list=] of the selected contacts as [=user contacts=].

At this point, you might want to provide constraints about formatting and ordering. If there are no formatting constraints, it's worth mentioning that too.

Now you can just do:

1. Let |contacts| be the result of [=launching a contact picker=] with |option|'s `multiple` member.
1. Unset |relevantBrowsingContext|'s [=contact picker is showing flag=].

It doesn't matter that "launch a contact picker" blocks on this line, as you're already in parallel. Now you don't need to wait for flags to be set etc etc.

---

The UI MUST make it clear which properties of the contact will be shared.

properties needs to be passed in as an argument to this algorithm.

---

Launch a new contact picker with select multiple contacts flag set if options’s multiple member is true. If this fails, then:

The reasons for failure aren't defined. I'd include this in "Launch a contact picker". Eg "* If (something something something), return failure".

---

Unset contact picker is showing flag.

This is being set in a different thread to where it's checked. I don't think this is a problem right now, but it's worth keeping an eye on so it doesn't create a race condition in future. The important part is that it's checked & set synchronously.

Also, the flag is only unset on failure, it also needs to be unset on success.

---

Let contact be a new ContactInfo with:

You can only create JavaScript objects when you're on the same thread as JavaScript. To do this:

1. [=Queue a task=] on (TODO task source goes here) to run these steps:
  1. …create ContactInfo objects…
  1. …resolve the promise…

I don't think any of the generic task sources are a good fit, so define your own. For reference: https://wicg.github.io/background-fetch/#background-fetch-task-source.

[jakearchibald]

We generally avoid returning dictionaries

Ignore this bit for now. I could be wrong.

[jakearchibald]

Yeah, I'm wrong. Returning a dictionary is fine.

[rayankans]

Thanks for the feedback @jakearchibald, this was super helpful. I resolved all of the points you brought up with the following exceptions:

Are the items in names, emails, numbers expected to be ordered in any way?
Are any of the fields validated? Could an email be an invalid email address like "whatever"?

I'm leaving this ambiguous for now. I'll open a spec issue for it.

I don't think any of the generic task sources are a good fit, so define your own

I have a few questions about this, so I'll this issue open until I have a chance to discuss it with you.