w3c / web-share

Web API proposal for sharing data from a web page
https://www.w3.org/TR/web-share/
Other
353 stars 65 forks source link

Fail when unknown field exists on share() and canShare()? #214

Closed saschanaz closed 2 years ago

saschanaz commented 3 years ago

(https://github.com/w3c/web-share/issues/211#issue-975366558)

navigator.share({
  text: "text",
  unknown: "unknown",
});

The current behavior ignoring unknown silently is not very useful nor intuitive. I suspect that changing the behavior won't break existing uses and will allow partial implementations (e.g. Firefox as of 2021) fail early without being hacky.

saschanaz commented 3 years ago

Furthermore, the current difference between files unsupported at browser level and at OS level is confusing.

navigaor.share({
  text: "text",

  // this will be silently ignored when unsupported at browser level
  // but will explicitly fail when unsupported at OS level
  files: [new File()],
});

I think it should either be ignored in both cases or fail in both cases.

marcoscaceres commented 2 years ago

I guess this really depends on the meaning of "can share", right? The spec, and by general convention with WebIDL APIs ignoring unknown members, seems to suggest that so long as one or more members matches, then something is sharable.

That solution, which maybe we should add as an example to the spec, is to do something like the following:

const data = { /* a bunch of members, some supported, some not... */ };
// Check them individually
const canShareEvery = Object.entries(data).every(([k, v]) => navigator.canShare({[k]: v}));

Or some variant of the above... like, one could filter on the unsharable members and remove those things from the application.

const sharableMembers = Object.entries(data).filter(([k, v]) => navigator.canShare({[k]: v}));

Or one could write a simple "yep/nope" splitter.

saschanaz commented 2 years ago

Maybe deprecate the multi member situation in that case?

marcoscaceres commented 2 years ago

Well, that gets us back to the original proposal where you could only test if a member name was supported. I'm inclined to cut our loses here because (although clunky), a developer can use the code above to check exactly what is supported.

If it turns out .canShare() can't be used correctly, then we should revisit some alternative API..

(as an aside, if we find generally in the platform that we need a "only these members" IDL check, we might want to push that up to WebIDL itself as some extended attribute... something like):

[ThrowOnUnknownMember]
dictionary {
 }

If we accept the current design, I then just realized this is a duplicate of https://github.com/w3c/web-share/issues/211

saschanaz commented 2 years ago

a developer can

We know this "can" does not mean they will 😄. The current API form strongly suggests doing bad thing, so I doubt anyone will go the repeated-single-member without explicit deprecation of the multi member thing.

marcoscaceres commented 2 years ago

I certainly don't disagree (gets us back to https://github.com/w3c/web-share/issues/108).

The question is really if it's enough of a problem for us to do anything about at this point?

My feeling is no (given lack of people coming here to complain... but not a great data point). I'd be ok to just live with .canShare() as is and move onto other things.

saschanaz commented 2 years ago

The thing is, this will become worse and worse if the spec "moves on other things" and add features.

I'm okay to keep it as is if nobody intends to add anything else here, but if someone ever does then we need to talk about this.

marcoscaceres commented 2 years ago

agree. But given we haven't added any new features is last 2 years, and the feature is more or less working as expected, I feel pretty comfortable with us moving onto other things.

saschanaz commented 2 years ago

Not sure what you mean by moving onto other things here. I'd argue that some note should there so that any future editors won't accidentally add anything.

marcoscaceres commented 2 years ago

Not sure what you mean by moving onto other things here.

I mean investing time on other specs and/or standardizing other parts of the web platform. I'd like for this spec to be "done" (i.e., moved to CR) so I can focus on other specs (e.g., Permissions, Web Manifest, etc.). This spec takes up quite a bit of time, but it's not, IMO, has high priority as some other specs I could be spending time on.

I'd argue that some note should there so that any future editors won't accidentally add anything.

The Editors are not resigning or anything. I don't think that's a risk.

saschanaz commented 2 years ago

I mean investing time on other specs and/or standardizing other parts of the web platform. I'd like for this spec to be "done" (i.e., moved to CR) so I can focus on other specs (e.g., Permissions, Web Manifest, etc.). This spec takes up quite a bit of time, but it's not, IMO, has high priority as some other specs I could be spending time on.

canShare() has been modified multiple times since the initial issue and there's a pending PR from you, so I doubt such thing won't happen without some explicit note, a pinned issue, or anything that remind future people (and ourselves in the future) that it's faulty. (That said, this note thing is indeed #211)

Edit: See also https://github.com/whatwg/webidl/issues/107 for the IDL issue. Edit: Anyway I don't have a big interest to fix anything other than the YouTube issue, so maybe just add Revisit tag and do the tag says.

marcoscaceres commented 2 years ago

Created a Revisit label + added.

marcoscaceres commented 2 years ago

Actually, can we close this one as it's the same as #108?

saschanaz commented 2 years ago

108 is too long with no conclusion, this one at least has a concrete suggestion 🤔

But this didn't attract any interest here, so let's go back to #108.