-
Notifications
You must be signed in to change notification settings - Fork 161
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
First pass as adding a Client Hint for Display Mode #977
base: main
Are you sure you want to change the base?
Conversation
Had a few moments to put this together, based on what I’ve been working on in the App Info doc. |
Note: Validation is failing due to a current issue with the spec, where |
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.
Thanks for crafting this @aarongustafson! Looking pretty good.
I'm wondering if we need to say exactly when this hint is sent (I imagine that must be part of client-hints-infrastructure spec)? Like, how does the user agent know to send it on page load or whatever...
Once we clean this up a little bit, might be great to have @yoavweiss take a look at it and make sure we have all the right machinery in place.
The header's ABNF is: | ||
</p> | ||
<pre> | ||
Sec-CH-Display-Mode = sf-string |
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.
This should probably be
Sec-CH-Display-Mode = sf-string | |
Sec-CH-Display-Mode = "fullscreen" / "standalone" / "minimal-ui" / "browser" |
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 apprehensive about locking this down to specific values, especially as the display mode may be one of the overrides not defined in this list.
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.
ah, well we should just expand the list. We can update the spec at any time, so there is no reason not to keep this list in sync.
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 think we would also want to have this possibly be values that end up in manifest-incubations spec - not sure the best way to do this spec-wise, but referencing display-mode is probably a good way.
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 wanted to keep this extensible, hence: "whose value is a suitable [=display modes value=]"
No need. This is specified in client hints. If it needs to be sent upon the first load, it's a critical client hint. Any client hint can be promoted to be a critical client by mentioning it in the
(I'm working with Yoav on User Preference Media Features Client Hints Headers.) |
Ok, perfect! glad to have your support/review @tomayac on the CH front. |
IIUC, we have a concerns from Mozilla (via @annevk) about Client Hints (w3c/manifest-app-info#32 (comment)):
I'd be interested to hear from Apple, maybe @hober or @rniwa if they have an objection or concerns? |
Co-authored-by: Marcos Cáceres <marcos@marcosc.com>
Co-authored-by: Marcos Cáceres <marcos@marcosc.com>
Co-authored-by: Marcos Cáceres <marcos@marcosc.com>
Co-authored-by: Marcos Cáceres <marcos@marcosc.com>
Co-authored-by: Marcos Cáceres <marcos@marcosc.com>
Co-authored-by: Marcos Cáceres <marcos@marcosc.com>
Yes, the concerns are around potential bloat in the request header. That’s not something we can really address in this context (and is being addressed in the Client Hints discussions). As there is no other way to inform the server about the display mode without involving headers, I don’t know that there is another way to solve for this use case. |
Note to self: Will need to update this PR to reference the |
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.
This change looks good, but as @marcoscaceres referenced - we don't have signals from other vendors. If we need a place for this spec to live in the meantime we can put it in manifest-incubations
The header's ABNF is: | ||
</p> | ||
<pre> | ||
Sec-CH-Display-Mode = sf-string |
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 think we would also want to have this possibly be values that end up in manifest-incubations spec - not sure the best way to do this spec-wise, but referencing display-mode is probably a good way.
https://github.com/WICG/manifest-incubations/blob/gh-pages/display_mode-client-hint.md |
Closes #954
This change (choose at least one, delete ones that don't apply):
Implementation commitment (delete if not making normative changes):
Commit message:
Adds a Client Hint to reveal the Display Mode being used by the user agent.
Person merging, please make sure that commits are squashed with one of the following as a commit message prefix:
Preview | Diff
Preview | Diff