Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sharing files #78

Merged
merged 1 commit into from
Oct 10, 2018
Merged

Sharing files #78

merged 1 commit into from
Oct 10, 2018

Conversation

ewilligers
Copy link
Collaborator

@ewilligers ewilligers commented Sep 26, 2018

Add a files member to Web Share dictionary.

Add canShare() so authors can determine if an implementation supports sharing a given ShareData dictionary. In particular, this allows authors to determine if an implementation supports sharing files.


Preview | Diff

index.html Outdated
@@ -224,6 +243,7 @@ <h3>
USVString title;
USVString text;
USVString url;
FileList files;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For some reason, this hasn't been linked properly in the preview. Maybe you need to reference !FILE-API somehow as a dependency for this text box.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I wonder if FileList should be considered deprecated, due to the comment here:

Note: The FileList interface should be considered "at risk" since the general trend on the Web Platform is to replace such interfaces with the Array platform object in ECMAScript [ECMA-262].

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if FileList should be considered deprecated

FileList is used widely. It should be safe to use. I haven't seen sequence<File> used anywhere, other than as the definition of FileList in early versions of the File API.

We won't use item() calls in any examples, due to the "at risk" comment.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The link in the IDL is now fixed.

index.html Outdated
@@ -95,6 +95,7 @@ <h3>
</p>
<pre class="idl">
partial interface Navigator {
[SecureContext] boolean canShare(optional ShareData data);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've talked on whatwg/webidl#107 about a more generic solution to feature detection of dictionary members, than every API making its own canX method. Perhaps we should check up on the status of that.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussion is ongoing...

index.html Outdated
<var>data</var>, run the following steps:
</p>
<ol data-link-for="ShareData">
<li>If none of <var>data</var>'s members <a>title</a>, <a>text</a>,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't really capture what happens if files is not supported. I know the spec assumes that it is (since an implementation compliant with the latest spec would support file sharing), but what if it isn't?

If files isn't supported, does it return true or false if I include both files and some other supported field?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additional unsupported dictionary fields are ignored. (We could mention that explicitly.) If an implementation chose to support canShare but not file sharing, then (a) that implementation would not be consistent with any version of the spec, (b) that implementation should return false if the dictionary only contained files. If would return true if the dictionary also contained some other supported field.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding text to say the method must return false if the dictionary contains files and file sharing is not supported would be possible for the special case of files, but similar text would probably not be possible for any future fields that might be added in future versions.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If an implementation chose to support canShare but not file sharing, then (a) that implementation would not be consistent with any version of the spec

I guess this is important for how developers are expected to use it.

Are we leaving open a possibility of an intermediate browser that supports canShare but not files? It's possible that a future browser will never want to support file sharing (because the OS doesn't have a concept of files, or whatever), but will want to support some other new feature that requires canShare. So I think we should allow this, possibly even explicitly saying that files is a MAY (not a MUST), and linking up canShare so it MUST return true if and only if files is accepted.

If we are allowing this possibility of canShare without files, then we need to enumerate this case, and that means developers are expected to check for both canShare and files, like this:

if (navigator.share && navigator.canShare && navigator.canShare({files})) {
  await showShareButton();
  navigator.share({files});
}

If we aren't, then it means the very existence of canShare implies that files is supported, so then developers have a legitimate expectation of this code always being correct:

if (navigator.share && navigator.canShare) {
  await showShareButton();
  navigator.share({files});
}

The latter option seems silly to me, so I think we should require developers to actually call canShare if they want to check if file sharing is allowed, which means we need to allow canShare to exist, but return false if given files.

index.html Outdated
<var>data</var>, run the following steps:
</p>
<ol data-link-for="ShareData">
<li>If none of <var>data</var>'s members <a>title</a>, <a>text</a>,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If an implementation chose to support canShare but not file sharing, then (a) that implementation would not be consistent with any version of the spec

I guess this is important for how developers are expected to use it.

Are we leaving open a possibility of an intermediate browser that supports canShare but not files? It's possible that a future browser will never want to support file sharing (because the OS doesn't have a concept of files, or whatever), but will want to support some other new feature that requires canShare. So I think we should allow this, possibly even explicitly saying that files is a MAY (not a MUST), and linking up canShare so it MUST return true if and only if files is accepted.

If we are allowing this possibility of canShare without files, then we need to enumerate this case, and that means developers are expected to check for both canShare and files, like this:

if (navigator.share && navigator.canShare && navigator.canShare({files})) {
  await showShareButton();
  navigator.share({files});
}

If we aren't, then it means the very existence of canShare implies that files is supported, so then developers have a legitimate expectation of this code always being correct:

if (navigator.share && navigator.canShare) {
  await showShareButton();
  navigator.share({files});
}

The latter option seems silly to me, so I think we should require developers to actually call canShare if they want to check if file sharing is allowed, which means we need to allow canShare to exist, but return false if given files.

index.html Outdated
</ol>
<div class="note" data-link-for="ShareData">
<p>
<code>canShare()</code> MUST return <code>false</code> whenever
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is normative, and therefore shouldn't be in a note. (I must've missed this text last time I looked.)

This is actually not true, as stated. If url is invalid, share should reject with TypeError, but canShare should return true because it doesn't check URL. I think canShare should return false if url is invalid, and thus this statement here better matches what I want than the definition above.

I think I'd be happier with this being the definition of canShare, rather than a corollary (so we don't have to duplicate or abstract out the checking logic). Do you think we can get away with simply deleting the steps, and writing "canShare() MUST return false whenever ..... It MUST return true otherwise." ?

index.html Outdated
@@ -95,6 +95,7 @@ <h3>
</p>
<pre class="idl">
partial interface Navigator {
[SecureContext] boolean canShare(optional ShareData data);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussion is ongoing...

Copy link
Collaborator

@mgiuca mgiuca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but did you want to do the Level 2 thing we talked about, where you copy the spec into a new HTML page, and add this stuff there? Thus we avoid having canShare be part of the basic spec.

index.html Outdated
<dfn>canShare()</dfn> method
</h4>
<p>
<code>canShare(</code><var>data</var><code>)</code> returns
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MUST return true unless .... in which case it MUST return false.

index.html Outdated
<a>TypeError</a>, and abort these steps.
</li>
<li>If the implementation does not support file sharing and none of
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not seem sustainable; imagine adding another optional member, the permutations.

I would rather just delete this, and add a parenthetical above, like this:

"If none of data's members title, text, or url are present, reject p with url or (if the implementation supports file sharing) files are present"

Add a files member to Web Share dictionary.

Add canShare() so authors can determine if an implementation supports
sharing a given ShareData dictionary. In particular, this allows authors
to determine if an implementation supports sharing files.
@ewilligers
Copy link
Collaborator Author

Rebased to Level 2, comments addressed.

Copy link
Collaborator

@mgiuca mgiuca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for rebasing onto Level 2.

I think this is a good structure for discussing potentially wild changes while keeping the "classic" Web Share spec stable.

@ewilligers ewilligers merged commit fb374da into w3c:master Oct 10, 2018
@ewilligers ewilligers deleted the sharing-files branch October 10, 2018 09:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants