Replace the URL template with a dictionary. #40

ewilligers · 2018-06-04T03:07:51Z

comp615 · 2018-06-04T20:31:49Z

index.html

+    "params": {
+      "title": "title",
+      "text": "text",
+      "url": "url"


Maybe a minor thing, but as I was reading through this. I always find it helpful when examples differentiate constants vs variables that I can change. For instance here I have to read on to know if I change keys or values. Better might be to use a contrived example. Keeping in mind I'm no better at naming:

"title": "new_story_headline", "text": "new_story_body", "url": "new_story_link_url"

I have changed the names.

mgiuca

Thanks for doing this. I have a bunch of comments (and in general, I think there is stuff removed that needs to stay, or be updated rather than deleted).

mgiuca · 2018-06-08T05:50:42Z

index.html

-        the directory containing the manifest URL.
+        The <a data-link-for="params">params</a> contain the names for a number
+        of form fields that will be populated with the shared data when
+        the target is <a>invoked</a> with a GET request.


Say that: The keys correspond to the key names in ShareData, while the values are arbitrary names that will be used as query parameters when the target is launched.

mgiuca · 2018-06-08T05:51:54Z

index.html

+      </p>
+      <p>
+        When a share takes place, if the user selects this app as the share
+        target, the user agent creates a form submission, and


I'm not sure we want it to exactly be a form submission (that has a number of bad quirks, like encoding). Let's say "like a form submission".

mgiuca · 2018-06-08T05:53:11Z

index.html

-        window or tab and navigate to:
-      </p>
-      <pre>
-https://example.org/includinator/share.html?title=My%20News&amp;text=&amp;url=http%3A%2F%2Fexample.com%2Fnews


I would like to keep this example "finished" URL, to show that it just makes a GET request to a particular URL. Can you work that in?

mgiuca · 2018-06-08T05:55:26Z

index.html

+      <p>
+        When a share takes place, if the user selects this app as the share
+        target, the user agent creates a form submission, and
+        opens a new browsing context at the action URL.


<code>action</code>

mgiuca · 2018-06-08T05:59:25Z

index.html

 };

 partial dictionary WebAppManifest {
  ShareTarget share_target;
 };
 </pre>
-      <p>


Why is this (and the below steps for post-processing the share_target) gone? It needs to be updated for the new format.

There are still some things that need to be performed / validated:

action must be parsed and resolved against manifest URL.

(If the parsing is failure, the share target is invalid.)

action must be within scope of scope URL.

method must be "GET".

That's all I can think of off the top of my head.

mgiuca · 2018-06-08T06:02:56Z

index.html

-          "!WebShare#dom-sharedata"><code>ShareData</code></a> dictionary.
-          After these placeholders are removed, it MUST form a <a data-cite=
+          The <dfn>action</dfn> member specifies the URL for the web share
+          target. It MUST be a <a data-cite=


I think it's actually wrong (and was originally) to say this MUST be a valid URL string.

"Valid URL string" refers to a 100% correct URL that would not trigger any validation error in the URL parser, which is way stricter than anything else is on the web. (Essentially, nothing ever requires a valid URL string, rather, URLs are run through the URL Parser and it is required to not fail.)

Just delete this sentence, and we will perform parsing in the pre-processing steps above.

(I realised that Web Share is also erroneous to say "Valid URL string" on its url field, and have sent out a PR: w3c/web-share#76.)

mgiuca · 2018-06-08T06:26:11Z

index.html

+          members.
+        </p>
+        <p>
+          The <dfn>title</dfn> member specifies the name of the form field


These are fairly non-descript. We could perhaps use the same terminology as Web Share:

text member

Arbitrary text that forms the body of the message being shared.

title member

The title of the document being shared. May be ignored by the target.

url member

A URL string referring to a resource being shared.

Or maybe literally link to Web Share's ShareData dictionary, since these should always be kept up-to-date?

mgiuca · 2018-06-08T06:30:31Z

index.html

 dictionary ShareTarget {
-  required USVString url_template;
+  required USVString action;
+  required ShareTargetParams params;


We should have "method" here, for future-proofing.

I think we should have optional DOMString method = "GET", with processing steps that say its value MUST be "GET". Essentially, everyone can ignore it, BUT if it's anything other than "GET", the whole share target is ignored.

This way, if a site from the future says "method: POST", current browsers will ignore it instead of erroneously delivering a GET request.

mgiuca · 2018-06-08T06:38:04Z

index.html

        <p>
-          Any fields that are not present in the <a>url_template</a> will be


This paragraph is still relevant.

mgiuca · 2018-06-08T06:47:31Z

index.html

@@ -435,153 +355,6 @@ <h3>
          "!WebShare#dom-sharedata-url"><code>url</code></a> field.
        </p>
      </section>
-      <section data-link-for="WebAppManifest">


This section needs to be rewritten (not deleted).

You should be able to delete the replace placeholders algorithm, and then update this to construct the URL from the action and data.

An interesting question will be whether the query should be encoded with the application/x-www-form-urlencoded scheme, or the new default encode set which I am currently working on adding to the URL standard (whatwg/url#369).

The former:

Is compatible with how HTML forms work. It uses arbitary encoding (though probably we'd lock it to UTF-8), encodes space as '+', and leaves -._* un-encoded.

You would just call the application/x-www-form-urlencoded serializer.

The latter:

Is compatible with how modern encoders work, like encodeURIComponent in JS. It uses UTF-8, encodes space as "%20", and leaves -_.!~*'() un-encoded.

It's compatible with how registerProtocolHandler works in practice (though it is not specified correctly; see Need an "unreserved" character set (and better define how to percent-encode arbitrary strings) whatwg/url#369).

This is how I currently define encoding in Web Share Target (see the bit at the bottom of replace placeholders).

We should discuss this, and raise it at TAG review. The most important thing is whether space as '+' is commonly accepted by web servers. As far as I know, this is only used by HTML forms, and may not be expected by endpoints that are not form submission targets.

The new spec text for Launching the web share target is based on https://html.spec.whatwg.org/#concept-form-submit

mgiuca · 2018-06-20T03:18:52Z

index.html

+        parameters when the target is launched.
+      </p>
+      <p>
+        When a share takes place, if the user selects this app as the share


Delete "app as the". We don't refer to share targets as apps (since the may not necessarily correspond to an app).

(I know this was in the previous text, but #whileyourehere.)

mgiuca · 2018-06-20T03:20:57Z

index.html

+      </p>
+      <p>
+        When a share takes place, if the user selects this app as the share
+        target, the user agent creates a GET request like a form submission, and


This is a bit of a weird way to describe it, since opening the new browsing context is what creates the GET request. (It doesn't "create a GET request, then open a browsing context".)

How about, "opens a new browsing context at the action URL, with query parameter values containing the shared data, just like an HTML form submission"?

mgiuca · 2018-06-21T01:18:15Z

index.html

-          <li>If <var>URL template string</var> is <code>undefined</code>,
+          <li>If <var>share
+          target["<a href="#dom-sharetarget-method">method</a>"]</var> is not an
+          ASCII case-insensitive match for the string <code>"GET"</code>,


Link to https://infra.spec.whatwg.org/#ascii-case-insensitive

Note: Use the data-cite, not href. So this probably should look like:

<a data-cite="!INFRA#ascii-case-insensitive">

mgiuca · 2018-06-21T01:23:32Z

index.html

-        "!URL#concept-url-fragment">fragment</a> parts of the URL template.
-        Once the fields are replaced, it should be a URL that is relative to
-        the directory containing the manifest URL.
+        The <a href="#dom-sharetarget-params">params</a> keys correspond to the key


Try to avoid using href, in favour of ReSpec's semantic linking options (use the existing code as a reference).

I think this should be:

<a data-link-for="ShareTarget">params</a>

(The current source file almost never uses "href", other than links to GitHub issues, and external sites, and I'd like to keep it that way.)

mgiuca · 2018-06-21T01:24:17Z

index.html

-        Once the fields are replaced, it should be a URL that is relative to
-        the directory containing the manifest URL.
+        The <a href="#dom-sharetarget-params">params</a> keys correspond to the key
+        names in <a href="https://wicg.github.io/web-share/#sharedata-dictionary">ShareData</a>,


<a data-cite="!WebShare#dom-sharedata">ShareData</a>

(Note: The "!" prefix makes it a normative reference; without a "!" it's informative. This is used to generate the bibliography at the bottom.)

Also, add "from [[!WebShare]]" (after "ShareData") since you are referring to a different spec.

mgiuca · 2018-06-21T02:00:20Z

index.html

+            "url"</code>,
+            <ul>
+              <li>
+                Let <var>name</var> be the value of <var>manifest["


Deal with missing member from params.

mgiuca · 2018-06-21T02:01:14Z

index.html

-          steps</a> on <var>final URL</var>, with empty <var>target</var> and
-          empty <var>features</var>.
+          <li>
+            Let entry list be a new empty list of <a href=


<var>entry list</var>

mgiuca · 2018-06-21T02:02:54Z

index.html

-          empty <var>features</var>.
+          <li>
+            Let entry list be a new empty list of <a href=
+            "https://xhr.spec.whatwg.org/#concept-formdata-entry">entries</a>.


I don't think it's worth referencing the XHR spec, given that the only thing that uses entry list is the application/x-www-form-urlencoded serializer, which doesn't take a list of XHR entries, it takes a "list of name-value tuples".

So just say "Let entry list be a new empty list of name-value tuples."

mgiuca · 2018-06-21T02:05:04Z

index.html

-          <li>If <var>template</var> contains U+007D (<code>}</code>), return
-          failure (unmatched closing brace).
+          <li>
+            Set <var>parsed action</var>'s query component to <var>query</var>.


Link to https://url.spec.whatwg.org/#concept-url-query

mgiuca · 2018-06-21T02:07:38Z

index.html

-              <li>If <var>end</var> &lt; <var>start</var>, return failure
-              (unmatched closing brace).
+              <li>
+                <a href="https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#append-an-entry">


I don't like the look of that "append an entry" algorithm. It mangles the data in ways that make sense for legacy form data processing, but doesn't properly preserve newlines, etc. Let's avoid using it.

If you want to link "append", just link here: https://infra.spec.whatwg.org/#list-append

mgiuca · 2018-06-27T01:48:32Z

Hmm, pr-preview hasn't updated to a76ba15.

Maybe posting a comment will nudge it.

mgiuca

lgtm with a few formatting nits.

mgiuca · 2018-06-27T01:53:39Z

index.html

-          <li>Let <var>parsed action</var> be the resulting URL record.
+          <li>Let <var>action</var> be the result of
+          <a data-cite="!URL#concept-url-parser">parsing</a> the
+          <a data-cite="!URL#concept-url">URL</a> <var>share


The closing </var> should close at the end of "target", and not at the end of the bracket. Same applies later down below.

mgiuca · 2018-06-27T01:58:33Z

index.html

-          <li>
-            Let <var>parsed action</var> be the resulting URL record.
+            Let <var>action</var> be <var>manifest["<a>share_target</a>"]["<a
+            data-link-for="ShareTarget">action</a>"]</var>.


Same here (</var>). I see this is actually already widespread in the doc, but I'll clean those up separately.

Replace <var>data[member]</var> with [200~<var>data</var>[<var>member</var>]. String literals such as "action" are not encoded in <var>

ewilligers · 2018-06-27T03:12:37Z

The closing </var> should close at the end of "target", and not at the end of the bracket.

fixed

Replace the URL template with a dictionary.

dec9e00

ewilligers mentioned this pull request Jun 4, 2018

Web Share Target API w3ctag/design-reviews#221

Closed

4 tasks

comp615 reviewed Jun 4, 2018

View reviewed changes

differentiate field naming

50db1ba

mgiuca requested changes Jun 8, 2018

View reviewed changes

ericwilligers added 2 commits June 19, 2018 14:36

Show example URL

793764d

Use application/x-www-form-urlencoded serializer

fe644fa

The new spec text for Launching the web share target is based on https://html.spec.whatwg.org/#concept-form-submit

mgiuca requested changes Jun 21, 2018

View reviewed changes

Avoid linking to HTML forms spec

a76ba15

mgiuca approved these changes Jun 27, 2018

View reviewed changes

Avoid enclosing punctuation in <var>

0280a8c

Replace <var>data[member]</var> with [200~<var>data</var>[<var>member</var>]. String literals such as "action" are not encoded in <var>

ewilligers merged commit d556237 into w3c:master Jun 27, 2018

ewilligers deleted the ShareTargetParams branch June 27, 2018 03:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace the URL template with a dictionary. #40

Replace the URL template with a dictionary. #40

ewilligers commented Jun 4, 2018 •

edited by mgiuca

Loading

comp615 Jun 4, 2018 •

edited

Loading

ewilligers Jun 5, 2018

mgiuca left a comment

mgiuca Jun 8, 2018

mgiuca Jun 8, 2018

mgiuca Jun 8, 2018

mgiuca Jun 8, 2018

mgiuca Jun 8, 2018

mgiuca Jun 8, 2018

mgiuca Jun 8, 2018

mgiuca Jun 8, 2018

mgiuca Jun 8, 2018

mgiuca Jun 8, 2018

mgiuca Jun 20, 2018

mgiuca Jun 20, 2018

mgiuca Jun 21, 2018

mgiuca Jun 21, 2018

mgiuca Jun 21, 2018

mgiuca Jun 21, 2018

mgiuca Jun 21, 2018

mgiuca Jun 21, 2018

mgiuca Jun 21, 2018

mgiuca Jun 21, 2018

mgiuca commented Jun 27, 2018

mgiuca left a comment

mgiuca Jun 27, 2018

mgiuca Jun 27, 2018

ewilligers commented Jun 27, 2018

		<p>
		Any fields that are not present in the <a>url_template</a> will be

Replace the URL template with a dictionary. #40

Replace the URL template with a dictionary. #40

Conversation

ewilligers commented Jun 4, 2018 • edited by mgiuca Loading

comp615 Jun 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgiuca left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgiuca commented Jun 27, 2018

mgiuca left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ewilligers commented Jun 27, 2018

ewilligers commented Jun 4, 2018 •

edited by mgiuca

Loading

comp615 Jun 4, 2018 •

edited

Loading