-
Notifications
You must be signed in to change notification settings - Fork 162
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
Make BeforeInstallPrompt optional #797
Conversation
<h2> | ||
Installation Events | ||
</h2> | ||
<p> | ||
Installation events and supporting the {{BeforeInstallPrompt}} is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why "installation events" (which includes appinstalled) is optional. I don't think there's anything particularly controversial about appinstalled is there? We should get that supported everywhere.
I'm happy to mark BIP as optional, since it's effectively optional already, as it's up to the user agent to decide when and if to fire it (so any user agent that doesn't implement it could claim conformance already). Marking it as optional just makes that clear (and also makes it legal for window.BeforeInstallPromptEvent to not exist).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any privacy risk with appinstalled
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We (me, @dominickng and @marcoscaceres ) couldn't think of any. You're going to get a CSS media query telling you that the window is standalone as soon as you open the window (which in Chrome Desktop is immediately upon install) anyway, so this doesn't give you any additional information.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m good then. Merge away!
@@ -562,10 +562,14 @@ <h3> | |||
</p> | |||
</section> | |||
</section> | |||
<section> | |||
<section class="atrisk"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Has there been any more discussion about the "at risk" status of this feature? I don't think we can just deprecate it at this point; too many sites rely on it (though as I say below, I think it's fine for it to be perpetually optional).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don‘t have a strong feeling on "at risk". Keeping it optional also leaves the door open for browsers to decide to never do anything with it or to remove support is they have it an are unhappy with it. If we get to the point that no UA is interested in supporting it anymore, we can label it "at risk" and begin to let folks know they may want to consider dropping it from their code.
@marcoscaceres @mgiuca Any reasons not to merge this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
LGTM |
Closes #???
This change (choose one):
changes normative sections without changing behavior)
Implementation commitment (delete if not making normative changes):
Commit message:
(Fill in. If making normative changes, describe exactly what the behavioral
difference will be.)
Preview | Diff