Re: [w3c/manifest] WIP: beforeinstallprompt (#506)

mgiuca commented on this pull request.

Some comments for the implementation.

> +    .then(value => console.assert(value === "dismissed"))
+    .catch(err => console.assert(false, assertion, err));
+} {
+  const assertion = "BeforeInstallPromptEvent has a prompt() method";
+  const e = new BeforeInstallPromptEvent("");
+  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 unstrusted event results in NotAllowedError error";

unstrusted -> untrusted

> +      const promptOutcome = await showInstallPrompt();
+      internalSlots.get(this).userChoiceHandlers.resolve(promptOutcome);
+    }.bind(this)())
+  }
+
+  get userChoice() {
+    return internalSlots.get(this).userChoice;
+  }
+}
+
+async function notifyBeforeInstallPrompt(element) {
+  await trackReadyState();
+  if (installProcesses.length) {
+    return;
+  }
+  const event = new BeforeInstallPromptEvent("beforeinstallprompt");

Shouldn't this be setting the trusted flag? I don't see any code that sets this flag to true, and when I click the button I get errors:

    Assertion failed: is trusted Event {isTrusted: false, type: "beforeinstallprompt", target: Window, currentTarget: Window, eventPhase: 2…}    index.html:241
    Uncaught DOMException: Untrusted events can't call prompt().    BeforeInstallPromptEvent.js:52

OK I played with it and you can't set `isTrusted` because the Event model doesn't let you. But I was able to simulate setting the trusted flag with this. You could do something like:

```diff
diff --git a/implementation/BeforeInstallPromptEvent.js b/implementation/BeforeInstallPromptEvent.js
index d9f31ba..f370598 100644
--- a/implementation/BeforeInstallPromptEvent.js
+++ b/implementation/BeforeInstallPromptEvent.js
@@ -26,6 +26,7 @@ class BeforeInstallPromptEvent extends Event {
     }
     // End WebIDL guard.
     const internal = {
+      simulatedTrusted: false,
       didPrompt: false,
     };
 
@@ -47,7 +48,7 @@ class BeforeInstallPromptEvent extends Event {
       internalSlots.get(this).didPrompt = true;
     }
 
-    if (this.isTrusted === false) {
+    if (this.isTrusted === false && internalSlots.get(this).simulatedTrusted === false) {
       const msg = "Untrusted events can't call prompt().";
       throw new DOMException(msg, "NotAllowedError");
     }
@@ -74,6 +75,7 @@ async function notifyBeforeInstallPrompt(element) {
     return;
   }
   const event = new BeforeInstallPromptEvent("beforeinstallprompt");
+  internalSlots.get(event).simulatedTrusted = true;
   window.dispatchEvent(event);
   if (!event.defaultPrevented) {
     await showInstallPrompt(element);
```

> @@ -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&lt;AppBannerPromptOutcome&gt; userChoice;

As discussed, we will omit it for now because it's not really clear whether we want this in the final spec.

> +        <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.

s/perfrom/perform

> +          <code>BeforeInstallPromptEvent</code>
+        </h3>
+        <pre class="idl">
+enum AppBannerPromptOutcome {
+  "accepted",
+  "dismissed"
+};
+
+[Constructor(DOMString typeArg, optional BeforeInstallPromptEventInit eventInit)]
+interface BeforeInstallPromptEvent : Event {
+    readonly attribute Promise&lt;AppBannerPromptOutcome&gt; userChoice;
+    Promise&lt;void&gt; prompt();
+};
+
+dictionary BeforeInstallPromptEventInit : EventInit {
+  AppBannerPromptOutcome userChoice;

I think we have to ask what is the use case for synthetically constructing BIP events (aside from "the web platform usually allows this"). Since the BIP's primary utility (`preventDefault` + `prompt`) is not available to fabricated events, why would you ever want to do this?

The only thing I can think of is that you might make a fake dialog (non-cancellable/repromptable) and they want to deliver a fake BIP event as the fake dialog is being shown. This design makes it impossible for `userChoice` to be meaningful, since you have to decide on the prompt outcome at the time you create the event, not asynchronously.

> they probably know what they are doing - so I would prefer just to keep it as simple as possible

I would complete that sentence as "they probably know what they are doing - so we should give them maximum flexibility" (i.e., let them specify a promise).

If not, then I don't see a point in allowing this event type to be synthesizable at all. It would certainly reduce the complexity of the whole thing; you wouldn't need the "trusted" vs "untrusted" concept.

> @@ -0,0 +1,143 @@
+/*globals BeforeInstallPromptEvent, DOMException*/
+"use strict";
+const internalSlots = new WeakMap();
+const installProcesses = [];
+const AppBannerPromptOutcome = new Set([
+  "accepted",
+  "dismissed",
+]);
+
+class BeforeInstallPromptEvent extends Event {
+  constructor(typeArg, eventInit) {
+    // WebIDL Guard. Wont be in spec in spec as it's all handled by WebIDL.

s/Wont be in spec in spec/Not in spec.

> +  prompt.id = "installprompt";
+  prompt.innerHTML = `
+    <h2>Add to Home screen</h2>
+    <p>... the foo app ...</p>
+    <button id="cancel">CANCEL</button>
+    <button id="add">ADD</button>
+  `;
+  const cancel = prompt.querySelector("#cancel");
+  const add = prompt.querySelector("#add");
+  document.body.appendChild(prompt);
+  const p = new Promise((resolve) => {
+    add.addEventListener("click", () => {
+      resolve("accepted");
+      //emulate installation to home screen
+      setTimeout(()=>{
+        window.dispatchEvent(new Event("install"));

appinstalled

> +    .then(value => console.assert(value === "dismissed"))
+    .catch(err => console.assert(false, assertion, err));
+} {
+  const assertion = "BeforeInstallPromptEvent has a prompt() method";
+  const e = new BeforeInstallPromptEvent("");
+  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 unstrusted event results in NotAllowedError error";

unstrusted

> +    console.assert(false, assertion);
+  } catch (err) {
+    console.assert(err instanceof DOMException, assertion);
+    console.assert(err.type = "NotAllowedError", assertion);
+  }
+  try {
+    e.prompt();
+    console.assert(false, assertion);
+  } catch (err) {
+    console.assert(err instanceof DOMException, assertion);
+    console.assert(err.type = "InvalidStateError", assertion);
+  }
+}
+
+// install events
+window.addEventListener("install", (ev) => {

appinstalled

> @@ -0,0 +1,143 @@
+/*globals BeforeInstallPromptEvent, DOMException*/
+"use strict";
+const internalSlots = new WeakMap();

I'm not a fan of this approach (though it's just proof-of-concept code so it doesn't really matter).

Can you just set a field called `__internal` on the BIPE object instead of using a global map?

> +    }
+    super(typeArg, Object.assign({ cancelable: true }, eventInit));
+
+    if (eventInit && typeof eventInit.userChoice !== "undefined" && !AppBannerPromptOutcome.has(String(eventInit.userChoice))) {
+      const msg = `The provided value '${eventInit.userChoice}' is not a valid` +
+        "enum value of type AppBannerPromptOutcome.";
+      throw new TypeError(msg);
+    }
+    // End WebIDL guard.
+    const internal = {
+      didPrompt: false,
+    };
+
+    internal.userChoice = new Promise((resolve) => {
+      internal.userChoiceHandlers = {
+        resolve,

I'm incredibly surprised that this is a thing in JS! Fancy, but I think I'd prefer `resolve: resolve` for clarity.

Or, why does it need to be a dictionary at all? Why not `internal.userChoiceResolve = resolve`?

> +    const internal = {
+      didPrompt: false,
+    };
+
+    internal.userChoice = new Promise((resolve) => {
+      internal.userChoiceHandlers = {
+        resolve,
+      };
+      if (eventInit && "userChoice" in eventInit) {
+        resolve(eventInit.userChoice);
+      }
+    });
+    internalSlots.set(this, internal);
+  }
+  prompt() {
+    if (internalSlots.get(this).didPrompt) {

```js
const internal = internalSlots.get(this)
```

To avoid calling this many times.

> +  }
+}
+
+async function notifyBeforeInstallPrompt(element) {
+  await trackReadyState();
+  if (installProcesses.length) {
+    return;
+  }
+  const event = new BeforeInstallPromptEvent("beforeinstallprompt");
+  window.dispatchEvent(event);
+  if (!event.defaultPrevented) {
+    await showInstallPrompt(element);
+  }
+}
+
+function trackReadyState() {

`async`

> @@ -0,0 +1,143 @@
+/*globals BeforeInstallPromptEvent, DOMException*/

Should this file be surrounded in a closure so it hides all the other variables?

> +}
+
+function trackReadyState() {
+  return new Promise((resolve) => {
+    if (document.readyState === "complete") {
+      return resolve();
+    }
+    document.addEventListener("readystatechange", () => {
+      if (document.readyState === "complete") {
+        resolve();
+      }
+    });
+  });
+}
+
+window.addEventListener("beforeinstallprompt", async function(event) {

It's weird that this code is here. I think of this file as being a polyfill for `BeforeInstallPromptEvent`, so it provides the event object as well as the methods for simulating the system sending those events (`notifyBeforeInstallPrompt`). It shouldn't also have a handler for the event which automatically calls preventDefault and prompt.

It's even weirder because `index.html` has its own BIP handler which may race with this one.

> +  {
+    const assertion = "only fires after document is fully loaded";
+    console.assert(document.readyState === "complete", assertion);
+  } {
+    const assertion = "is instance of BeforeInstallPromptEvent";
+    console.assert(ev instanceof BeforeInstallPromptEvent, assertion);
+  } {
+    const assertion = "is trusted";
+    console.assert(ev.isTrusted, assertion, ev);
+  } {
+    const assertion = "calling prompt successfully returns void";
+    console.assert(typeof ev.prompt() === "undefined", assertion);
+  } {
+    const assertion = "Even with success, prompt() can only be called once";
+    try {
+      ev.prompt();

Why does this call `prompt` without first calling `preventDefault`?

I'm confused because you have another handler (in BIPE.js) that calls preventDefault and prompt. This seems to be testing that that event was handled first, before this one... but as we discussed previously the order isn't guaranteed. I think it would be best to have just a single bipe handler (in this file, not the other one) that tests all of these things.

-- 
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-4986447