Re: [w3c/manifest] Add shortcuts feature to the explainer and spec (#768) from Matt Giuca on 2019-10-04 (public-webapps-github@w3.org from October 2019)

From: Matt Giuca <notifications@github.com>
Date: Thu, 03 Oct 2019 18:47:39 -0700
To: w3c/manifest <manifest@noreply.github.com>
Cc: Subscribed <subscribed@noreply.github.com>
Message-ID: <w3c/manifest/pull/768/review/297227574@github.com>
mgiuca requested changes on this pull request.

Hi Aaron,

I did a detailed review pass over the PR. Sorry if there are things here that I could have caught earlier but didn't see them.

Mostly these are just detailed nits. Overall this works great.

> @@ -194,6 +194,62 @@ TBW: using description and screenshots.
 ## Theme color and background color
 TBW...
 
+## Adding shortcuts
+Numerous operating systems grant native applications the ability to add menu items to the app icon itself. These often provide quick access to key tasks for an app. Typically, these are exposed via a right click, long tap, or a similar context menu-triggering action. For web applications, you can define a set of shortcuts to be exposed when the app is installed. Each shortcut item must have a name and a target URL. You may also include additional information, such as a shorter name, a description for the action, one or more icons, and parameters to be passed along to the target URL.

"and parameters to be passed along to the target URL"

No longer true; delete this.

> +        <p>
+          The steps for <dfn>processing the <code>shortcuts</code> member</dfn>
+          are given by the following algorithm. The algorithm takes a
+          <a data-cite=
+          "WEBIDL#sequence-type">sequence</a>&lt;<a>ShortcutItem</a>&gt;
+          <var>shortcuts</var> as an argument. This algorithm returns an
+          <a data-cite=
+          "WEBIDL#sequence-type">sequence</a>&lt;<a>ShortcutItem</a>&gt;. For
+          each <var>shortcut</var> (<a>ShortcutItem</a>) in the sequence, set
+          <var>shortcut.icons</var> to the result of running <a>processing
+          `ImageResource` members</a> given <var>shortcut.icons</var> and
+          <var>manifest URL</var>.
+        </p>
+        <p>
+          A user agent SHOULD expose shortcuts via interactions that are
+          consistent with exposure of an application icon’s context menu in the

nit: This right quote should be a straight apostrophe.

> +          "WEBIDL#sequence-type">sequence</a>&lt;<a>ShortcutItem</a>&gt;
+          <var>shortcuts</var> as an argument. This algorithm returns an
+          <a data-cite=
+          "WEBIDL#sequence-type">sequence</a>&lt;<a>ShortcutItem</a>&gt;. For
+          each <var>shortcut</var> (<a>ShortcutItem</a>) in the sequence, set
+          <var>shortcut.icons</var> to the result of running <a>processing
+          `ImageResource` members</a> given <var>shortcut.icons</var> and
+          <var>manifest URL</var>.
+        </p>
+        <p>
+          A user agent SHOULD expose shortcuts via interactions that are
+          consistent with exposure of an application icon’s context menu in the
+          host operating system (e.g., right click, long press). A user agent
+          SHOULD render the shortcuts in the same order as they are provided in
+          the manifest. A user agent SHOULD represent the shortcuts in a manner
+          consistent with exposure of an application icon’s context menu in the

Same here.

> +            <samp>https://example.com/manifest.webmanifest</samp>:
+          </p>
+          <ul>
+            <li>The first shortcut would be displayed with the text "Play
+            Later". If the operating system supports icons for context menu
+            items and it also supports SVG images for that purpose, the user
+            agent would present
+            <samp>https://example.com/icons/play-later.svg</samp> next to the
+            text. When launched, the user agent would instantiate a new
+            <a>top-level browsing context</a> and navigate to
+            <samp>https://example.com/play-later</samp>.
+            </li>
+            <li>The first shortcut would be displayed with the text
+            "Subscriptions". When launched, the user agent would instantiate a
+            new <a>top-level browsing context</a> and navigate to
+            <samp>https://example.com/subscriptions?sort=desc</samp>.

The `sort=desc` part is no longer true since you removed params.

Maybe just add `?sort=desc` to the `"url"` in the example (without changing this sentence), to show that you can use query parameters in your URL.

> +        <dfn>ShortcutItem</dfn> and its members
+      </h2>
+      <pre class="idl">
+        dictionary ShortcutItem {
+          required USVString name;
+          USVString short_name;
+          USVString description;
+          required USVString url;
+          sequence&lt;ImageResource&gt; icons;
+        };
+      </pre>
+      <p>
+        Each <a>ShortcutItem</a> represents a link to a key task or page within
+        a web app. A user agent can use these values to assemble a context menu
+        to be displayed by the operating system when a user engages with the
+        web app’s icon.

Quote -> apostrophe

> +        </h3>
+        <p>
+          The <dfn>url</dfn> member of a <a>ShortcutItem</a> is a <a>URL</a> to
+          which a user agent should navigate when the associated shortcut is
+          activated.
+        </p>
+        <p>
+          The steps for <dfn>processing the <code>url</code> member of a
+          shortcut</dfn> are given by the following algorithm. The algorithm
+          takes a <a>ShortcutItem</a> <var>url</var>, and a <var>manifest
+          URL</var>, which is the <a>URL</a> from which the <var>manifest</var>
+          was fetched. This algorithm will return a <a>URL</a> or
+          <code>undefined</code>.
+        </p>
+        <ol>
+          <li>Let <var>value</var> be <var>url</var>

No need to create a *value* when you already have a variable *url*.

> +          The <dfn>url</dfn> member of a <a>ShortcutItem</a> is a <a>URL</a> to
+          which a user agent should navigate when the associated shortcut is
+          activated.
+        </p>
+        <p>
+          The steps for <dfn>processing the <code>url</code> member of a
+          shortcut</dfn> are given by the following algorithm. The algorithm
+          takes a <a>ShortcutItem</a> <var>url</var>, and a <var>manifest
+          URL</var>, which is the <a>URL</a> from which the <var>manifest</var>
+          was fetched. This algorithm will return a <a>URL</a> or
+          <code>undefined</code>.
+        </p>
+        <ol>
+          <li>Let <var>value</var> be <var>url</var>
+          </li>
+          <li>If <a>Type</a>(<var>value</var>) it not <a>String</a>, return

I don't think this is necessary. (The WebIDL already guarantees that `url` is a string.) You can delete this.

> +          <a>string</a> that allows the developer to describe the purpose of
+          the shortcut. User agents may choose to expose this information to
+          assistive technology.
+        </p>
+      </section>
+      <section data-dfn-for="ShortcutItem" data-link-for="ShortcutItem">
+        <h3>
+          <code>url</code> member
+        </h3>
+        <p>
+          The <dfn>url</dfn> member of a <a>ShortcutItem</a> is a <a>URL</a> to
+          which a user agent should navigate when the associated shortcut is
+          activated.
+        </p>
+        <p>
+          The steps for <dfn>processing the <code>url</code> member of a

Given my comments below, this doesn't need to be a separate steps-for. In fact, these steps aren't called from the steps for processing ShortcutInfo, so the URL would never be parsed.

Delete this section, and just say "parse *shortcut*.`["url"]` using *manifest URL* as the base URL" up in the steps for processing ShortcutInfo.

> +          <dfn>Launching a shortcut</dfn>
+        </h3>
+        <p>
+          When <a>ShortcutItem</a> <var>shortcut</var> having
+          <a>WebAppManifest</a> <var>manifest</var> is invoked, run the
+          following steps:
+        </p>
+        <ol>
+          <li>Let <var>url</var> be <var>shortcut.url</var>.
+          </li>
+          <li>Let <var>browsing context</var> be the result of creating a new
+          <a>top-level browsing context</a>.
+          </li>
+          <li>
+            <a>Navigate</a> <var>browsing context</var> to a new <a>request</a>
+            whose url is <var>url</var>, header list is <var>header list</var>,

What is *header list*? That isn't mentioned elsewhere in the document, so I think it should be dropped from this sentence.

> +        </h3>
+        <p>
+          When <a>ShortcutItem</a> <var>shortcut</var> having
+          <a>WebAppManifest</a> <var>manifest</var> is invoked, run the
+          following steps:
+        </p>
+        <ol>
+          <li>Let <var>url</var> be <var>shortcut.url</var>.
+          </li>
+          <li>Let <var>browsing context</var> be the result of creating a new
+          <a>top-level browsing context</a>.
+          </li>
+          <li>
+            <a>Navigate</a> <var>browsing context</var> to a new <a>request</a>
+            whose url is <var>url</var>, header list is <var>header list</var>,
+            and body is <var>body</var>.

Same with *body*.

> +        <h3>
+          <dfn>Launching a shortcut</dfn>
+        </h3>
+        <p>
+          When <a>ShortcutItem</a> <var>shortcut</var> having
+          <a>WebAppManifest</a> <var>manifest</var> is invoked, run the
+          following steps:
+        </p>
+        <ol>
+          <li>Let <var>url</var> be <var>shortcut.url</var>.
+          </li>
+          <li>Let <var>browsing context</var> be the result of creating a new
+          <a>top-level browsing context</a>.
+          </li>
+          <li>
+            <a>Navigate</a> <var>browsing context</var> to a new <a>request</a>

*request* doesn't need to be a variable since it's never referred to again.

I don't think you really need to create a request here. Can you just say "Navigate *browsing context* to *url*"?

> +          The <a>icons</a> member of an <a>ShortcutItem</a> member is an
+          <a>array</a> of <a>ImageResource</a>s that can serve as iconic
+          representations of the shortcut in various contexts.
+        </p>
+      </section>
+      <section>
+        <h3>
+          <dfn>Launching a shortcut</dfn>
+        </h3>
+        <p>
+          When <a>ShortcutItem</a> <var>shortcut</var> having
+          <a>WebAppManifest</a> <var>manifest</var> is invoked, run the
+          following steps:
+        </p>
+        <ol>
+          <li>Let <var>url</var> be <var>shortcut.url</var>.

nit: shortcut.url should not be formatted as a variable, but as an object, like you see in [processing a manifest](https://www.w3.org/TR/appmanifest/#dfn-processing-a-manifest): `<var>shortcut</var>["<a>url</a>"]`.

> +          representations of the shortcut in various contexts.
+        </p>
+      </section>
+      <section>
+        <h3>
+          <dfn>Launching a shortcut</dfn>
+        </h3>
+        <p>
+          When <a>ShortcutItem</a> <var>shortcut</var> having
+          <a>WebAppManifest</a> <var>manifest</var> is invoked, run the
+          following steps:
+        </p>
+        <ol>
+          <li>Let <var>url</var> be <var>shortcut.url</var>.
+          </li>
+          <li>Let <var>browsing context</var> be the result of creating a new

Yep, the `launch` event was [specifically designed with this feature in mind](https://github.com/WICG/sw-launch/blob/master/explainer.md#background).

> @@ -5,7 +5,7 @@ node_js:
 
 branches:
   only:
-    - gh-pages
+    - master

This change seems incorrect and unrelated. We don't have a `master` branch; it's `gh-pages` (dating back to when GitHub forced you to use that branch name if you wanted to host content on github.io).

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/w3c/manifest/pull/768#pullrequestreview-297227574
Received on Friday, 4 October 2019 01:48:02 UTC