- From: Matt Giuca <notifications@github.com>
- Date: Sun, 23 Oct 2016 23:24:39 -0700
- To: w3c/manifest <manifest@noreply.github.com>
- Message-ID: <w3c/manifest/pull/506/review/5376534@github.com>
mgiuca commented on this pull request.
Big lot of comments for discussion.
Thanks for doing this, it looks pretty great overall.
> + throw new DOMException(msg, "InvalidStateError");
+ }
+
+ if (internalSlots.get(this).didPrompt) {
+ const msg = ".prompt() can only be successfully called once.";
+ throw new DOMException(msg, "InvalidStateError");
+ }
+ requestInstallPrompt(this);
+ }
+
+ get userChoice() {
+ return internalSlots.get(this).userChoice;
+ }
+ }
+
+ async function notifyBeforeInstallPrompt(element) {
How about calling this `simulateBeforeInstallPrompt`?
(Since it is not part of the BIP API surface, it's just used by the buttons on the test page to simulate an action the UA normally takes.)
> + }
+
+ if (internalSlots.get(this).didPrompt) {
+ const msg = ".prompt() can only be successfully called once.";
+ throw new DOMException(msg, "InvalidStateError");
+ }
+ requestInstallPrompt(this);
+ }
+
+ get userChoice() {
+ return internalSlots.get(this).userChoice;
+ }
+ }
+
+ async function notifyBeforeInstallPrompt(element) {
+ if (document.readyState === "complete") {
This should be `!==`
> +window.addEventListener("appinstalled", () => {
+ console.info("Application installed successfully");
+});
+
+window.addEventListener("beforeinstallprompt", test1, {
+ once: true
+});
+
+function test1(e) {
+ console.info("Running test 1");
+ const assertion = "Throws when preventDefault not called";
+ try {
+ e.prompt();
+ console.assert(false, assertion);
+ } catch (err) {
+ console.assert(err.name === "InvalidStateError", assertion);
I'd prefer this has a different assertion message than the above. It makes it hard to figure out whether it isn't throwing, or whether it's just the wrong type. (e.g., I tried this and got "NotAllowedError" due to not being trusted with the polyfill.)
How about:
```js
try {
e.prompt();
console.assert(false, "Throws when preventDefault not called");
} catch (err) {
console.assert(err.name === "InvalidStateError",
"Caught " + err.name + "; expected InvalidStateError");
}
```
> + }
+ try {
+ e.preventDefault();
+ e.prompt();
+ window.addEventListener("appinstalled", setupTest2, {
+ once: true
+ });
+ } catch (err) {
+ console.assert(false, "Unexpected exceptions");
+ } {
+ const assertion = "Calling prompt() twice is an InvalidStateError";
+ try {
+ e.prompt();
+ console.assert(false, assertion);
+ } catch (err) {
+ console.assert(err.name === "InvalidStateError", assertion);
Same here.
> + didPrompt: false,
+ userChoice: null,
+ userChoiceResolver: null, // Implicit in spec
+ };
+
+ internal.userChoice = new Promise((resolve) => {
+ if (eventInit && "userChoice" in eventInit) {
+ return resolve(eventInit.userChoice);
+ }
+ internal.userChoiceResolver = resolve;
+ });
+ internalSlots.set(this, internal);
+ }
+
+ prompt() {
+ if (this.isTrusted === false) {
This still requires a real, trusted BIP event. I'm not quite sure what you're trying to do here because this can never truly work (i.e., this is *inside* the definition of the polyfill `BeforeInstallPromptEvent` yet its own `prompt` method requires a real `BeforeInstallPromptEvent`).
Are you trying to make this a sort of formal JavaScript language version of the spec ("how it should be") even though it can't work? Or are you trying to make a polyfill that can actually be used? You could do the `simulatedIsTrusted` thing I suggested on the previous review round to make this actually work.
Your response to my suggestion was:
> Within a few weeks, we will have both Chrome and Firefox implementing this for real in C++, so let's keep the current sample implementation as is.
But that doesn't change the fact that this sample implementation is non-functional.
> +</style>
+<button onclick="notifyBeforeInstallPrompt(this)">
+ Simulate install BIP
+</button>
+<button onclick="showInstallPrompt(this)">
+ Simulate manual "Add to Home Screen"
+</button>
+<script>
+/*globals BeforeInstallPromptEvent, DOMException*/
+"use strict";
+//
+{
+ const assertion = "No arguments should throw";
+ try {
+ new BeforeInstallPromptEvent();
+ console.assert(false, assertion);
I'd prefer if a) the assertion strings were inline, and b) they were different on every assert.
(It's a good policy in general to never have two lines that can throw the same error message, so you can tell which one failed -- especially if the error reporting tool doesn't show line numbers as is the case at least in Chrome.)
Also I think having the assertion string inline makes it much more readable.
I noted some specific cases below.
> + Simulate install BIP
+</button>
+<button onclick="showInstallPrompt(this)">
+ Simulate manual "Add to Home Screen"
+</button>
+<script>
+/*globals BeforeInstallPromptEvent, DOMException*/
+"use strict";
+//
+{
+ const assertion = "No arguments should throw";
+ try {
+ new BeforeInstallPromptEvent();
+ console.assert(false, assertion);
+ } catch (err) {
+ console.assert(true, assertion);
Is there any reason to `assert(true)`? Surely this never has any effect?
> + *
+ */
+ class BeforeInstallPromptEvent extends Event {
+
+ constructor(typeArg, eventInit) {
+ // WebIDL Guard. Not in spec, as it's all handled by WebIDL.
+ if (arguments.length === 0) {
+ throw new TypeError("Not enough arguments. Expected at least 1.");
+ }
+ const initType = typeof eventInit;
+ if (arguments.length === 2 && initType !== "undefined" && initType !== "object") {
+ throw new TypeError("Value can't be converted to a dictionary.");
+ }
+ super(typeArg, Object.assign({ cancelable: true }, eventInit));
+
+ if (eventInit && typeof eventInit.userChoice !== "undefined" && !AppBannerPromptOutcome.has(String(eventInit.userChoice))) {
nit: Super long line, consider breaking?
> + console.assert(false, assertion);
+ } catch (err) {
+ console.assert(true, assertion);
+ }
+} {
+ const assertion = "BeforeInstallPromptEvent is cancelable by default";
+ const e1 = new BeforeInstallPromptEvent("");
+ console.assert(e1.cancelable === true, assertion);
+ const e2 = new BeforeInstallPromptEvent("", {
+ cancelable: true
+ });
+ console.assert(e2.cancelable === true, assertion);
+ const e3 = new BeforeInstallPromptEvent("", {
+ cancelable: false
+ });
+ console.assert(e3.cancelable === false, assertion);
FYI: Chrome currently fails this. [crbug.com/658640](https://crbug.com/658640).
> + const assertion = "prompt() is a function";
+ const e = new BeforeInstallPromptEvent("");
+ console.assert(e.prompt instanceof Function, assertion);
+} {
+ const assertion = "prompt() takes no arguments";
+ const e = new BeforeInstallPromptEvent("");
+ console.assert(e.prompt.length === 0, assertion);
+} {
+ const assertion = "calling prompt() with untrusted event throws NotAllowedError";
+ const e = new BeforeInstallPromptEvent("");
+ try {
+ e.prompt();
+ console.assert(false, assertion);
+ } catch (err) {
+ console.assert(err instanceof DOMException, assertion);
+ console.assert(err.name === "NotAllowedError", assertion);
FYI: Chrome currently fails this. [crbug.com/658639](https://crbug.com/658639).
> + console.assert("prompt" in e, assertion);
+} {
+ const assertion = "prompt() is a function";
+ const e = new BeforeInstallPromptEvent("");
+ console.assert(e.prompt instanceof Function, assertion);
+} {
+ const assertion = "prompt() takes no arguments";
+ const e = new BeforeInstallPromptEvent("");
+ console.assert(e.prompt.length === 0, assertion);
+} {
+ const assertion = "calling prompt() with untrusted event throws NotAllowedError";
+ const e = new BeforeInstallPromptEvent("");
+ try {
+ e.prompt();
+ console.assert(false, assertion);
+ } catch (err) {
FYI: Chrome currently returns a rejectable promise rather than throwing an exception. [crbug.com/658638](https://crbug.com/658638).
> + userChoice: "dismissed"
+ });
+ console.assert(true, assertion);
+ } catch (err) {
+ console.assert(false, assertion, err);
+ }
+} {
+ const assertion = "Resolves userChoice promise with enum values";
+ const b1 = new BeforeInstallPromptEvent("", {
+ userChoice: "accepted"
+ });
+ const b2 = new BeforeInstallPromptEvent("", {
+ userChoice: "dismissed"
+ });
+ Promise
+ .all([b1.userChoice, b2.userChoice])
I'm still not sure this is the right behaviour for the `userChoice` argument. Or even if we want it. Will discuss on spec.
> - instance, there are sufficient <a>installability signals</a> to warrant
- <a>installation</a> of the web application.
+ trigger the <a>installation process</a> through the user agent's
+ <abbr>UI</abbr>.
+ </p>
+ <p>
+ Alternatively, the <a>installation process</a> can occur through an
+ <dfn>automated install prompt</dfn>: that is, a <abbr>UI</abbr> that
+ the user agent presents to the user when, for instance, there are
+ sufficient <a>installability signals</a> to warrant <a>installation</a>
+ of the web application.
+ </p>
+ <p>
+ In either case, when a user agent <dfn data-lt=
+ "present an install prompt">presents an install prompt</dfn>, the
+ end-user's choice is represented as on of the following values (either
s/on/one
> + sufficient <a>installability signals</a> to warrant <a>installation</a>
+ of the web application.
+ </p>
+ <p>
+ In either case, when a user agent <dfn data-lt=
+ "present an install prompt">presents an install prompt</dfn>, the
+ end-user's choice is represented as on of the following values (either
+ "accepted" or "dismissed"). These values are represented in the API of
+ this specification via <a>AppBannerPromptOutcome</a> enum.
+ </p>
+ <dl data-dfn-for="AppBannerPromptOutcome">
+ <dt>
+ <dfn>accepted</dfn>:
+ </dt>
+ <dd>
+ The end-user indicated that the user agent MUST attempt to
Is "MUST" meaningful alongside the word "attempt"?
A user agent might "attempt" to install the application but fail once it realises it can't be bothered, it hasn't been implemented, it's not possible, etc. So the "MUST" is kind of meaningless then, isn't it?
> + </li>
+ <li>If there is already an <a>installation process</a> being
+ <a data-lt="present an install prompt">presented</a>, terminate this
+ algorithm.
+ </li>
+ <li>Let <var>event</var> be a <a data-lt=
+ "construct a BeforeInstallPromptEvent">newly constructed</a>
+ <a>BeforeInstallPromptEvent</a> named "<a>beforeinstallprompt</a>",
+ which is cancelable.
+ </li>
+ <li>
+ <a>Queue a task</a> on the <a>application life-cycle task
+ source</a> to <a>fire</a> <var>event</var> at the <a>Window</a>
+ object of the <a>top-level browsing context</a>.
+ </li>
+ <li>If <var>event</var>'s <a>canceled flag</a> is not set, then
Doesn't this step have to take place after the event is fired? Otherwise it would not be cancelled because the event handler hasn't happened yet.
> @@ -330,8 +392,8 @@
populate an <a>installation process</a>' UI.
</li>
<li>If the <a>installation succeeded</a>, <a>queue a task</a> on
- the the <dfn>application life-cycle task source</dfn> to <a>fire
- an event</a> named <code>appinstalled</code> at the
+ the <a>application life-cycle task source</a> to <a>fire an
Can this change (and the above removal of Installation) just be put into the spec before this PR, just to avoid having this large commit be polluted by non-related changes?
> + install prompt</a>.
+ </p>
+ </section>
+ <section data-dfn-for="BeforeInstallPromptEvent">
+ <h3>
+ <code>BeforeInstallPromptEvent</code> Interface
+ </h3>
+ <pre class="idl">
+ [Constructor(DOMString typeArg, optional BeforeInstallPromptEventInit eventInit)]
+ interface BeforeInstallPromptEvent : Event {
+ readonly attribute Promise<AppBannerPromptOutcome> userChoice;
+ Promise<void> prompt();
+ };
+
+ dictionary BeforeInstallPromptEventInit : EventInit {
+ AppBannerPromptOutcome userChoice;
There was a discussion about this on the previous review round that went unfinished:
[My previous statement](https://github.com/w3c/manifest/pull/506/files/4af07193642bf9ae5d3f002337c64b196b0db908#r84207025)
I still don't see the point of this `userChoice` constructor argument being not a promise, implying that you've already made the choice and now you only have to understand why*. It _might_ be useful if `userChoice` was a promise, allowing the person fabricating this event to allow the choice to be resolved at some future point. But even then, I see little point in actually supporting the event fabrication use case at all. This would all (speccing, implementation, testing) be a lot easier if we banned fabrication altogether, or if the Web Platform doesn't allow that, just letting it exist but not do much (and not specifically inventing features to support this useless use case).
If you can justify why someone (a web developer, not a UA implementor) would want to create a `BeforeInstallPromptEvent` with a `userChoice` -- even a promise -- then I'm all ears.
\* Bonus points if you get that reference.
> + developer that this event has expired.
+ </li>
+ <li>
+ <a>Request to present an install prompt</a> with this event.
+ </li>
+ </ol>
+ <p>
+ To <dfn>request to present an install prompt</dfn> with
+ <a>BeforeInstallPromptEvent</a> <var>event</var>:
+ </p>
+ <ol>
+ <li>Set <a>[[\didPrompt]]</a> of <var>event</var> to
+ <code>true</code>.
+ </li>
+ <li>
+ <a>In parallel</a>:
How can this be done in parallel? Step 2.2 is dependent on the results of Step 2.1.
> + <a>Present an install prompt</a> and let
+ <var>promptOutcome</var> be the result.
+ </li>
+ <li>Resolve <a>[[\userChoice]]</a> with
+ <var>promptOutcome</var>.
+ </li>
+ </ol>
+ </li>
+ </ol>
+ </section>
+ <section>
+ <h4>
+ <code>userChoice</code> attribute
+ </h4>
+ <p>
+ The <dfn>userChoice</dfn> attribute, when getting, return the
s/return/returns?
(Maybe? I really don't understand the grammar of this awkward sentence, but as you mentioned previously it is spec language.)
> + promise held by the <a>[[\userChoice]]</a> internal slot.
+ </p>
+ </section>
+ <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">
+ const tasksThatPreventInstallation = [];
Can this be `[...]` to imply it's got stuff in it, rather than just being an empty array?
Another way to express this would be to show a single task rather than an array. Perhaps, `waitForUserClickOnInstall`?
```js
event.preventDefault();
await waitForUserClickOnInstall;
event.prompt();
```
> <section>
<h3>
Extensions to the <code>Window</code> object
</h3>
<p>
The following extensions to the <code>Window</code> object specify
the <a>event handler attributes</a> on which events relating to the
- <a>installation</a> of a web application are fired.
+ <a>installation</a> of a web application are <a>fired</a>.
This could be taken out of this PR (as mentioned above).
> };
</pre>
<div class="example">
<p>
- This example shows two ways of handling the "appinstalled" event.
+ This example shows two ways of handling the "<a>appinstalled</a>"
This could be taken out of this PR (as mentioned above).
> <h4>
<code>onappinstalled</code> attribute
</h4>
<p>
- <dfn for="Window">onappinstalled</dfn> is an <a>event handler IDL
- attribute</a> for the "<a>appinstalled</a>" event type. The
+ The <dfn>onappinstalled</dfn> is an <a>event handler IDL
This could be taken out of this PR (as mentioned above).
> @@ -2319,10 +2563,9 @@ <h3 id="applying">
purposes</a>.
</p>
<p>
- When an <a>image object</a> is used as an <a>icon</a>, a developer
- can hint that the image is intended to serve some special
- <dfn>purpose</dfn> in the context of the host OS (i.e., for better
- integration).
+ When an <a>image object</a> is used as an icon, a developer can hint
This could be taken out of this PR (as mentioned above).
> <a href="https://dom.spec.whatwg.org/#event"><dfn><code>Event</code>
interface</dfn></a>
</li>
<li>
- <a href="https://dom.spec.whatwg.org/#concept-event-fire"><dfn>Fire
- an event</dfn></a>
+ <a href=
This could be taken out of this PR (as mentioned above).
--
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-5376534
Received on Monday, 24 October 2016 06:26:47 UTC