search

w3c / manifest

Manifest for web apps
https://www.w3.org/TR/appmanifest/
Other
660 stars 162 forks source link

Add installation prompt control flow #417

Closed owencm closed 7 years ago

owencm commented 8 years ago

I've heard some feedback from developers that they are very excited by encouraging users to press the 'Add to Home Screen' button in Chrome, and the equivalents in Opera etc, but that it is a problem that they can't track that event for analytics purposes.

It would be great if we could somehow expose to developers that the user has installed their web app via some UA-provided button.

We could create a new event for this, or one alternate idea would be to fire a non-cancellable BeforeInstallPrompt event which immediately resolves the userChoice promise whenever the user presses a UA-provided button to install the web app.

marcoscaceres commented 8 years ago

FWIW, I think the aspect of onbeforeinstall that allows a site to hold the event until later to use the prompt method to be very unusual.

Agree. If with using the Permission API, you could hold the "prompt" state without needing to listen for BIP first. And BIP wouldn't need to constantly fired. It could be fired only once - and subsequent navigations could simply check the permission state.

I'm biased... but I'm starting to like the permission thing more and more :)

@mounirlamouri would really like your input here, as you know the Permissions API better than anyone.

benfredwells commented 8 years ago

What would 'granted' or 'denied' mean in response to navigator.permissions.query?

If 'granted' == 'already installed' and 'denied' means 'not installed' it feels like a weird conceptual fit to permissions.

marcoscaceres commented 8 years ago

If 'granted' == 'already installed'

What about something like:

mgiuca commented 8 years ago

Hi there. I just had a long chat with @dominickng and @benfredwells about this. We have some mixed thoughts.

Firstly, let's start with what I proposed above (a global method rather than a prompt method on the BeforeInstallPromptEvent object). That is:

OK, let's call that the base proposal.

Now you're suggesting we add (essentially) two new things:

  1. permissions.query({name: 'install'}), to query whether it has been installed, can't yet be installed, or can be installed at the user's discretion, and
  2. permissions.onchange, to be notified when that state changes.

Let's call that the permissions proposal. Notably, it doesn't (and can't) remove anything from the base proposal. In theory, we could drop the BIP event and just use the change event, but then that would mean you call preventDefault on the change event object, and that sounds totally wrong, so we must keep BIP.

What does permissions add?

So what do those two new capabilities buy us? At first glance (for Chrome's current policy), not much:

  1. query essentially tells us whether the install has succeeded ("granted"), BIP has been triggered ("prompt") or nothing has been allowed yet ("denied"). But we can already tell whether it's been installed, and whether BIP has been triggered (just store a boolean variable in response to the install and beforeinstallprompt events, respectively). So this adds nothing.
  2. change tells us when that state changes. But again, the BIP and install events already tell us when the user is being prompted and when the app is being installed.

So why do we want this?

The main reason we can think of is the separate BIP and change events allows UAs to have some interesting different policy options:

The user agents can decide on the above policies and change policies without having to update the spec. So that's kind of cool.

Some other pros:

The problems

The main problem is with what "granted" means. You said:

"An install attempt was made by showing the dialog. If it succeeded - you should have gotten oninstall event... if you missed it, your fault!" (unless we add oninstallerror, down the road). But it has nothing to do with installation success/fail.

The problem with this is that "granted" is normally an ongoing state. It means you are free to use, say, the microphone or geolocation, without restriction. In the install case, it should mean you are free to repeatedly install the app whenever you like (which we don't want to allow).

What it will actually mean in practice is "the app has been installed, and you cannot request it again". Since installation is an action (not a state), the "granted" state will be functionally equivalent to the "denied" state. What happens if you call navigator.installApp() from the "granted" state? Does it succeed silently (actually doing nothing)? Does it attempt to re-install the app? (On Android at least, that creates duplicate home screen icons, and Chrome is unable to detect whether one already exists.) Does it reject the promise? (Then in what way is it "granted"?) What if installation fails? Do we still go into a "granted" state which means "you failed to install and cannot try again, sorry"? There are lots of conceptual problems with "granted" meaning what you said it means.

Furthermore, what does permissions.request mean for the "install" permission? It's supposed to prompt the user then if they say yes, go into the "granted" state. What does this mean? It wouldn't actually install anything, you'd be in the granted state. Does that mean the app is free to install itself at some point in the future? (That isn't good.) Or is it just a no-op? This is where we think installation doesn't really match up with the permissions system: the permissions system is about granting perpetual access to a resource, not taking a single action once.

Basically, we could write into the spec answers to all of these questions that make it behave the way we want, but it would then feel like it doesn't fit into the permissions system properly.

Some more negatives:

In closing

OK, so I think we first need to decide whether the pros I listed above are actually desirable. If not, then I don't see a reason to attach this to the permissions system. If they are, then we have two choices: we can extend the beforeinstallprompt APIs to provide those benefits without using the permissions API, or we can go ahead and use the permissions APIs, after we come up with some good answers for what "granted" and request mean.

kenchris commented 8 years ago

Would it make sense to just not use the "granted" state? ie only "denied" and "prompt".

What happens when a user removed the app?

mgiuca commented 8 years ago

Would it make sense to just not use the "granted" state? ie only "denied" and "prompt".

We thought about not using the "granted" state at all (after installation you just go back to "denied"). This would mean that:

Basically, if we got rid of the "granted" state, then I think it would work (in fact, I think that's the only way this would work). But in doing so, I think it implies that the permissions model is the wrong model (if you have a permission that can never be granted, then why is it a permission?)

What happens when a user removed the app?

At the moment, Chrome can't detect this (in fact I think Android doesn't give us a way to know). So we can't react to it. But I think it works out mostly OK, because if the user removes an app, we shouldn't be re-prompting them. They installed your app and deleted it; you've had your chance. If they want it back, they can manually add to home screen from the menu.

kenchris commented 8 years ago

Yes, it feels to me that it is not the right fit. I believe that developers will think it is a permission to prompt or to install, but you are not going to allow further installs, or disallow manual installations when denied.

Potentially when the WebAPK code lands, can't you query on android whether it is installed or not?

dominickng commented 8 years ago

As far as I'm aware, we can query if a WebAPK is installed, but they won't be used for all types of installations, so we wouldn't be able to consistently check.

marcoscaceres commented 8 years ago

Thanks for the detailed write up and thoughts... some reactions below. I think we should have a call and walk through everything in detail. I'll try to gather more feedback from folks internally also. If need be, I'm happy to fly out to Sydney for a day so we can go through this in detail (I'm in Melbourne, so no big deal for me).

permissions.query({name: 'install'}), to query whether it has been installed, can't yet be installed, or can be installed at the user's discretion, and

The installation itself is orthogonal: The permissions.query({name: 'install'}) is an inquiry about the ability for the app to show the installation prompt. A successful install is still signaled by "oninstall" in my proposal. This is to cover the use case where installation is self-initiated by the end user (by digging through the menus or through an "ambient installation").

Notably, it doesn't (and can't) remove anything from the base proposal.

It's unclear why it can't remove anything from the base proposal? It makes the base proposal redundant, for reasons you also state (but with caveats I questioned below):

  1. BIP is equivalent to permission.onchange.
  2. nav.installApp() is permissions.request({name: install}) when prompt state is available.

In the case of BIP and having a cancellable onchange event, that strikes me as architectural purity: is there a technical reason to not allow the event to be cancellable? That it "sounds totally wrong" needs further consideration, IMHO: It might be that in whatever we come up with, the UA never shows the install prompt automatically (allows for "ambient install"), and only ever allows the the developer to permission.request({install}).

The above solves the problem in that BIP is not needed. onchange does not need to be cancellable, and gives control of the install prompt back to the developer - with the UA providing "ambient install" as a fallback.

So what do those two new capabilities buy us?

I think this makes an incorrect presupposition: it presumes that because Chrome already has BIP and prompt(), that other implementations have this also (which they don't). So, when you say "us", it means "Chrome" in this case. In the case of Gecko, for instance, we already have the Permissions API - so what it buys "us" (Gecko) is one less API to add.

I guess my point is: we can't take the current BIP (Chrome) model as a given - as we've seen, there is plenty of scope for improvements from what we've learned over the last years.

"silent transition"

I'm advocating for "silent transition". I think it strikes the right balance between UA, developer, and end-user control.

The user agents can decide on the above policies and change policies without having to update the spec. So that's kind of cool.

Agree. We still have a time to refine, experiment, etc. if we end up with a mixed policy.

We may not need an install event at all (the permissions.change event to "granted" may be sufficient, but see below).... There are lots of conceptual problems with "granted" meaning what you said it means.

Agree. "granted" remains highly ambiguous because there is a difference between "the installation process finished successfully (and a icon was placed somewhere successfully)", or "you were granted permission to install (which may or may not have succeeded)", or "the application is, and remains, 'installed' (for which the icon still available somewhere, maybe)".

To me, "granted" can only mean "you were granted permission to install"... but if that succeeded, or if the icon remains on the home screen, is not in scope and is a different concern. The UA allowing multiple icons on the homescreen from mismanagement would likely be a bug.

the permissions system is about granting perpetual access to a resource, not taking a single action once.

I concede you may be right here. But I need to give this further consideration. However, this predicated on how we frame "granted" - and if it's only conceptual, then it might be workable to just treat it as a no-op.

@dominickng says the permissions API itself is still in flux.

This is true in as far that all specs are in flux, only because we shape them with our requirements :)

We still have problems with request(). Since request is supposed to prompt the user and if they say "OK" go into the "granted" state, what do we do? I guess it would have to be a no-op.

Request is not required to do such a thing (or at least it should not be). If the request has been granted, then yes: it's a no-op. Once you are granted, you are granted. Only "prompt" allows a developer to put the a prompt in front of the end-user.

kenchris commented 8 years ago

Yes, let's do a call, these have been quite productive in the past. I can participate this week, but then I am off on vacation.

mgiuca commented 8 years ago

OK let's do a call this week. I will email Marcos and Ken to organize offline. If anybody else watching this thread wants to participate, please PM me.

mgiuca commented 8 years ago

Since we're hopefully meeting today, I won't reply to Marcos point-by-point. But I think we need to step back from the specific details of the API (using permissions or the details of onbeforeinstallprompt) and figure out what the behaviour should be.

Specifically, the main dispute here seems to be whether we should a) allow auto-prompt, giving the site an opportunity to suppress it and show it at a later time of its choosing (prompt-by-default), or b) disallow auto-prompt, but allow the UA to notify the site that it can show the prompt at a time of its choosing (prompt-on-demand).

It seems reasonable to allow UAs to choose between prompt-by-default or prompt-on-demand behaviour (or, of course, to never allow a prompt and only A2HS when the user explicitly asks for it), and we should work towards a spec that permits the UA to do both.

My primary concern is developers needing to write special logic to handle different browsers (or, by corollary, only writing logic for one browser and breaking on other browsers). So I'd like to find a solution that a) allows Chrome to prompt automatically but let developers cancel the prompt, without forcing other browsers to do the same, and b) doesn't require developers to behave differently on browsers that use prompt-by-default or prompt-by-demand. Hopefully those are agreeable goals and we can discuss ways to achieve that tonight.

marcoscaceres commented 8 years ago

Ok, so we are going to try to standardize on Google's implementation. It's been shipping for a while, it's supported in Opera, and Samsung's browser.

Going to send it in parts.

anssiko commented 8 years ago

Thanks @marcoscaceres, @kenchris should be happy to review the PR from our side.

marcoscaceres commented 8 years ago

Started WIP patch here: https://github.com/w3c/manifest/pull/506

It's way too drafty to comment on still... need to rework the whole install section, but let's get the API in place at least.

Trying to do the implementation at the same time in Gecko. Will also send that in parts. I have window.onbeforeinstallprompt implemented thus far... but not the actual BeforeInstallPrompt event itself.

marcoscaceres commented 8 years ago

Ok, so #506 revealed a couple of issues that we could address. This aim is to minimize impact on existing developer code... but the following would be a breaking change.

I'm wondering, can we make the design work more like this:

window.onbeforeinstallprompt = async function ( e )  {
  if (!userIsDoingThings) {
    return; // automatically show it.  
  }
  e.preventDefault(); 
  await Promise.all(thingsTheUserIsDoingThatBlockInstall);
  const promptResult = await ev.prompt();
  switch (promptResult.userChoice) {
    case "dismissed":
      // The user didn't want the app 😢
      analytics({ userHatesUs: true });
      break;
    case "accepted":
      // Awesome! it's installing 
      analytics({ userHatesUs: false });
      break;
    default:
      console.error(`Unknown choice? ${promptResult.userChoice}`);
  }
}

window.oninstall = e => {
  // The application installed!
}

Key things that could change:

  1. get rid of .userChoice attribute on the event.
  2. define a new interface PromptResult that contains the user choice and maybe other stuff.
mgiuca commented 8 years ago

There's a lot to unpack there. To specifically enumerate what you're proposing (from my reading of the example):

  1. prompt() resolves only when the user makes a choice (whereas currently it resolves immediately).
  2. prompt() returns the user's choice directly, so there is no need to request userChoice on the event (whereas currently it returns void).

I much prefer this. I only realised while reviewing your patch that propmpt resolves immediately and I thought that was weird.

Note that we still need userChoice on the event, because it is used to tell us about the user's choice when the banner is automatically shown. In your above example, you aren't recording analytics in the early return case. What you need there is:

window.onbeforeinstallprompt = async function ( e )  {
  var choice;
  if (!userIsDoingThings) {
    choice = await e.userChoice; // automatically show it.  
  } else {
    e.preventDefault(); 
    await Promise.all(thingsTheUserIsDoingThatBlockInstall);
    const promptResult = await ev.prompt();
    choice = promptResult.userChoice;
  }
  switch (choice) {
    case "dismissed":
      // The user didn't want the app 😢
      analytics({ userHatesUs: true });
      break;
    case "accepted":
      // Awesome! it's installing 
      analytics({ userHatesUs: false });
      break;
    default:
      console.error(`Unknown choice? ${choice}`);
  }
}

You could use appinstalled to figure out when the user accepts the automatic prompt, but there is no signal to tell you when the user has cancelled it, other than the rejection of BeforeInstallPromptEvent.userChoice.

Whether we want to make breaking changes is something we need data for and I don't personally feel comfortable making that decision (would want to speak with Alex again).

marcoscaceres commented 8 years ago

Note that we still need userChoice on the event, because it is used to tell us about the user's choice when the banner is automatically shown.

Good point. If prompt becomes a sync call, then it simplifies things to:

window.onbeforeinstallprompt = async function(e) {
  if (thingsTheUserIsDoingThatBlockInstall.length) {
    e.preventDefault();
    await Promise.all(thingsTheUserIsDoingThatBlockInstall);
    e.prompt();
  }
  const { userChoice: choice } = await e.userChoice;
  switch (choice) {
    case "dismissed":
      // The user didn't want the app 😢
      analytics({ userHatesUs: true });
      break;
    case "accepted":
      // Awesome! it's installing
      analytics({ userHatesUs: false });
      break;
    default:
      console.error(`Unknown choice? ${choice}`);
  }
};
marcoscaceres commented 8 years ago

You could use appinstalled to figure out when the user accepts the automatic prompt, but there is no signal to tell you when the user has cancelled it, other than the rejection of BeforeInstallPromptEvent.userChoice.

I don't think that dismiss would be a rejection, as it's not "exceptional" for the user to dismiss the install. It just resolves to "dismissed".

marcoscaceres commented 8 years ago

The thing that still bothers me about the overall design is that .onappinstall essentially equates to ev.userChoice = "accepted" - hence user choice is not really needed. I'm a little uncomfortable that we are revealing actual background processing details in the current design. For instance:

  1. how long the UA takes to present the install prompt: time between prompt() called and prompt() promise resolved - and system specific errors in the form of the rejection (which are probably none of the app's business that I can think of).
  2. how long it takes for a user to make a choice: time it takes for await e.userChoice resolving.
  3. how long it takes to write to disk. Time between ev.userChoice resolved and .onappinstalled called.

Making .prompt() synchronous and relying on .onappinstalled masks some of the above, because all the app can know is how long did it take from .prompt() to the app actually being installed - but none of the details of the process.

dominickng commented 8 years ago

The thing that still bothers me about the overall design is that .onappinstall essentially equates to ev.userChoice = "accepted" - hence user choice is not really needed. I'm a little uncomfortable that we are revealing actual background processing details in the current design.

.onappinstall isn't necessarily the same as ev.userChoice = "accepted". The user could accept the banner, but the app might fail to install for a bunch of reasons. WebAPKs are a good example of this, as is the bookmark app system which Chrome uses on desktop.

I hazily remember initially speccing prompt() as a synchronous method returning a bool, but it was suggested to make it a promise to allow UAs more flexibility in case they needed to do async work in the method. I don't mind overly here (changing Chrome's implementation to return a bool is probably fine since I don't think many people at all actually use the promise returned by prompt). But retaining the flexibility in the API does seem like a good thing (naively).

mgiuca commented 8 years ago

Is there any reason to have prompt return a value at all? Can it just be a void method (no Promise)?

If it isn't going to be a long-Promise (resolve when the user makes a choice), then there is really no reason for the app to do anything in response to its resolution. It doesn't have to be synchronous (i.e., the prompt can be shown after a message loop). But it could be a fire-and-forget, with no need for the user to wait on it resolving when the dialog is shown. Then if you want to wait for the user's choice you use userChoice.

I think it either makes sense for prompt to be a long promise or fire-and-forget. The middle-ground we currently have doesn't make sense.

marcoscaceres commented 8 years ago

.onappinstall isn't necessarily the same as ev.userChoice = "accepted". The user could accept the banner, but the app might fail to install for a bunch of reasons. WebAPKs are a good example of this, as is the bookmark app system which Chrome uses on desktop.

I completely agree, but the fundamental question is: does the app need to know about these (system) failures? If so, what is the use case for them knowing? There is not much the application can do to recover.

If notifying the application of installation does have a valid use case, then we need to also handle the case where BIP is not involved... so we would need a .onappinstallerror or some such event handler.

I hazily remember initially speccing prompt() as a synchronous method returning a bool, but it was suggested to make it a promise to allow UAs more flexibility in case they needed to do async work in the method.

The return value is orthogonal to any asynchronous work. In Gecko, the work (of requesting some king of install overlay) will definitely need to be done asynchronously.

But the question about if the application has any business/use-case about knowing these details is what I'm questioning.

I don't mind overly here (changing Chrome's implementation to return a bool is probably fine since I don't think many people at all actually use the promise returned by prompt). But retaining the flexibility in the API does seem like a good thing (naively).

It depends on what the return value reveals - and the use case for that information.

marcoscaceres commented 8 years ago

Is there any reason to have prompt return a value at all? Can it just be a void method (no Promise)?

I can't see why not.

If it isn't going to be a long-Promise (resolve when the user makes a choice), then there is really no reason for the app to do anything in response to its resolution. It doesn't have to be synchronous (i.e., the prompt can be shown after a message loop). But it could be a fire-and-forget, with no need for the user to wait on it resolving when the dialog is shown. Then if you want to wait for the user's choice you use userChoice.

Exactly. I would feel more comfortable with this.

I think it either makes sense for prompt to be a long promise or fire-and-forget. The middle-ground we currently have doesn't make sense.

Agree. Making it fire and forget would be better.

mgiuca commented 8 years ago

I got some data from Chrome (Android, stable version 53) about beforeinstallprompt which I can share here:

Unfortunately, we do not have data about access to userChoice. Nor do we have data about the number of beforeinstallprompt events that are actually handled by a listener.

All of this is below the unofficial Blink deprecation threshold of 0.03% so we could potentially make breaking changes to this API if the need arises.

marcoscaceres commented 8 years ago

Ok, let's work quickly to fix some of this before those numbers go up.

If possible, can we start by making "BIP" just a normal event and relocate .prompt() somewhere else?

marcoscaceres commented 8 years ago

By normal event, I mean a "simple event" that just uses the Event interface, but is cancellable.

marcoscaceres commented 8 years ago

*uses

marcoscaceres commented 8 years ago

So, there is a case where .prompt() can enter into an invalid state :(.

  1. BIP is fired.
  2. app calls .preventDefault();
  3. app begins awaiting install blocking tasks to finish.
  4. End-user chooses "add to homescreen" from browser's menu.
  5. app calls .prompt()
  6. IPC message sent to UI thread - browser discovers "install process in progress"
  7. ??? Invalid state ??? .userChoice can't ever resolve.

To fix this, .prompt() only resolves when given ownership of an install process - blocking manual installs for this app.

mgiuca commented 8 years ago

For the public record: between your second-most and most recent messages, we had an offline discussion where we concluded that we'd try to change prompt() to a fire-and-forget (non-promise-based) interface, but that we wouldn't move it out of the BeforeInstallPromptEvent.

Responding to your most recent message: I spoke to Dom about this. He says that, in Chrome at least, the user-triggered add to homescreen and the automated (or site-controlled) install banner are independent. Even when there is no race condition, they are still independent and using one does not preclude the other. Take the following non-racey case:

  1. BIP is fired.
  2. App calls .preventDefault().
  3. App sets up a then on the .userChoice promise.
  4. User chooses "add to homescreen" from browser's menu, and completes the install process.
  5. App calls .prompt.

This is like your use case but the installation fully completes before Step 5.

In this case, the prompt is shown (despite the app already being installed) and the user can choose OK, which installs the app a second time. userChoice resolves once the user chooses on the prompt.

So the answer is just that the two are independent. The app should be installed twice (the second install may be a no-op depending on the system; on Android it would actually create a second shortcut).

As for the race case you outlined, well the system just has to deal with this; for example queueing up the prompt behind the existing install process. I don't think this is an invalid state.

marcoscaceres commented 8 years ago

As for the race case you outlined, well the system just has to deal with this; for example queueing up the prompt behind the existing install process. I don't think this is an invalid state.

I hadn't thought about queue'ing it. Although that's not great UX (user might ask, "Didn't I just install this?"), that's definitely a sensible solution.

mgiuca commented 8 years ago

I think the chances of it happening are so remote that as long as we do something sensible, it doesn't necessarily have to be "great UX".

Furthermore, if we just queue it up, then it's indistinguishable (to the user) from the site simply deciding to call prompt() 100ms after they installed the app. Which is essentially just bad timing.

I think most sites using this will be calling prompt() in response to a user action anyway, not on a timer. The main use case for this is to create a button in your site that the user can click to activate installation. So then the race condition is not an issue.

marcoscaceres commented 8 years ago

@mgiuca, @dominickng, do you have an opinion on the following situation:

window.addeventlistener("beforeinstallprompt", (ev) => {
  // Calling without .preventDefault throws.
  // But is it still possible to call .prompt() after?
  try {
    ev.prompt();
  }catch(err){}
  ev.preventDefault();
  ev.prompt(); // Should this succeed?
  ev.prompt();  // this will now throw
});

That is, should we prevent calling .prompt() twice only after 1 successful call?

marcoscaceres commented 8 years ago

Updated example code above.

mgiuca commented 8 years ago

I think it should throw. Calling prompt should essentially un-set the preventDefault bit.

marcoscaceres commented 8 years ago

Ok, will make it so you can only call it once - no matter what. I won't change the default prevented tho, but I have an internal flag that gets set.

mgiuca commented 8 years ago

Ack.

marcoscaceres commented 8 years ago

Ok, this is what is going in to the spec (as prose and IDL, obvs.). I've got a battery of about 100 test to go with it.

All, please review the final design. 

"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.
    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))) {
      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.userChoicePromise = new Promise((resolve) => {
      internal.userChoiceHandlers = {
        resolve,
      };
      if (eventInit && "userChoice" in eventInit) {
        resolve(eventInit.userChoice);
      }
    });
    internalSlots.set(this, internal);
  }
  prompt() {
    if (internalSlots.get(this).didPrompt) {
      const msg = ".prompt() can only be called once.";
      throw new DOMException(msg, "InvalidStateError");
    } else {
      internalSlots.get(this).didPrompt = true;
    }

    if (this.isTrusted === false) {
      const msg = "Untrusted events can't call prompt().";
      throw new DOMException(msg, "NotAllowedError");
    }

    if (this.defaultPrevented === false) {
      const msg = ".prompt() needs to be called after .preventDefault()";
      throw new DOMException(msg, "InvalidStateError");
    }

    (async function task() {
      const userChoice = await showInstallPrompt();
      internalSlots.get(this).userChoiceHandlers.resolve(userChoice);
    }.bind(this)())
  }

  get userChoice() {
    return internalSlots.get(this).userChoicePromise;
  }
}

// Browser behavior for firing the event
async function notifyBeforeInstallPrompt(element) {
  await trackReadyState(); // don't fire until document fully loaded!
  if (installProcesses.length) { // If the user is already installing, stop
    return;
  }
  const event = new BeforeInstallPromptEvent("beforeinstallprompt");
  window.dispatchEvent(event);
  if (!event.defaultPrevented) {
    await showInstallPrompt(element);
  }
}
mgiuca commented 8 years ago

Wow, OK. I will have a proper look at that tomorrow. Thanks!

marcoscaceres commented 8 years ago

@mgiuca, I've added the above to https://github.com/w3c/manifest/pull/506 - so it's easier to review. Also updated the variable names to match your suggestions from the PR (i.e., "promptOutcome" instead, etc.).

If anyone wants to play with it (Chrome Canary only, as it uses async/await): https://rawgit.com/w3c/manifest/beforeinstallprompt/implementation/index.html

marcoscaceres commented 8 years ago

Ok, spec text now matches reference implementation. However, I'm still bothered that ev.userChoice can be left unresolve in case of an error.

We have two options:

  1. We add a new enum value to AppBannerPromptOutcome, like "unknown".
  2. Or, maybe more correct, we call reject() on the [[\userChoice]] promise.
dominickng commented 8 years ago

My initial thought is that rejecting the userChoice promise is the right way to go here. What particular error conditions were you thinking of for this? General issues installing the app once there is a definitive signal to proceed to installation?

marcoscaceres commented 8 years ago

@dominickng, I think the rejections would only happen as a result of a bad call to prompt(). So I'm thinking of rejecting with the errors generated there.

dominickng commented 8 years ago

Ah yes, that makes complete sense. I agree that if prompt() is rejected, then userChoice should reject as well.

marcoscaceres commented 8 years ago

@dominickng, prompt() now basically looks like this:

  prompt() {
    let error = null;

    if (internalSlots.get(this).didPrompt) {
      const msg = ".prompt() can only be called once.";
      error = new DOMException(msg, "InvalidStateError");
    } else if (this.isTrusted === false) {
      const msg = "Untrusted events can't call prompt().";
      error = new DOMException(msg, "NotAllowedError");
    } else if (this.defaultPrevented === false) {
      const msg = ".prompt() needs to be called after .preventDefault()";
      error = new DOMException(msg, "InvalidStateError");
    }

    if (!internalSlots.get(this).didPrompt) {
      internalSlots.get(this).didPrompt = true;
    }

    if (error) {
      internalSlots.get(this).userChoiceHandlers.reject(error);
      throw error;
    }
    // Show the prompt... 
 }
dominickng commented 8 years ago

Yes, that seems good to me. @mgiuca, did you have any thoughts?

marcoscaceres commented 8 years ago

Made a fix to the code above. I changed last else in the chain to the following, instead:

    if (!internalSlots.get(this).didPrompt) {
      internalSlots.get(this).didPrompt = true;
    }
mgiuca commented 8 years ago

I think I prefer the old behaviour -- userChoice would be left unresolved in the case of a prompt error.

Leaving promises unresolved isn't a bad thing: it just means that the conditions to resolve them weren't met. I don't see a strong relation between a prompt error and userChoice; in fact you can still successfully make a choice after a prompt error. For example:

window.addEventListener('beforeinstallprompt', () => {
  try {
    e.prompt();  // Error: preventDefault not called
  } catch (e) {}
  e.preventDefault();
  e.prompt();  // This is fine.
});

It would be bad to reject userChoice on the first prompt, because it's still meaningful later.

In fact, just this:

window.addEventListener('beforeinstallprompt', () => {
  e.prompt();  // Error: preventDefault not called
});

It's going to continue with the automatic prompt after that. userChoice should not be rejected by the call to prompt().

marcoscaceres commented 8 years ago

@mgiuca, d'oh, I think then we had a miscommunication back in https://github.com/w3c/manifest/issues/417#issuecomment-254726542

I thought we had agreed that .prompt() can only be called once. If prompt() can be called more than once, then I agree that the userChoice promise cannot be resolved.

So, should we agree that this is ok:

window.addEventListener('beforeinstallprompt', () => {
  try {
    e.prompt();  // Error: preventDefault not called
  } catch (e) {}
  e.preventDefault();
  e.prompt();  // This is fine.
});
mgiuca commented 8 years ago

I meant that prompt can only be called once successfully. Calling prompt with an error should not set the didPrompt flag, IMHO.

If you do want to make it so calling it in error prevents you from calling it again (I won't strongly object), then I still think my second example above holds: if you call prompt in error, but then don't call preventDefault, you will still get a prompt, so userChoice can still resolve. Ergo, calling prompt in error should not reject userChoice.