Feat(events): add BeforeInstallPromptEvent #520

marcoscaceres · 2016-11-07T09:42:42Z

Defines BeforeInstallPromptEvent, related Dictionaries, and .prompt() method as recently discussed in the relevant bug.

marcoscaceres · 2016-11-07T09:43:06Z

This is what the text is based on:

const internalSlots = new WeakMap();
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 && !hasValidUserChoice(eventInit)) {
      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.userResponsePromise = new Promise((resolve, reject) => {
      internal.resolvePromptPromise = resolve;
      internal.rejectPromptPromise = reject;
    });
    internalSlots.set(this, internal);
    if (eventInit && eventInit.userChoice) {
      const promptResponseObject = {
        userChoice: eventInit.userChoice,
      };
      internal.resolvePromptPromise(promptResponseObject);
    }
  }

  async prompt() {
    const internal = internalSlots.get(this);
    if (!this.isTrusted) {
      const msg = ".prompt() can only called by trusted events.";
      internal.rejectPromptPromise(new DOMException(msg, "NotAllowedError"));
    } else if (!internal.didPrompt) {
      await requestInstallPrompt(this);
    }
    return internal.userResponsePromise;
  }
}

marcoscaceres · 2016-11-07T09:44:30Z

There is one more medium sized PR coming tomorrow that defines the final steps.

marcoscaceres · 2016-11-07T10:22:50Z

Fixed a couple of little things, so updated PR...

marcoscaceres · 2016-11-08T07:46:42Z

index.html

+        <pre class="idl">
+          [Constructor(DOMString typeArg, optional BeforeInstallPromptEventInit eventInit)]
+          interface BeforeInstallPromptEvent : Event {
+              readonly attribute Promise&lt;AppBannerPromptOutcome&gt; userChoice;


Oops, forgot to remove... removing now

mgiuca

Looks pretty good. I have some quick comments, but I haven't had a deeper look at it and I want to do that tomorrow (on my workstation where I've got the repo checked out so I can build it and look at it in HTML). Can we wait until then to land this?

mgiuca · 2016-11-08T08:02:30Z

index.html

+            AppBannerPromptOutcome userChoice;
+          };
+
+          dictionary UserResponseObject {


Why not PromptResponseObject?

Like it! Will rename.

mgiuca · 2016-11-08T08:03:08Z

index.html

+          <a>BeforeInstallPromptEvent.prompt</a>() method).
+        </p>
+        <p>
+          The <a>BeforeInstallPromptEvent</a> has three internal slots, which


Drop "three"; that sort of language can get out of date without anyone noticing.

yeah, it already bit me once.

mgiuca · 2016-11-08T08:06:02Z

index.html

+          };
+
+          dictionary BeforeInstallPromptEventInit : EventInit {
+            AppBannerPromptOutcome userChoice;


I'm still not a fan of allowing this as an argument (I don't see why this is ever useful and it's weird that it's an outcome and not a promise). I don't remember the outcome of the discussion last time I brought it up. I want to have a closer look tomorrow.

I feel pretty strongly about having the argument. I want a way of building these events from user-land in a way that allows .prompt() to resolve. It makes testing from user-land code much easier - and provides a complete solution, which also providing all the security assurances to the browser created events.

marcoscaceres · 2016-11-08T08:18:15Z

Looks pretty good. I have some quick comments, but I haven't had a deeper look at it and I want to do that tomorrow (on my workstation where I've got the repo checked out so I can build it and look at it in HTML). Can we wait until then to land this?

Absolutely. Sorry, I should have posted a link where you can view the rendered version:
https://rawgit.com/w3c/manifest/BIP/index.html

marcoscaceres · 2016-11-08T08:33:24Z

Rebased, so all the parts are now in place (be sure to "shift-refresh" if viewing the rawgit link).

mgiuca

Some more thoughts on the algorithm definition. Sorry it took the whole day to reply. I was ... distracted.

mgiuca · 2016-11-09T07:05:53Z

index.html

+            <dfn>[[\promptOutcome]]</dfn>
+          </dt>
+          <dd>
+            A <a>AppBannerPromptOutcome</a> enum value, initially set to


mgiuca · 2016-11-09T07:06:05Z

index.html

+            <dfn>[[\userResponsePromise]]</dfn>
+          </dt>
+          <dd>
+            A promise, which resolves with an <a>PromptResponseObject</a>,


mgiuca · 2016-11-09T07:13:11Z

index.html

+            </li>
+            <li>Let <a>[[\userResponsePromise]]</a> be a newly created promise.
+            </li>
+            <li>Let <a>[[\promptOutcome]]</a> be <code>null</code>.


I think promptOutcome is unnecessary.

If you search the remaining text for promptOutcome, note that it is always assigned to, never read from, except:

"If [[promptOutcome]] is not null" -- that's just using it as a Boolean not an outcome value.

"whose userChoice member is set to event's [[promptOutcome]]." -- promptOutcome was just set to a value immediately above, so it doesn't have to be a member of BIPE, it can just be a local variable to that algorithm.

Therefore, promptOutcome can be replaced with a Boolean. But I suspect we can simplify it further.

Brilliant! You are right... just updating the JS implementation. I'll try to post an update later tonight.

mgiuca · 2016-11-09T07:14:17Z

index.html

+            <li>Run the following steps <a>in parallel</a>:
+              <ol>
+                <li>If <a>[[\promptOutcome]]</a> is not <code>null</code>,
+                resolve <var>p</var> with <a>[[\userResponsePromise]]</a> and


Wait, "resolve p with userResponsePromise"? That means to resolve a promise with a promise. I think you mean "resolve p with promptOutcome"... but I'm trying to eliminate this (see above).

Why is p a new promise at all? Why can't we just have prompt() return userResponsePromise?

Then we can kill promptOutcome entirely.

Wait, "resolve p with userResponsePromise"? That means to resolve a promise with a promise. I think you mean "resolve p with promptOutcome"... but I'm trying to eliminate this (see above)

Resolving the promise with the promise unwraps the internal promise - so not to expose the internal promise... but I'm trying to simplify as you suggested. Also distracted :)

Oh so your intention was specifically to not expose the internal promise? (i.e, every time you call prompt() it's a different promise?)

Is there a good reason to do that?

Ok... still in shock... back to this...

Why is p a new promise at all? Why can't we just have prompt() return userResponsePromise?

It avoids exposing the internal promise. Then .prompt() can vend promises that all get resolved when the internal one does.

Oh so your intention was specifically to not expose the internal promise? (i.e, every time you call prompt() it's a different promise?)

Yeah, I realized we could just do that after the discussion we had in the bug (the discussion about e.prompt() === e.prompt()). I think returning a new promise works nicely because in reality the internal promise just becomes an implementation detail. But it will be nice in the spec because it makes the behavior quite nice and clear, IMHO.

marcoscaceres · 2016-11-09T13:32:09Z

@mgiuca, I updated .prompt() based on your feedback. I made the following changes:

Remove redundant [[\promptOutcome]]
Refactored .prompt() to no longer rely on [[\promptOutcome]]
Still resolving .prompt() with [[\userResponsePromise]].

I also updated the example implementation in #520 (comment)

@domenic, if you have a few mins, I would really appreciate if you could give us some feedback on: https://rawgit.com/w3c/manifest/BIP/index.html#beforeinstallpromptevent-interface

The prototype implementation on which the spec text is based: #520 (comment)

marcoscaceres · 2016-11-09T13:33:26Z

err, I mean @domenic... fixed above.

domenic · 2016-11-09T23:07:27Z

I'm a bit unclear why the promise fulfills with an object which is just a wrapper around userChoice, instead of with userChoice directly. Maybe it's future extensibility.

All the internal slots have an extra slash in them in the rendered output.

The constructor is a bit troublesome, since events all share the same constructor logic in most implementations, namely https://dom.spec.whatwg.org/#constructing-events. Maybe that logic needs to move to the prompt method? That seems like it will change things a decent bit.

"request to present an install prompt" isn't cross-linking correctly.

When referring to internal slots, it seems better to use obj.[[slotName]] notation. Currently obj is missing, which is a bit confusing since it differs: in prompt() it should be this, whereas in "request to present an install prompt" it should be event, I think.

mgiuca · 2016-11-09T23:29:12Z

index.html

+          </dt>
+          <dd>
+            A promise, which resolves with a <a>PromptResponseObject</a>, which
+            represent the outcome of <a>presenting an install prompt</a>.


mgiuca · 2016-11-09T23:31:20Z

index.html

+          </p>
+          <ol>
+            <li>
+              <a>Present an install prompt</a> and <var>outcome</var> be the


and let outcome be the result

mgiuca · 2016-11-09T23:35:17Z

index.html

+          <pre class="example" title="'beforeinstallprompt' in action">
+            window.addEventListener("beforeinstallprompt", async (event) =&gt; {
+              event.preventDefault();
+              // await e.g., user composing an email...


This comment is a bit confusing. I think I understand it (if the user is composing an email in the application you would wait until they finish the task before prompting). But I think that's just a bit confusing of a use case and the primary use case for this is not to avoid interrupting the user, but to present in-context UI for prompting.

So how about:

// Wait for e.g., the user to request installation from inside the app.

mgiuca · 2016-11-09T23:36:51Z

index.html

+              // await e.g., user composing an email...
+              await Promise.all(tasksThatPreventsInstallation);
+              const { userChoice } = await event.prompt();
+              console.info(`user selected: ${userChoice}`);


How about "user choice was: ${uc}" because the grammar would be weird of "user selected: accepted".

marcoscaceres · 2016-11-10T01:36:19Z

@domenic wrote,

I'm a bit unclear why the promise fulfills with an object which is just a wrapper around userChoice, instead of with userChoice directly. Maybe it's future extensibility.

Yeah, it's for extensibility: The (non-standard) Chrome implementation currently has additional information being returned (e.g., "platforms", which could be "Web, Play Store,"), but we are still discussing if that will be included. It's likely we will add more things.

All the internal slots have an extra slash in them in the rendered output.

Will fix.

The constructor is a bit troublesome, since events all share the same constructor logic in most implementations, namely https://dom.spec.whatwg.org/#constructing-events. Maybe that logic needs to move to the prompt method? That seems like it will change things a decent bit.

Ah, yeah. Although the .prompt() logic can probably stay the same, and I can just remove the constructor prose. The internal slots can be set as private instance properties without involvement of the constructor (i.e., they are an implementation detail in a sense).

"request to present an install prompt" isn't cross-linking correctly.

Fixing.

When referring to internal slots, it seems better to use obj.[[slotName]] notation. Currently obj is missing, which is a bit confusing since it differs: in prompt() it should be this, whereas in "request to present an install prompt" it should be event, I think.

Fixing.

marcoscaceres · 2016-11-10T04:25:38Z

@mgiuca, seems you were right about not having {userChoice} be passed during construction. It's incompatible with DOM's event creation. My apologies.
whatwg/dom#372 (comment)

On the upside, makes this much simpler.

marcoscaceres · 2016-11-10T05:07:26Z

Ok, I've integrated all feedback.

The final thing is if we stick with resolving with PromptResponseObject (safe option if we want to add additional things in the future, like .platforms) - or we feel confident we won't and we just resolve prompt() with AppBannerPromptOutcome (i.e., "dismissed", "accepted").

mgiuca · 2017-04-20T08:39:33Z

(That commit 0a05f3e was a rebase.)

mgiuca · 2017-04-20T08:43:02Z

I just did a pass over the beforeinstallprompt spec and pushed my changes (i.e., the delta between Marcos' last commit 0a05f3e and mine, 4bb2977).

Preview on rawgit.

Kenneth, do you want to have a look?

kenchris · 2017-04-20T09:39:45Z

index.html

              </li>
            </ol>
          </li>
        </ol>
+        <div class="issue">
+          Implementations may wish to show a prompt if and only if the site


comma around if and only if?

I think comma around "and only if" is correct.

(Why can't I do many comments together? Maybe this pull request was started so long ago that it's grandfathered out.)

kenchris · 2017-04-20T09:41:05Z

index.html

+          <a
+          data-cite="DOM#dom-event-preventdefault"><code>preventDefault</code></a>)
+          prevents the user agent from <a data-lt="presents an install
+          prompt">presenting an automated install prompt</a> until a later time


at a later time

Ah I think you misinterpreted this sentence. (Marcos wrote it and I just got confused myself now.) It's not trying to prevent prompting later. It's trying to prevent prompting now, but saying that the UA can prompt again later. I've rewritten it for clarity. WDYT?

kenchris · 2017-04-20T09:42:03Z

index.html

+            <dfn>[[\didPrompt]]</dfn>
+          </dt>
+          <dd>
+            A boolean, initially <code>false</code>. Represents if this event


represents whether? maybe if is fine... just sounds weird to me :)

kenchris · 2017-04-20T09:43:14Z

index.html

+              <a>Present an install prompt</a> and let <var>outcome</var> be
+              the result.
+            </li>
+            <li>Let <var>responseObj</var> be a newly created


do we need the Obj here?

Do you mean can we just combine these into a single step? Agree... done.

kenchris · 2017-04-20T09:43:55Z

index.html

+          </h4>
+          <p>
+            This example shows how one might prevent an automated install
+            prompt from showing until the user clicks a button to install the


do we need some examples to when it makes sense to delay?

Do you mean to explain why it makes sense to use this rather than just letting it happen?

I reworded that second sentence thusly:
"In this way, the site can leave installation at the user's discretion (rather than prompting at an arbitrary time), whilst still providing a prominent UI to do so."

kenchris · 2017-04-20T09:44:34Z

index.html

+          </p>
+          <pre class="example"
+               title="Using beforeinstallprompt to present an install button">
+            window.addEventListener("beforeinstallprompt", async (event) =&gt; {


do you need the () around event here? is that because of async... normally you dont with one argument

It isn't a syntax error (but I didn't confirm whether it works; I assume it does). Done.

kenchris · 2017-04-20T09:45:31Z

index.html

+              installButton.disabled = false;
+
+              // Wait for the user to click the button.
+              await installButtonClicked;


I think it is a bit magic where this variable comes from

Hmm yeah. async/await is nice but a bit magic here.

Refactored into a traditional addEventListener setup. The outer function no longer needs to be async.

mgiuca · 2017-04-21T06:54:46Z

I addressed those comments.

Meta-review questions:

I see a bunch of people using htmltidy on their specs. I tried to use this but it complained about the special respec elements and refused to do anything. Is there a trick for getting it to work on respec documents?
Do you prefer me to squash all the commits into one before landing this (or perhaps Marcos' one + my changes), or keep the long tail of git commits?

PTAL. Thanks!

kenchris · 2017-04-21T06:56:10Z

I think that squashing is nicer... But you could do one commit for Marcos work and one for your modifications

mgiuca · 2017-04-24T04:05:14Z

cannot use await now though (not async method...)

The inner function is async, and that's where I use await. I tested it, and it works. (The outer function does not need to be async since it just immediately does things including setting up a click handler.)

I think that squashing is nicer... But you could do one commit for Marcos work and one for your modifications

Squashed into two commits.

Also, how about my HTMLTidy question? From above: "I see a bunch of people using htmltidy on their specs. I tried to use this but it complained about the special respec elements and refused to do anything. Is there a trick for getting it to work on respec documents?"

Lastly, please wait for me to get some more people at Google to look at this before merging. Thanks!

marcoscaceres · 2017-04-26T06:54:57Z

@mgiuca

I see a bunch of people using htmltidy on their specs. I tried to use this but it complained about the special respec elements and refused to do anything. Is there a trick for getting it to work on respec documents?

No, there should be no trick. Make sure you are using HTML Tidy version 5.4.0 (I think that is latest).

Then just:

tidy -config tidyconfig.txt -o index.html index.html

- Fixed links. - Simplified logic and fixed bugs in the algorithm. - Reworked usage example. - Rewrote some text. - Added an issue box about install prompts. - Addressed other review comments.

mgiuca · 2017-04-26T07:13:26Z

Thanks. I must've been using a very old version of tidy. (Had to build from GitHub source.)

Now tidied (and squashed that into my commit).

mgiuca · 2017-04-26T07:19:35Z

FYI, I wrote up a doc with the things we're thinking about on Chrome before we close this out:
Chrome change proposal

This lists the things we'll have to change in Chrome to become compliant with this spec. I'm checking with some people internally whether we're OK to make these changes.

kenchris · 2017-04-26T07:23:09Z

I cannot access that doc

mgiuca · 2017-04-26T07:26:12Z

Sorry, fixed.

marcoscaceres · 2017-05-02T05:49:40Z

@mgiuca, did you get an ok to change Blink to match? Should we merge?

mgiuca · 2017-05-02T06:02:07Z

@marcoscaceres Almost... I am giving @slightlyoff one more day because I haven't heard back. Unless he objects, I think we will merge as-is. Thanks for your patience.

marcoscaceres · 2017-05-02T06:12:55Z

np. No hurry.

mgiuca · 2017-05-09T08:07:32Z

Hey,

Good you merged 👍 :)

For the record, I am still having a back-and-forth with Alex. He had some concerns (not about the spec per se but the compatibility issues if we change Chrome). But I am happy this is finally landed. We will continue to figure out the migration course for Chrome, and if we have any issues with the spec we can open new issues instead of continuing this megathread.

THANKS!

marcoscaceres force-pushed the BIP branch 9 times, most recently from 8d3a5da to a34f0fa Compare November 7, 2016 10:21

marcoscaceres force-pushed the BIP branch 3 times, most recently from dce5e44 to 7a9d04d Compare November 7, 2016 10:43

marcoscaceres mentioned this pull request Nov 8, 2016

feat(installation): define when/how to fire BIP #524

Merged

marcoscaceres commented Nov 8, 2016

View reviewed changes

mgiuca reviewed Nov 8, 2016

View reviewed changes

marcoscaceres force-pushed the BIP branch from ece20b2 to 663efe8 Compare November 8, 2016 08:23

mgiuca reviewed Nov 9, 2016

View reviewed changes

marcoscaceres force-pushed the BIP branch from 8f35a38 to 3ee567b Compare November 10, 2016 07:14

marcoscaceres unassigned kenchris Mar 20, 2017

mgiuca force-pushed the BIP branch from 3ee567b to 0a05f3e Compare April 20, 2017 08:38

kenchris reviewed Apr 20, 2017

View reviewed changes

mgiuca force-pushed the BIP branch from b459e26 to 95fadd6 Compare April 24, 2017 04:00

Feat(events): add BeforeInstallPromptEvent (closes #417)

6fa3d47

mgiuca force-pushed the BIP branch from 95fadd6 to 3c3bd98 Compare April 24, 2017 04:02

Rework beforeinstallprompt sections.

27dcb3c

- Fixed links. - Simplified logic and fixed bugs in the algorithm. - Reworked usage example. - Rewrote some text. - Added an issue box about install prompts. - Addressed other review comments.

mgiuca force-pushed the BIP branch from 3c3bd98 to 27dcb3c Compare April 26, 2017 07:12

marcoscaceres merged commit b0ad114 into gh-pages May 9, 2017

marcoscaceres deleted the BIP branch May 9, 2017 04:54

Feat(events): add BeforeInstallPromptEvent #520

Feat(events): add BeforeInstallPromptEvent #520

Conversation

marcoscaceres commented Nov 7, 2016

marcoscaceres commented Nov 7, 2016 • edited Loading

marcoscaceres commented Nov 7, 2016

marcoscaceres commented Nov 7, 2016

Choose a reason for hiding this comment

mgiuca left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcoscaceres commented Nov 8, 2016

marcoscaceres commented Nov 8, 2016

mgiuca left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcoscaceres commented Nov 9, 2016 • edited Loading

marcoscaceres commented Nov 9, 2016

domenic commented Nov 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcoscaceres commented Nov 10, 2016

marcoscaceres commented Nov 10, 2016

marcoscaceres commented Nov 10, 2016 • edited Loading

mgiuca commented Apr 20, 2017

mgiuca commented Apr 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgiuca commented Apr 21, 2017

kenchris commented Apr 21, 2017

mgiuca commented Apr 24, 2017 • edited Loading

marcoscaceres commented Apr 26, 2017

mgiuca commented Apr 26, 2017

mgiuca commented Apr 26, 2017

kenchris commented Apr 26, 2017

mgiuca commented Apr 26, 2017

marcoscaceres commented May 2, 2017

mgiuca commented May 2, 2017

marcoscaceres commented May 2, 2017

mgiuca commented May 9, 2017

marcoscaceres commented Nov 7, 2016 •

edited

Loading

marcoscaceres commented Nov 9, 2016 •

edited

Loading

marcoscaceres commented Nov 10, 2016 •

edited

Loading

mgiuca commented Apr 24, 2017 •

edited

Loading