1264819 - Web Manifest: Implement BeforeInstallPromptEvent

Status: RESOLVED WONTFIX

(Core :: DOM: Core & HTML, defect, P3)

Description

[Marcos Caceres [:marcosc]]

The beforePromptEvent is fired before prompting to install a progressive web app. It allows the developer to "prevent the default action" (i.e., the prompt)... and to later request the prompt. 

Details:
https://github.com/w3c/manifest/issues/417

Spec text coming RSN.

Comment 1

[Marcos Caceres [:marcosc]]

bz, can I confirm that it's NOT possible to implement the following as a JS-implemented binding:  

```WebIDL 
[NoInterfaceObject]
interface AppInstallEventHandlersMixin {
  attribute EventHandler onbeforeinstallprompt;
  attribute EventHandler oninstall;
};
Window implements AppInstallEventHandlersMixin;

interface BeforeInstallPromptEvent : Event {
  Promise<InstallationPromptChoice> prompt();
};

enum InstallationPromptChoice {
  "accepted",
  "dismissed",
  "rejected",
};
```

Particularly:  

 * window.oninstall 
 * window.onbeforeintallprompt 

Need to be implemented in C++. However, I was unable to find any guides that describe how to wire that all up. 

Similarly BeforeInstallPrompt() can't be implemented in JS because the WebIDL generator doesn't know how to deal with methods (i.e., `prompt()`).

Comment 2

[Boris Zbarsky [:bzbarsky]]

Yes, you can't add new things to Window via pure JS, obviously... at some point some actual C++ does need to get created here.

See http://mxr.mozilla.org/mozilla-central/source/dom/events/EventNameList.h#8 for adding event handlers to stuff; these presumably should be WINDOW_ONLY_EVENT.  Adding the relevant entries to that header will produce the right methods on nsGlobalWindow.

> Similarly BeforeInstallPrompt() can't be implemented in JS because the WebIDL generator doesn't
> know how to deal with methods (i.e., `prompt()`).

You mean it can't have its implementation generated completely for you by adding it to GENERATED_EVENTS_WEBIDL_FILES?  That's because the generator has no way to tell what "prompt()" should do; that's not described in the IDL, sure.

The problem with actually implementing BeforeInstallPromptEvent in JS (not the same as having codegen automatically implement it for you in C++) is that Event doesn't have the right constructor signatures on it or something?  Or was there a more serious problem?  I recally this came up in another bug you were working on, but can't find it right now.

It might be simplest to just implement this event in C++, of course, but I'd like to double-check what the issue was with actually implementing events in JS (via JSImplementation, etc).

[As a side note: Having a prompt() method on an event like this is actually pretty weird API design; it's not clear to me why that's the API we're choosing to use here.]

Comment 3

[Marcos Caceres [:marcosc]]

(In reply to Boris Zbarsky [:bz] from comment #2)
> You mean it can't have its implementation generated completely for you by
> adding it to GENERATED_EVENTS_WEBIDL_FILES?  That's because the generator
> has no way to tell what "prompt()" should do; that's not described in the
> IDL, sure.

Yeah, was hoping "GENERATED_EVENTS_WEBIDL_FILES" would generate some stub where I could then write the implementation for prompt().  
 
> The problem with actually implementing BeforeInstallPromptEvent in JS (not
> the same as having codegen automatically implement it for you in C++) is
> that Event doesn't have the right constructor signatures on it or something?
> Or was there a more serious problem?  I recally this came up in another bug
> you were working on, but can't find it right now.

Yes, I believe it's something like that. Unfortunately, I just trashed my attempt at implementing in JS to try to do it in c++. However, if I get the oninstall/onbeforeinstallprompt to work, I'll try it again in JS and see what it was complaining about.   
 
> It might be simplest to just implement this event in C++, of course, but I'd
> like to double-check what the issue was with actually implementing events in
> JS (via JSImplementation, etc).
> 
> [As a side note: Having a prompt() method on an event like this is actually
> pretty weird API design; it's not clear to me why that's the API we're
> choosing to use here.]

Yeah, it's a bit strange - but kinda makes. It's a modified copy of what Chrome implements (this is to enable their progressive web apps things).

This is a usage example:

```JS
// criticalTasksToFinish in this example is an array of promises
// representing task that the user might be undertaking.
async function handleInstallPrompt(ev){
  if(!criticalTasksToFinish.length){
    // User is ok to be prompted. So don't prevent the default.
    return;
  }
  // The user is doing something critical! So don't bother them
  // with installation prompts: Let's prevent the default!
  ev.prevenDefault();
  // Let's instead wait till she, he, or xhe is done.
  await Promise.all(criticalTasksToFinish);
  // Ok, browser, show the install prompt!
  const promptResult = await ev.prompt();
  switch(promptResult){
    case "accepted":
      // Yay! the user accepted. We should wait for install event now...
      window.oninstall = makeUserFeelLoved;
      break;
    case "dismissed":
      // Hmmm... the user didn't make a choice... maybe it was not a good
      // time to ask? Should maybe look into this.
      break;
    case "rejected":
      // The user hates us. That's ok tho.
      break;
  }
}
window.addEventListener("beforeinstallprompt", handleInstallPrompt);
```

Comment 4

[Marcos Caceres [:marcosc]]

* but kinda makes sense, I mean.

Comment 5

[Marcos Caceres [:marcosc]]

Output from trying to implement as JS binding:

1:03.70 /Users/marcos/gecko/obj-x86_64-apple-darwin15.4.0/dist/include/mozilla/dom/BeforeInstallPromptEventBinding.h:139:21: error: declaration of 'WrapObject' overrides a 'final' function
 1:03.70   virtual JSObject* WrapObject(JSContext* aCx, JS::Handle<JSObject*> aGivenProto) override;
 1:03.70                     ^
 1:03.70 /Users/marcos/gecko/obj-x86_64-apple-darwin15.4.0/dist/include/mozilla/dom/Event.h:94:21: note: overridden virtual function is here
 1:03.70   virtual JSObject* WrapObject(JSContext* aCx, JS::Handle<JSObject*> aGivenProto) override final;
 1:03.70                     ^
 1:03.70 In file included from /Users/marcos/gecko/obj-x86_64-apple-darwin15.4.0/dom/bindings/RegisterBindings.cpp:32:
 1:03.70 Warning: -Woverloaded-virtual in /Users/marcos/gecko/obj-x86_64-apple-darwin15.4.0/dist/include/mozilla/dom/BeforeInstallPromptEventBinding.h: 'mozilla::dom::BeforeInstallPromptEvent::GetIsTrusted' hides overloaded virtual function
 1:03.70 /Users/marcos/gecko/obj-x86_64-apple-darwin15.4.0/dist/include/mozilla/dom/BeforeInstallPromptEventBinding.h:144:8: warning: 'mozilla::dom::BeforeInstallPromptEvent::GetIsTrusted' hides overloaded virtual function [-Woverloaded-virtual]
 1:03.70   bool GetIsTrusted(ErrorResult& aRv, JSCompartment* aCompartment = nullptr) const;
 1:03.70        ^
 1:03.70 /Users/marcos/gecko/obj-x86_64-apple-darwin15.4.0/dist/include/mozilla/dom/Event.h:114:3: note: hidden overloaded virtual function 'mozilla::dom::Event::GetIsTrusted' declared here: different number of parameters (1 vs 2)
 1:03.70   NS_DECL_NSIDOMEVENT
 1:03.70   ^
 1:03.70 /Users/marcos/gecko/obj-x86_64-apple-darwin15.4.0/dist/include/nsIDOMEvent.h:160:14: note: expanded from macro 'NS_DECL_NSIDOMEVENT'
 1:03.70   NS_IMETHOD GetIsTrusted(bool *aIsTrusted) override; \
 1:03.70              ^
 1:04.48 libgfx_layers.a.desc
 1:04.67 libembedding_components_find.a.desc
 1:05.51 libjs_ipc.a.desc
 1:06.15 1 warning and 1 error generated.
 1:06.17
 1:06.17 In the directory  /Users/marcos/gecko/obj-x86_64-apple-darwin15.4.0/dom/bindings
 1:06.17 The following command failed to execute properly:
 1:06.17 /usr/bin/clang++ -o RegisterBindings.o -c -fvisibility=hidden -fvisibility-inlines-hidden -DNDEBUG=1 -DTRIMMED=1 -DOS_POSIX=1 -DOS_MACOSX=1 -DHAVE_SIDEBAR -DSTATIC_EXPORTABLE_JS_API -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -I/Users/marcos/gecko/dom/bindings -I/Users/marcos/gecko/obj-x86_64-apple-darwin15.4.0/dom/bindings -I/Users/marcos/gecko/obj-x86_64-apple-darwin15.4.0/dist/include/mozilla/dom -I/Users/marcos/gecko/dom/base -I/Users/marcos/gecko/dom/battery -I/Users/marcos/gecko/dom/bluetooth/common/webapi -I/Users/marcos/gecko/dom/camera -I/Users/marcos/gecko/dom/canvas -I/Users/marcos/gecko/dom/geolocation -I/Users/marcos/gecko/dom/html -I/Users/marcos/gecko/dom/indexedDB -I/Users/marcos/gecko/dom/media/webaudio -I/Users/marcos/gecko/dom/media/webspeech/recognition -I/Users/marcos/gecko/dom/svg -I/Users/marcos/gecko/dom/workers -I/Users/marcos/gecko/dom/xbl -I/Users/marcos/gecko/dom/xml -I/Users/marcos/gecko/dom/xslt/base -I/Users/marcos/gecko/dom/xslt/xpath -I/Users/marcos/gecko/dom/xul -I/Users/marcos/gecko/js/xpconnect/src -I/Users/marcos/gecko/js/xpconnect/wrappers -I/Users/marcos/gecko/layout/style -I/Users/marcos/gecko/layout/xul/tree -I/Users/marcos/gecko/media/mtransport -I/Users/marcos/gecko/media/webrtc -I/Users/marcos/gecko/media/webrtc/signaling/src/common/time_profiling -I/Users/marcos/gecko/media/webrtc/signaling/src/peerconnection -I/Users/marcos/gecko/obj-x86_64-apple-darwin15.4.0/ipc/ipdl/_ipdlheaders -I/Users/marcos/gecko/ipc/chromium/src -I/Users/marcos/gecko/ipc/glue -I/Users/marcos/gecko/obj-x86_64-apple-darwin15.4.0/dist/include -I/Users/marcos/gecko/obj-x86_64-apple-darwin15.4.0/dist/include/nspr -I/Users/marcos/gecko/obj-x86_64-apple-darwin15.4.0/dist/include/nss -fPIC -DMOZILLA_CLIENT -include /Users/marcos/gecko/obj-x86_64-apple-darwin15.4.0/mozilla-config.h -MD -MP -MF .deps/RegisterBindings.o.pp -Qunused-arguments -Qunused-arguments -Wall -Wc++11-compat -Wempty-body -Wignored-qualifiers -Woverloaded-virtual -Wpointer-arith -Wsign-compare -Wtype-limits -Wunreachable-code -Wwrite-strings -Wc++11-compat-pedantic -Wc++14-compat -Wc++14-compat-pedantic -Wclass-varargs -Wimplicit-fallthrough -Wloop-analysis -Wthread-safety -Wno-invalid-offsetof -Wno-inline-new-delete -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-unknown-warning-option -Wno-return-type-c-linkage -fno-exceptions -fno-strict-aliasing -fno-rtti -fno-exceptions -fno-math-errno -std=gnu++11 -pthread -pipe -g -O3 -fomit-frame-pointer /Users/marcos/gecko/obj-x86_64-apple-darwin15.4.0/dom/bindings/RegisterBindings.cpp
 1:06.18 make[5]: *** [RegisterBindings.o] Error 1
 1:06.18 make[5]: *** Waiting for unfinished jobs....
 1:07.05 libdom_xml.a.desc
 1:07.34 make[4]: *** [dom/bindings/target] Error 2
 1:07.34 make[4]: *** Waiting for unfinished jobs....
 1:07.40 libdom_fetch.a.desc
 1:08.90 libjs_xpconnect_loader.a.desc
 1:08.92 libdom_power.a.desc
 1:09.41 libdom_media_platforms_agnostic_eme.a.desc
 1:11.50 libimage_decoders.a.desc
 1:11.54 make[3]: *** [compile] Error 2
 1:11.54 make[2]: *** [default] Error 2
 1:11.54 make[1]: *** [realbuild] Error 2
 1:11.54 make: *** [build] Error 2
 1:11.56 439 compiler warnings present.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Promise<InstallationPromptChoice> prompt(); in an Event is so uncommon (I think the super weird ServiceWorker events have been so far the only ones doing something like that) that codegen does not and will not support it.

And events aren't something one can implement in JS, and I'd rather keep it that way, given that
any normal event interface (not inheriting UIEvent) should be implementable using codegen.

So, sorry, C++ it is.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> Yeah, was hoping "GENERATED_EVENTS_WEBIDL_FILES" would generate some stub where I could then write the
> implementation for prompt().  

As in, write a JS implementation of a single method for an otherwise C++-implemented binding?  We don't have that hooked up, and doing that would require some interesting infrastructure (e.g. how do you find the right JS Function object?).

If you want to implement in C++, you can use GENERATED_EVENTS_WEBIDL_FILES with prompt() commented out in the IDL, then just copy the generated implementation over into the source tree, move the idl over to the list of normal IDLs, and then uncomment prompt() in the IDL and implement it.  That will save you the trouble of implementing all the event boilerplate, at least....

>  const promptResult = await ev.prompt();

What state from "ev" gets used here?  As in, why does it need to be a method on the event itself?

> Output from trying to implement as JS binding:

OK, so that looks like two issues: the unforgeable isTrusted thing, which we could deal with, and the fact that event WrapObject is a bit special, which we _might_ be able to special-case in codegen, but would be adding a bunch of complexity.

If implementing this in C++ is at all an option, I strongly recommend that...

Comment 8

[Marcos Caceres [:marcosc]]

(In reply to Boris Zbarsky [:bz] from comment #7)
> If you want to implement in C++, you can use GENERATED_EVENTS_WEBIDL_FILES
> with prompt() commented out in the IDL, then just copy the generated
> implementation over into the source tree, move the idl over to the list of
> normal IDLs, and then uncomment prompt() in the IDL and implement it.  That
> will save you the trouble of implementing all the event boilerplate, at
> least....

Great idea. Will do that.

> >  const promptResult = await ev.prompt();
> 
> What state from "ev" gets used here?  As in, why does it need to be a method
> on the event itself?

The resulting "user choice" ("accepted", "dismissed", "rejected"). The user's choice is directly related to the installation prompt. There is no other way, right now, to get this information. 

There is a bit of short explanation of the logic behind this written by Alex Russell here:
https://github.com/slightlyoff/AppInstallImprovements/blob/master/explainer.md#controlling-installation

Note that I'm open to changing his proposal - as it's not quite right anyway, and it's not what I'm currently specifying or planning to implement. We (those working on the spec) are open to suggestion on how to make the API better.

In particular, and if you think it's fine, I can make:

prompt() be sync, return void.

And just do what Alex proposes IDL-wise:

[SameObject] readonly attribute Promise<UserChoice> userChoice;

So, a developer would do something like (I've ignored error handling for clarity):

window.addEventListener("beforeinstallprompt, async eventHandler(ev){
  ev.preventDefault()
  await Promise.all([...tasksThatBlockInstalling]);
  ev.prompt(); // Request the install prompt again
  const choice = await ev.userChoice;
  doSomethingInterestingWith(choice); 
});

WDYT? 

> If implementing this in C++ is at all an option, I strongly recommend that...

yeah, I'm totally fine with that. You know me, I always try JS first (my c++ is lacking) but no problems doing it in C++; just means I'll need a bit more guidance and just feel bad about bugging people for help.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Hmm.  So in terms of the API design, I guess my question is what about "ev.prompt()" involves state on the event?  Is it the fact that ev.userChoice hangs off the event?  But in your prompt-returns-Promise alternative that's not a problem.

Put another way, why is prompt() an instance method on the event as opposed to just being some static method?  Is it because the UA knows what prompt it was trying to show and that information lives in the event?

Comment 10

[Marcos Caceres [:marcosc]]

(In reply to Boris Zbarsky [:bz] from comment #9)
> Hmm.  So in terms of the API design, I guess my question is what about
> "ev.prompt()" involves state on the event?  Is it the fact that
> ev.userChoice hangs off the event?  But in your prompt-returns-Promise
> alternative that's not a problem.
> 
> Put another way, why is prompt() an instance method on the event as opposed
> to just being some static method?  Is it because the UA knows what prompt it
> was trying to show and that information lives in the event?

In the proposed design, prompt() throws if preventDefault() was not first called on the event: so prompt() is dependent on the "default prevented flag" of the event.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Ah.  So the idea is that you can only prompt if no one else has prevented you from doing so?  And hence the behavior is event listener ordering dependent?  :(

Comment 12

[Marcos Caceres [:marcosc]]

(In reply to Boris Zbarsky [:bz] from comment #11)
> Ah.  So the idea is that you can only prompt if no one else has prevented
> you from doing so?  And hence the behavior is event listener ordering
> dependent?  :(

Ok, yeah... that would totally suck. Will talk to the Chrome folks and see if we can come up with something better.

Comment 13

[Marcos Caceres [:marcosc]]

So as Blink's design is shipping in Android Chrome, Opera, and Samsung's browser, and they've been shipping it for like a year, I'm just going to standardize their design (even tho it's a bit odd).

Comment 14

[Marcos Caceres [:marcosc]]

not working on this, so...

Comment 15

[Marcos Caceres [:marcosc]]

We don't plan to support this part of the spec, so closing. It's going to be optional in the spec anyway and may eventually be removed.