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

Better describe intention of navigator.canShare() and usage #211

Closed
saschanaz opened this issue Aug 20, 2021 · 16 comments · Fixed by #238
Closed

Better describe intention of navigator.canShare() and usage #211

saschanaz opened this issue Aug 20, 2021 · 16 comments · Fixed by #238

Comments

@saschanaz
Copy link
Member

Previous context: #108 (comment)

navigator.canShare({
  text: "foo",
  somethingUnsupported: {}
});

This should fail but currently it does not.

What weird is that share() will anyway succeed with that argument.

@saschanaz
Copy link
Member Author

Hmmmmm.

Given that share() will succeed anyway, I wonder we can explicitly describe canShare() as "a check to ensure at least one of the fields are supported" and leave the behavior as-is. If all of them are unsupported then the current algorithm will ban it.

Wdyt? @marcoscaceres

@saschanaz saschanaz changed the title navigator.canShare() must fail for unknown dictionary members Better describe intention of navigator.canShare() and usage Aug 20, 2021
@saschanaz
Copy link
Member Author

It would also be worth mentioning that if support for a specific field is important, then the field should be passed alone for the expected result.

@marcoscaceres
Copy link
Member

Definitely wouldn't hurt to provide such guidance. Do you want to have a go at putting together some wording for it or suggest something here?

@saschanaz
Copy link
Member Author

saschanaz commented Aug 24, 2021

I doubt I'll have time to do that too soon, but something like:

NOTE: canShare() only checks the validity of the supported values and ignores the unsupported ones.

const result = navigator.canShare({
  text: "Star Wars",
  darthVader: window.parent // this is completely ignored
});
result // true, since `text` can be shared

To check whether a specific field is supported, pass that field as the sole one.

const result = navigator.canShare({
  darthVader: window.parent
});
result // false

Unsupported values being ignored will make the object be perceived as an empty dictionary, which will be rejected by canShare() as nothing can be shared.

(Happy to review that if you intend to do that for me)

@marcoscaceres
Copy link
Member

That’s great! Thank you! I’ll adapt that and ping you for review.

@hamishwillee
Copy link

hamishwillee commented Aug 27, 2021

  • If you don't pass a data object to canShare() it will always return false - is that correct?
  • I ask because it isn't clear to me what this means: "Return the result of validate share data with data and this's relevant settings object's API base URL."? I.e. I can understand what returning the result of validating data means, but not when you add "with data and this's relevant settings object's API base URL"

@marcoscaceres
Copy link
Member

If you don't pass a data object to canShare() it will always return false - is that correct?

yes. This is because when called with undefined, it magically gets assigned a default empty ShareData object.

-What does this mean: "Return the result of validate share data with data and this's relevant settings object's API base URL."?
I.e. I can understand what returning the result of validating data means, but not when you add "with data and this's relevant settings object's API base URL"

Ah, that basically just means this... if it was JavaScript:

validateShareData(data, document.baseURI); 

Where "this" is the this object (the instance of Navigator).
And "relevant settings object" is just concept from HTML that gives you access to "API base URL" - which is effectively document.baseURI

I know, it's super convoluted to read. But it's what we got to work with and it matches browser internals (C++) rather nicely.

@hamishwillee
Copy link

@marcoscaceres Thanks very much. It is mostly confusing because at the point you see this text you don't know that the URI can be relative (assuming reading from top to bottom).

@marcoscaceres
Copy link
Member

marcoscaceres commented Aug 27, 2021

ah, gotcha. Yes, this is why we pass it along to the other validation algorithm. But the assumption is that URLs are always relative, which is why we always resolve them against some "base".

@hamishwillee
Copy link

hamishwillee commented Aug 27, 2021

Just to confirm, the main purpose of this method is actually feature detection of supported properties of the data object right?

I ask because the MDN docs (I am updating) indicate that you would use it to verify that the share data you would send in the share() would work. ie it implies that you call canShare to test some data to send, and if that returns true you then share() it.

But as I see it, that is pointless because canShare() for data with multiple fields can return true even if one of the fields is not supported - so if you use this approach you might as well just try and share() in the first place.

To illustrate, say you want to send a new field "vcard" along with a title. A system that supports the new field will report success. A system that doesn't support the new field will drop vcard field and still return true, because title is specified.
So the only way that canShare() will provide reliable data is if you use it to test just the vcard field. If that succeeds then you know that your title and vcard might succeed.

Does that make sense? Maybe I am missing something.

@marcoscaceres
Copy link
Member

marcoscaceres commented Aug 27, 2021

Just to confirm, the main purpose of this method is actually feature detection of supported properties of the data object right?

It's kinda dual purpose... "is property supported" and "If I try to pass this url/file/etc., would it actually get shared?"

I ask because the MDN docs (I am updating) indicate that you would use it to verify that the share data you would send in the share() would work. ie it implies that you call canShare to test some data to send, and if that returns true you then share() it.

Correct!

But as I see it, that is pointless because canShare() for data with multiple fields can return true even if one of the fields is not supported

If any are found to be in error or not supported, it always returns 'false'. But the design is actually broken in other ways and it's not future proof 😡 (what you found below!). A few of us pointed this out for over a year, however, no one cared enough to fix it, so broken .canShare() shipped in Chrome and Safari.
#108

  • so if you use this approach you might as well just try and share() in the first place.

You can't, because you need a "click", and by calling .share() you "consume transient activation" - meaning you don't get to call .share() (again until another "click" or other gesture).

To illustrate, say you want to send a new field "vcard" along with a title. A system that supports the new field will report success. A system that doesn't support the new field will drop vcard field and still return true, because title is specified.
So the only way that canShare() will provide reliable data is if you use it to test just the vcard field. If that succeeds then you know that your title and vcard might succeed.

Does that make sense? Maybe I am missing something.

Nope! You got it. It's quite unfortunate. We might fix it in the future if anyone cares to.

@hamishwillee
Copy link

Thanks again. Awesome.

so if you use this approach you might as well just try and share() in the first place.

You can't, because you need a "click", and by calling .share() you "consume transient activation" - meaning you don't get to call .share() (again until another "click" or other gesture).

FYI only - minor clarification - you misunderstood me :-). In this path I was proposing that there is no point testing the data before calling share. Instead you'd just trigger share with your possibly unreliable data and "hope" - and hence only need the one transient action.

But as we then discussed, given the limitations of the implementation of canShare, the recommendation would be to use canShare to test each field you want to send individually, THEN you could use share safely.

Doesn't change anything, just pointing it out.

@hamishwillee
Copy link

Can we please keep this open until next week. I hope to add links to the updated docs for you to sanity check!

@hamishwillee
Copy link

Can you please sanity check out the proposed new version of canShare() docs on MDN?

Appreciate this is "not your job", but really appreciate it if you are able to find the time.

I'll update share on Monday, followed by the parent doc.

@saschanaz
Copy link
Member Author

But as we then discussed, given the limitations of the implementation of canShare, the recommendation would be to use canShare to test each field you want to send individually, THEN you could use share safely.

Depends on what "safe" is. If one is okay when at least one field is supported and shared, then passing all at once should be completely okay. If one needs to make sure everything is supported, then the individual call is needed.

(I'm not sure this behavior is any useful, though.)

@marcoscaceres
Copy link
Member

Closed by #211

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 a pull request may close this issue.

3 participants