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

Spec bug: Deal with undefined or missing url #39

Closed
domenic opened this issue Jun 20, 2017 · 5 comments
Closed

Spec bug: Deal with undefined or missing url #39

domenic opened this issue Jun 20, 2017 · 5 comments

Comments

@domenic
Copy link

domenic commented Jun 20, 2017

Currently if you do navigator.share() on https://example.com/, it will share https://example.com/undefined, by my reading. (I.e. it will convert undefined to a USVString, then resolve it as a relative URL.)

If you add a default to the dictionary of url = "", it will instead share the current page URL, which seems nice.

@mgiuca
Copy link
Collaborator

mgiuca commented Jun 20, 2017

Let's talk about the desired behaviour before getting into the spec treatment.

If you add a default to the dictionary of url = "", it will instead share the current page URL, which seems nice.

I don't think that's desirable default behaviour. True, it's something you'll commonly want, but we shouldn't force it. All fields should be omitted by default.

The intention (and current implementation in Chrome and in the Web Platform Tests draft) is for the following url values to have the given meaning:

  • No url: No URL transmitted. (test)
  • url: undefined: No URL transmitted. (Not explicitly tested, but we have tests for text: undefined and missing url.)
  • url: null: https://<origin>/<directory>/null transmitted. This isn't great but it's a side-effect of having a non-nullable field in the dictionary, null is just treated as any other non-string JavaScript type and converted into a string. (test)
  • url: '': https://<origin>/<directory>/<page> transmitted. (test)

On to the encoding of this logic in the spec.

Currently if you do navigator.share() on https://example.com/, it will share https://example.com/undefined, by my reading. (I.e. it will convert undefined to a USVString, then resolve it as a relative URL.)

WebIDL 3.2.14.4.1 treats undefined specially (unlike null) and omits it from the dictionary value that's created. So it won't convert undefined into the string "undefined".

However, I think you have a point in that my definition of share doesn't check whether url exists in the dictionary. I think this is simply a type error in the spec (classic missing null-check!). In navigator.share, if url is missing from the dictionary, it should skip Steps 2, 3 and 4. I will put up a pull request.

mgiuca added a commit to mgiuca/web-share that referenced this issue Jun 20, 2017
Fixes a spec bug where a non-existent URL would be parsed.

Closes w3c#39.
@mgiuca mgiuca changed the title Consider making the default for url "" Spec bug: Deal with undefined or missing url Jun 20, 2017
@domenic
Copy link
Author

domenic commented Jun 20, 2017

Hmm OK, I thought it'd be nicer to make the syntax for sharing the current URL navigator.share(), instead of navigator.share({ url: "" }). The latter is rather cryptic whereas the former is rather natural. But I don't care too strongly... maybe something to ask for a broader opinion on?

@domenic
Copy link
Author

domenic commented Jun 20, 2017

Oh, I see, the idea is to be able to share text/a title without sharing a URL. Is that something native platform APIs commonly do? (Probably, I just don't know.) In that case your fix makes perfect sense.

@mgiuca
Copy link
Collaborator

mgiuca commented Jun 21, 2017

I'm not sure how common it is, but it's perfectly normal to share just text on Android (for example). e.g., in Chrome for Android, select some text on the page and a context menu appears. Click "Share" and it shares just the text, no URL. I suspect most of the time people will want to share a URL, but I see no reason to bias towards having a URL by default (and I don't see a way you would turn off URL sharing if we made that the default).

In that case, do you want to review #42 further or I'll just land it?

@domenic
Copy link
Author

domenic commented Jun 21, 2017

#42 LGTM now that I understand. Yeah, the main argument in my mind is that if we defaulted the URL, it would be impossible to turn off URL sharing.

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

No branches or pull requests

2 participants