Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Beforeinstallprompt #9

Closed
wants to merge 10 commits into from
Closed

Conversation

dmurph
Copy link
Collaborator

@dmurph dmurph commented Mar 17, 2021

@mgiuca can you PTAL at this (I have to clean up the forked commit chain here, it has gotten really messy), but I'm interested in any ideas you have about putting the spec text into here.

I wonder if we need all of this, or if we can trim it down? But maybe it's nice to have everything.

I think the only stuff left in manifest spec is installability & installability signals, so I did one change on the Installation process to link to that, and removed the appinstalled event.

This doesn't link properly at all - but interested in your thoughts about how this might look (before I start manually data-citing everything).


Preview | Diff

@dmurph dmurph requested a review from mgiuca March 17, 2021 21:20
@mgiuca
Copy link
Member

mgiuca commented Apr 1, 2021

Hi Daniel!

Sorry it's taken so long for me to look at this.

I think this basically looks fine as is (modulo linking things correctly back to the Manifest spec). (But I am biased as I wrote most of it.)

removed the appinstalled event

Is there a reason you removed it? From memory, we had a line in "steps to install the web application" that said something to the effect of "fire an Event named "appinstalled" at window" (that's all we need to say; no need to define an AppInstalledEvent type since it has no custom members or methods). Can you put that line back? (Or am I misremembering?)

You still mention the window.onappinstalled event handler, and have an example of handling the event both ways, which currently doesn't make sense since there's no code that fires an event called "appinstalled".

I'd like it back, since we support both events in Chrome (& presumably Edge).

I wonder if we need all of this, or if we can trim it down? But maybe it's nice to have everything.

It could possibly be trimmed down. I was thinking maybe we didn't need the distinction between the three types of install prompt (manual, automated and site-triggered), but looking at it, I think those are all treated semantically differently. (For example, manual doesn't trigger BIP at all, automated requires BIP to fire right before it, and site-triggered requires BIP to fire, then the site can opt into triggering it.) So I think we need all of this.

Another option is to delete automated prompts entirely -- since I don't think any current implementations do it. (i.e. remove this from being a normative possibility.) That's in there because it was originally the only behaviour that Chrome did. But we took that out, and the current spec text (before it was removed) gives UAs the option of either automated or site-triggered. But Chrome for Desktop and Android (or any other browser that implements this) I believe, do not use automated prompts. We could just remove that whole possibility, and add it back if an implementor needs it.

Copy link
Member

@mgiuca mgiuca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments above.

lgtm with appinstalled event firing back. Or, if you want to remove automated install prompts, feel free to ping for another review.

Comment on lines +152 to +153
the user when, for instance, there are sufficient <a data-cite=
"appmanifest#installability-signals">installability signals</a> to
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the "installability signals" from the other spec, as they turned out to be somewhat Chrome specific. So it might be good to just say:

Suggested change
the user when, for instance, there are sufficient <a data-cite=
"appmanifest#installability-signals">installability signals</a> to
the user when, for instance, there are sufficient signals to

install prompt</dfn>: that is, a UI that the user agent presents to
the user when, for instance, there are sufficient <a data-cite=
"appmanifest#installability-signals">installability signals</a> to
warrant <a data-cite="appmanifest#dfn-installed">installation</a> of
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

General note: you should never need to use data-cite= for any W3C spec. If a term is not available via xref, it needs to be data-export=""ed from the other spec.

Comment on lines +252 to +253
<section data-dfn-for="BeforeInstallPromptEvent" data-link-for=
"BeforeInstallPromptEvent">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<section data-dfn-for="BeforeInstallPromptEvent" data-link-for=
"BeforeInstallPromptEvent">
<section data-dfn-for="BeforeInstallPromptEvent">

@dmurph dmurph closed this May 17, 2021
@dmurph dmurph deleted the beforeinstallprompt branch May 17, 2021 22:24
@dmurph
Copy link
Collaborator Author

dmurph commented May 17, 2021

Apologies, I didn't mean to close this but I can't seem to reverse it. Pull request is here:
#12

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants