Re: [w3c/manifest] Feat(events): add BeforeInstallPromptEvent (#520)

mgiuca approved this pull request.

Great to hear that!

I put a few more review comments (sorry I didn't do this earlier). (I vaguely feel like we've talked about these but I can't find evidence of that now.)

To summarize our (Google) current feelings on this, I think we can proceed with this API (we'd rather standardize this thing that's already implemented) but we do have some doubts about this that might warrant a more direct (top-level) API in the future.

The main concern is re-prompting: the current API really only gives the user one shot at it; if they cancel, then the site has no way to trigger another prompt programmatically until another beforeinstallprompt event is fired. This could be seen as an intentional restriction (the site should not be spamming prompts), but let's say we want to design a browser that lets you show a cancelled prompt 3 times before rate limiting you to no more prompts for the next 5 minutes. The only way to implement that with the current design would be to fire a second beforeinstallprompt event immediately after the first and second ones are cancelled.

We can work with this but it is something to consider for future iterations that it is not ideal.

Having said that, approve with a few noted comments.

> +            </li>
+            <li>Resolve <var>p</var> with
+            <var>this</var>.<a>[[\userResponsePromise]]</a>.
+            </li>
+            <li>If <var>this</var>.<a>[[\userResponsePromise]]</a> is pending:
+              <ol>
+                <li>If this event's <code>isTrusted</code> attribute is <code>
+                  false</code>, reject
+                  <var>this</var>.<a>[[\userResponsePromise]]</a> with
+                  <a>NotAllowedError</a>, optionally informing the developer
+                  that untrusted events can't call <code>prompt()</code>.
+                </li>
+                <li>Else if <var>this</var>.<a>[[\didPrompt]]</a> is
+                <code>false</code>, then, <a>in parallel</a>, <a>request to
+                present an install prompt</a> with this event. Wait, possibly
+                indefinitely, for the end-user to make a choice.

Need to set didPrompt to true here (otherwise multiple prompts can be shown if this is called again before the first one resolves).

> +          </dt>
+          <dd>
+            A promise that represents the outcome of <a>presenting an install
+            prompt</a>.
+          </dd>
+        </dl>
+        <section>
+          <h4>
+            <code>prompt()</code> method
+          </h4>
+          <p>
+            The <dfn>prompt</dfn> method, when called, runs the following
+            steps:
+          </p>
+          <ol>
+            <li>Let <var>p</var> be a newly created promise.

Why create *p* and immediately resolve it with `[[userResponsePromise]]`?

Can't you simply delete 1 and 2 here, and in the final step, return `[[userResponsePromise]]`?

> +        <section class="informative">
+          <h4>
+            Usage example
+          </h4>
+          <p>
+            This example shows how one might prevent an automated install
+            prompt from showing until the user has finished a set of tasks.
+            Those tasks are represented as an array of promises, which the
+            application "awaits" to finish before an install prompt is
+            presented to the end-user.
+          </p>
+          <pre class="example" title="'beforeinstallprompt' in action">
+            window.addEventListener("beforeinstallprompt", async (event) =&gt; {
+              event.preventDefault();
+              // Wait for e.g., the user to request installation from inside the app.
+              await Promise.all(tasksThatPreventsInstallation);

I think I've said this before elsewhere but I'm not a huge fan of this example. It's quite unclear what "tasks that prevents installation" means, and it doesn't speak to the main use case we have in mind for this feature (which is not blocking on some task, but waiting for user input, e.g., clicking an "install me" button on a site).

Can we rework this to have the code be

1. Upon receipt of the beforeinstallprompt event, set a button in the site UI to "enabled"?
2. Await pressing the button.
3. Call `prompt()`.

-- 
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/520#pullrequestreview-25964659