- From: Matt Giuca <notifications@github.com>
- Date: Thu, 13 Oct 2016 20:25:23 -0700
- To: w3c/manifest <manifest@noreply.github.com>
- Message-ID: <w3c/manifest/pull/506/review/4190216@github.com>
mgiuca commented on this pull request.
Thanks for getting this going, Marcos!
> + User agents that support an <a>automated install prompt</a> MUST run
+ the following steps before initiating an install process:
+ </p>
+ <ol>
+ <li>If a <a>manually</a> instantiated <a>installation process</a> for
+ this web application, terminate this algorithm.
+ </li>
+ <li>Let <var>event</var> be a newly constructed
+ <a>BeforeInstallPromptEvent</a> event named "beforeinstallprompt",
+ which does not bubble and is cancellable.
+ </li>
+ <li>Let <var>prompt promise</var> be the promise corresponding to the
+ event's prompt() method.
+ </li>
+ <li>Let <var>user-choice promise</var> be the promise corresponding
+ to the event's <code>prompt()</code> method.
s/prompt() method/userChoice attribute/
> + <li>If a <a>manually</a> instantiated <a>installation process</a> for
+ this web application, terminate this algorithm.
+ </li>
+ <li>Let <var>event</var> be a newly constructed
+ <a>BeforeInstallPromptEvent</a> event named "beforeinstallprompt",
+ which does not bubble and is cancellable.
+ </li>
+ <li>Let <var>prompt promise</var> be the promise corresponding to the
+ event's prompt() method.
+ </li>
+ <li>Let <var>user-choice promise</var> be the promise corresponding
+ to the event's <code>prompt()</code> method.
+ </li>
+ <li>Fire <var>event</var> at the <code>Window</code> object.
+ </li>
+ <li>Queue a task on the Realm’s settings object’s responsible event
nit: Should the curly quotes be straight quotes for consistency?
Also I don't know what a Realm is. Link?
> + <li>Fire <var>event</var> at the <code>Window</code> object.
+ </li>
+ <li>Queue a task on the Realm’s settings object’s responsible event
+ loop:
+ <ol>
+ <li>If <var>event</var>'s <a>cancel flag</a> is set, terminate
+ this algorithm.
+ </li>
+ <li>Otherwise, attempt to present the end-user with a UI that
+ lets them choose if they would like to install the web
+ application.
+ </li>
+ <li>If the previous step succeeds, resolve <var>prompt
+ promise</var>. Otherwise, reject <var>prompt promise</var> with
+ an ""AbortError", or "OperationError", or "NotAllowedError", or
+ "UnknownError" (depending on why the operation failed).
I think it should be more specific about when these errors happen.
> + <li>If <var>event</var>'s <a>cancel flag</a> is set, terminate
+ this algorithm.
+ </li>
+ <li>Otherwise, attempt to present the end-user with a UI that
+ lets them choose if they would like to install the web
+ application.
+ </li>
+ <li>If the previous step succeeds, resolve <var>prompt
+ promise</var>. Otherwise, reject <var>prompt promise</var> with
+ an ""AbortError", or "OperationError", or "NotAllowedError", or
+ "UnknownError" (depending on why the operation failed).
+ </li>
+ <li>In parallel, await the end-user's decision.
+ </li>
+ <li>Let <var>user choice</var> be the result of the previous
+ step, represented by either the string "accepted" or "dismissed".
nit: These string constants should be in tt font.
> @@ -233,6 +233,59 @@
</p>
<section>
<h3>
+ Installing via automated prompting
+ </h3>
+ <p>
+ User agents that support an <a>automated install prompt</a> MUST run
+ the following steps before initiating an install process:
+ </p>
+ <ol>
+ <li>If a <a>manually</a> instantiated <a>installation process</a> for
+ this web application, terminate this algorithm.
+ </li>
+ <li>Let <var>event</var> be a newly constructed
Should this reference the below "steps to construct a BeforeInstallPromptEvent event"?
> + </p>
+ <p>
+ Thus, the default action of the <a>BeforeInstallPromptEvent</a> is to
+ present an automated install prompt to the end user. Cancelling the
+ default action (via <code>.preventDefault()</code>) prevents the user
+ agent from presenting the automated install prompt.
+ </p>
+ <p>
+ To create an instance of the <code>BeforeInstallPromptEvent</code>
+ interface, run the <dfn>steps to construct a BeforeInstallPromptEvent
+ event</dfn>:
+ </p>
+ <ol>
+ <li>Let <var>userChoice</var> be null.
+ </li>
+ <li>Let [[\prompCalled]] be false.
promptCalled
> + </ol>
+ </li>
+ </ol>
+ <section>
+ <h4>
+ <code>prompt()</code> method
+ </h4>
+ <p>
+ The <dfn><code>prompt()</code> method</dfn>, when called, runs the
+ following steps:
+ </p>
+ <ol>
+ <li>If the event's cancel flag is not set, return a promise
+ rejected with an "InvalidStateError".
+ </li>
+ <li>If [[\]], return a promise rejected with an
If what? Is this supposed to be "If [[\promptCalled]] is true"?
> + </li>
+ <li>Fire <var>event</var> at the <code>Window</code> object.
+ </li>
+ <li>Queue a task on the Realm’s settings object’s responsible event
+ loop:
+ <ol>
+ <li>If <var>event</var>'s <a>cancel flag</a> is set, terminate
+ this algorithm.
+ </li>
+ <li>Otherwise, attempt to present the end-user with a UI that
+ lets them choose if they would like to install the web
+ application.
+ </li>
+ <li>If the previous step succeeds, resolve <var>prompt
+ promise</var>. Otherwise, reject <var>prompt promise</var> with
+ an ""AbortError", or "OperationError", or "NotAllowedError", or
nit: Double quote before AbortError. Also these should be formatted / linked properly.
> @@ -410,6 +463,94 @@ <h3 id="installation-sec">
</h2>
<section>
<h3>
+ <code>BeforeInstallPromptEvent</code>
+ </h3>
+ <pre class="idl">
+enum AppBannerPromptOutcome {
+ "accepted",
+ "dismissed"
+};
+
+[Constructor(DOMString typeArg, optional BeforeInstallPromptEventInit eventInit)]
+interface BeforeInstallPromptEvent : Event {
+ readonly attribute Promise<AppBannerPromptOutcome> userChoice;
The Chromium version of BeforeInstallPromptEventInit has a `platforms` attribute:
sequence<DOMString> platforms;
Is this deliberately omitted? (I seem to recall it was contentious but don't remember details.)
> + <code>BeforeInstallPromptEvent</code>
+ </h3>
+ <pre class="idl">
+enum AppBannerPromptOutcome {
+ "accepted",
+ "dismissed"
+};
+
+[Constructor(DOMString typeArg, optional BeforeInstallPromptEventInit eventInit)]
+interface BeforeInstallPromptEvent : Event {
+ readonly attribute Promise<AppBannerPromptOutcome> userChoice;
+ Promise<void> prompt();
+};
+
+dictionary BeforeInstallPromptEventInit : EventInit {
+ AppBannerPromptOutcome userChoice;
This is something you've added here (that isn't in Chrome), right?
TBH it's not clear to me why we want to allow web developers to construct their own `BeforeInstallPromptEvent` objects that aren't being delivered by the UA. There isn't much use to this. If we do want to allow it for completeness, we need to make sure that it's not possible to use these fabricated BIPE objects to show a prompt -- more on that later.
It's strange that we go to the effort of allowing the website to specify a `userChoice` here (to simulate a real BIPE) but we don't provide any mechanism for them to override the `prompt` method. I suppose they could just subclass `BeforeInstallPromptEvent`, but if we allow that, then they can also override `userChoice` in the subclass. So I still don't see the point of this.
But if we must let the client supply a userChoice here, why is it an `AppBannerPromptOutcome` and not a `Promise<AppBannerPromptOutcome>`? The way it is now, the client has to pass the user's actual choice (accepted or dismissed) to the constructor of the event, before the simulated prompt is even shown, and the promise gets immediately resolved during construction. Shouldn't this constructor take a promise that can be resolved at a later time?
> + prior to activating an <a>automated install prompt</a>, allowing a
+ developer to prevent the default action for an install prompt.
+ </p>
+ <p>
+ Thus, the default action of the <a>BeforeInstallPromptEvent</a> is to
+ present an automated install prompt to the end user. Cancelling the
+ default action (via <code>.preventDefault()</code>) prevents the user
+ agent from presenting the automated install prompt.
+ </p>
+ <p>
+ To create an instance of the <code>BeforeInstallPromptEvent</code>
+ interface, run the <dfn>steps to construct a BeforeInstallPromptEvent
+ event</dfn>:
+ </p>
+ <ol>
+ <li>Let <var>userChoice</var> be null.
These meta-variable names are confusing. *userChoicePromise* is `userChoice` (the `Promise<AppBannerPromptOutcome>`), and *userChoice* is the outcome that the promise is resolved with (the `AppBannerPromptOutcome`). That's messing with my head.
How about rename *userChoice* to *promptOutcome* (the outcome the promise is resolved with), and rename *userChoicePromise* to *userChoice* (the promise).
> + <code>prompt()</code> method
+ </h4>
+ <p>
+ The <dfn><code>prompt()</code> method</dfn>, when called, runs the
+ following steps:
+ </p>
+ <ol>
+ <li>If the event's cancel flag is not set, return a promise
+ rejected with an "InvalidStateError".
+ </li>
+ <li>If [[\]], return a promise rejected with an
+ "InvalidStateError".
+ </li>
+ <li>Let <var>p</var> be a new promise.
+ </li>
+ <li>Run the following steps in parallel:
Missing steps. Should refer to the above "steps before initiating an install process".
> @@ -233,6 +233,59 @@
</p>
<section>
<h3>
+ Installing via automated prompting
+ </h3>
+ <p>
+ User agents that support an <a>automated install prompt</a> MUST run
+ the following steps before initiating an install process:
+ </p>
+ <ol>
+ <li>If a <a>manually</a> instantiated <a>installation process</a> for
Why not just make this algorithm the "steps for an automatically instantiated installation process" and drop this first early-exit step?
> + <section>
+ <h4>
+ <code>prompt()</code> method
+ </h4>
+ <p>
+ The <dfn><code>prompt()</code> method</dfn>, when called, runs the
+ following steps:
+ </p>
+ <ol>
+ <li>If the event's cancel flag is not set, return a promise
+ rejected with an "InvalidStateError".
+ </li>
+ <li>If [[\]], return a promise rejected with an
+ "InvalidStateError".
+ </li>
+ <li>Let <var>p</var> be a new promise.
As discussed above, we need to make sure that the following code *fails*:
```js
var e = new BeforeInstallPromptEvent({});
e.preventDefault();
e.prompt()
.then(() => console.log('prompt() succeeded'))
.catch(e => console.error('prompt() failed: ' + e));
```
That is, you must not be able to fabricate a `BeforeInstallPromptEvent` object, call `preventDefault` on it (to set its cancel flag), then call `prompt` on it to show an install banner without the UA's permission.
As I said above, I'm not sure why we allow these events to be fabricated at all, but if we do, there needs to be a check that this event was created by the UA and not by the site itself.
> + "InvalidStateError".
+ </li>
+ <li>Let <var>p</var> be a new promise.
+ </li>
+ <li>Run the following steps in parallel:
+ </li>
+ <li>Return <var>p</var>.
+ </li>
+ </ol>
+ </section>
+ <section>
+ <h4>
+ <code>userChoice</code> attribute
+ </h4>
+ <p>
+ The <dfn><code>userChoice</code> attribute</dfn>, when getting,
"when getting"? Can we reword this?
> @@ -410,6 +463,94 @@ <h3 id="installation-sec">
</h2>
<section>
<h3>
+ <code>BeforeInstallPromptEvent</code>
+ </h3>
+ <pre class="idl">
+enum AppBannerPromptOutcome {
+ "accepted",
+ "dismissed"
+};
+
+[Constructor(DOMString typeArg, optional BeforeInstallPromptEventInit eventInit)]
+interface BeforeInstallPromptEvent : Event {
+ readonly attribute Promise<AppBannerPromptOutcome> userChoice;
I find the fact that this is a promise attribute (as opposed to a function that returns a promise) really weird. I know it's what Chrome does. I wonder if it's too late to change it?
It causes a lot of weirdness because there are side-effects upon reading the attribute. For example, in Chrome a user-constructed `BeforeInstallPromptEvent` logs a console error just by the event object itself being printed to the console, because it tries to read the `userChoice` attribute. And [this](https://bugs.chromium.org/p/chromium/issues/detail?id=655877).
> + </li>
+ <li>Let <var>p</var> be a new promise.
+ </li>
+ <li>Run the following steps in parallel:
+ </li>
+ <li>Return <var>p</var>.
+ </li>
+ </ol>
+ </section>
+ <section>
+ <h4>
+ <code>userChoice</code> attribute
+ </h4>
+ <p>
+ The <dfn><code>userChoice</code> attribute</dfn>, when getting,
+ returns a promise.
returns *userChoicePromise*.
I know that variable is kind scoped to the `BeforeInstallPromptEvent` constructor... hmm. I think we need some way to refer to these promises across the different algorithms. There's also weirdness that the "steps before initiating an install process" refer to these promises as "the promise corresponding to the event's *X* method", rather than by name.
> @@ -441,6 +583,17 @@ <h3 id="installation-sec">
</div>
<section>
<h4>
+ <code>onbeforeinstallprompt</code> attribute
Should this come after onappinstalled, to match the order they appear in the IDL?
--
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/506#pullrequestreview-4190216
Received on Friday, 14 October 2016 03:25:55 UTC