-
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
Adding field display_override
to the manifest
#932
Conversation
@mgiuca can you PTAL? Note - this is definitely a 'draft' in the sense that I've never done a spec pull request here - sorry if there are a bunch of mistakes here! I would love guidance |
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.
Hi Daniel,
Great! Thanks for submitting this PR and getting your feet wet!
I've put a number of comments. My review was somewhat hasty so I didn't give you the precise syntax or wording in a lot of my suggestions, but I think you can figure it out and ping me if you need exact suggested wording or syntax advice.
Apologies: As I said in one of the comments, it's a bit hard to cargo-cult the syntax from elsewhere in this file, since Respec changed the syntax since this was written.
Also I think you already found this, but the language that these algorithms is written in is HTML Infra; it's a semi-formal language and we should try to follow those conventions rather than just writing English pseudo-code.
Cheers!
Matt
index.html
Outdated
"sizes": "128x128" | ||
}], | ||
"start_url": "/index.html", | ||
"display_override": ["minimal-ui", "standalone"], |
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 quite clear on what this site's intentions are (maybe make that more clear in the description above).
They want minimal-ui, but if that's not supported fall back to standalone, but if display-override isn't supported, fall back to browser? I don't really see why you'd ever want this.
Wouldn't they either want display-override: minimal, standalone, and display: minimal, or display-override: minimal, display: standalone?
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, yes you're right
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.
Please resolve :-)
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 pinging, fixed.
Thanks for the reviews! I'm not exactly sure how to use github pull request tool to respond to comments, hopefully I did that in an acceptable way Can you please take a look again? |
Hi, Yeah GitHub review tool is a big mess; I find it very hard to reply to everything and very easy to lose comments. I'll do a full review tomorrow; for now just responded to your direct question. |
Gentle ping on this request - @mgiuca do you mind giving this another review? |
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 Daniel. Looking great now.
A few minor comments, and then a suggestion to rewrite the algorithm which you can either accept or reject. Not sure which is cleaner, so have a think about it.
And a good weekend!
index.html
Outdated
[=iteration/while=] |considering_display_mode| is not {{browser}} | ||
<ol> | ||
<li> | ||
If the user agent supports the |candidate_display_mode|, then |
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 thinking about this while loop. I think it's correct but it's still a bit messy to do a loop with a switch in it when essentially you're just doing a linear scan through a list.
How's this (thinking out loud ... this might be messier, feel free to ignore this and keep the while loop).
First, some definitions:
- The fallback chain for browser is [browser].
- The fallback chain for minimal-ui is [minimal-ui] + the fallback chain for browser.
- The fallback chain for standalone is [standalone] + the fallback chain for minimal-ui.
- The fallback chain for fullscreen is [fullscreen] + the fallback chain for standalone.
(Or you could explicitly spell out the full fallback chain for each mode.)
Then, in the algorithm:
- First thing you do is go "the display fallback chain is
display_override
+ the fallback chain fordisplay
". - For each candidate of display fallback chain, if the user agent supports candidate, return it.
(Effectively, you combine the entire fallback chain into a single list combining both fields, just like you do in the explainer, and scan over it in one general iteration.)
WDYT?
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.
Let me know what you think of the changes :)
Thanks for the review! Please take a look again at the changes :) |
index.html
Outdated
</ol> | ||
</li> | ||
<li> | ||
Define the |fallback_chain| for {{browser}} to be [{{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.
Don't make fallback_chain a variable, since it doesn't act like a variable, it's more like a concept (strictly speaking, you could think of it as a function, fallback_chain(display_mode) = ...
, but I think it's not worth being so formal about it).
So I think move these definitions outside of this algorithm, and express them just in plain English: "The fallback chain for browser
is «browser
»"., etc. (Note: Use «» to denote a literal list, not, [], per Infra.)
Then down below, replace "the fallback_chain of" with "the fallback chain of" and you're good.
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 put this above in the text - hopefully it reads ok?
@marcoscaceres I think this is close to being ready from a technical standpoint. Can we get a commitment to implement it in Firefox? @hober I'm not sure if you or anyone from Safari has reviewed this proposal yet. (See the WICG proposal; TAG review). This is just a change to the manifest parser, and doesn't directly involve implementation of new features (though it enables a path to adding new display modes). Do you have any objections, and would Safari be able to commit to implementing this parser change? (Daniel, could you link to the TAG review from the PR description?) |
Updated patch with Matt's comments. It looks like there is an error with something unrelated to my change, but hopefully it is still reviewable? @hober, @marcoscaceres, can you PTAL? This change is required for future display modes, like tabbed & custom window overlay. |
7167668
to
99288ca
Compare
as you can see, I'm struggling with rebasing my fork with changes from here. Any advice appreciated lol. It seems to really want to apply those chore changes in my patch 🤷 |
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.
Great work, Daniel! Sorry this has been so long in the making.
I've approved but added some minor comments. One is a type problem we're going to have in the future, but isn't currently a problem, so I've added a long comment but the effect of it is just to add a note about it, for now.
index.html
Outdated
</li> | ||
<li> | ||
[=list/For each=] |fallback_mode:DisplayModeType| of the | ||
fallback_chain of |manifest|.{{WebAppManifest/display}}: |
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.
Remove the underscore in "fallback_chain
" since this is a prose term defined above without an underscore.
And link to the above definition (see above comment to use <dfn>
). I think you can use {{fallback chain}}
for this. Or maybe [=fallback chain=]
. I usually guess until it works :)
index.html
Outdated
@@ -1044,6 +1110,7 @@ <h2> | |||
DOMString iarc_rating_id; | |||
USVString start_url; | |||
DisplayModeType display = "browser"; | |||
sequence<DisplayModeType> display_override; |
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 just thought of a problem (which is not a problem right now, but will be as soon as we add a new display mode): we need a way to say for new display modes that they are allowed in display_override
but not display
.
We either need to do this in the type system (so it is type-impossible to use, say, "display": "tabbed"
, but you can use "display_override": ["tabbed"]
), or have steps in the algorithm to fail if you use a "new" display mode in display
. My preference is the former.
I think what we should do is have two enums, for now let's just call them "DisplayModeType" and "EnhancedDisplayModeType". The IDL should have:
DisplayModeType display = "browser";
sequence<DisplayModeType or EnhancedDisplayModeType> display_override;
And never put a new value in DisplayModeType
.
That allows you to use the new ones in display_override
but not display
, and we don't need to mention this in the algorithms at all. Does that sound good? We could also rename the old one to LegacyDisplayModeType
or something, I don't really mind.
If we do that now, EnhancedDisplayModeType
will be empty, so maybe we just add a note to do that in the future? Let's add a note just under the listing of DisplayModeType
's possible values, that no new values will be added to this, and future display modes will be added to a new enum that can only be used with display_override
but not display
, for forwards-compatibility reasons.
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.
Sound good, I'll add a note about this under the DisplayModeType
's possible values, saying:
No new values will be added to this enum, and instead future display modes will be added to a new enum that can only be used with
display_override
but notdisplay
, for forwards-compatibility reasons.display_override
will then be defined assequence<DisplayModeType or EnhancedDisplayModeType>
, assuming the new enum is namedEnhancedDisplayModeType
.
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.
Sounds good. Maybe reword it so it reads more easily: "will be added to a new enum, tentatively named EnhancedDisplayModeType
, ..." and then you can remove the "assuming" qualifier to the last sentence.
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.
Based on internal implementation experience, it will be much easier to present this as the other option, where the algorithm fails for any 'new' display mode in display
. What do you think about that?
I made a note under the algorithm:
In the future, new values added to {{DisplayModeType}} will not be
added to the fallback chain, and the algorithm should treat the a
{{WebAppManifest/display}} field set to a new value as {{browser}}.
And a note in DisplayModeType:
Feedback provided in a different CL (2375597: dpwas: Add window-controls-overlay as a new display mode | https://chromium-review.googlesource.com/c/chromium/src/+/2375597) and in w3c/manifest#932 is that we need a way to say new display modes are allowed in display_override but not display. The plan is to introduce a new enum EnhancedDisplayMode, and display_override should be: sequence<DisplayModeType or EnhancedDisplayModeType>. To support that, this CL deprecates the existing display_override field, and renames it. Renaming here should be fine, as long as we don't use JSON serialization. Once the new field has landed, we will remove this field, and mark it reserved so that future users don't reuse the field. Explainer: https://github.com/WICG/display-override/blob/master/explainer.md Design document: https://docs.google.com/document/d/1hEmbGVHMN38q1YTaaGccQ-Y5CHr7xIURYPRWXTuvZLo/edit?usp=sharing I2P: https://groups.google.com/a/chromium.org/d/topic/blink-dev/WvIeZT8uSzw/discussion Manifest w3c PR: w3c/manifest#932 Bug: 1092667 Change-Id: Ifbc0160a88263d4994d138f99921371ceea2bcb8 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2436417 Commit-Queue: Mike Jackson <mjackson@microsoft.com> Reviewed-by: Alexey Baskakov <loyso@chromium.org> Reviewed-by: Daniel Murphy <dmurph@chromium.org> Cr-Commit-Position: refs/heads/master@{#811932}
This reverts commit 3ce0075. Reason for revert: Keeping single enum Original change's description: > dpwas: deprecate existing display_overrides field in proto > > Feedback provided in a different CL (2375597: dpwas: Add > window-controls-overlay as a new display mode | > https://chromium-review.googlesource.com/c/chromium/src/+/2375597) > and in w3c/manifest#932 is that we need a > way to say new display modes are allowed in display_override but > not display. > > The plan is to introduce a new enum EnhancedDisplayMode, and > display_override should be: > sequence<DisplayModeType or EnhancedDisplayModeType>. > > To support that, this CL deprecates the existing display_override field, > and renames it. Renaming here should be fine, as long as we don't use > JSON serialization. Once the new field has landed, we will remove > this field, and mark it reserved so that future users don't reuse > the field. > > Explainer: https://github.com/WICG/display-override/blob/master/explainer.md > Design document: https://docs.google.com/document/d/1hEmbGVHMN38q1YTaaGccQ-Y5CHr7xIURYPRWXTuvZLo/edit?usp=sharing > I2P: https://groups.google.com/a/chromium.org/d/topic/blink-dev/WvIeZT8uSzw/discussion > Manifest w3c PR: w3c/manifest#932 > > Bug: 1092667 > Change-Id: Ifbc0160a88263d4994d138f99921371ceea2bcb8 > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2436417 > Commit-Queue: Mike Jackson <mjackson@microsoft.com> > Reviewed-by: Alexey Baskakov <loyso@chromium.org> > Reviewed-by: Daniel Murphy <dmurph@chromium.org> > Cr-Commit-Position: refs/heads/master@{#811932} TBR=dmurph@chromium.org,loyso@chromium.org,mjackson@microsoft.com Change-Id: Ie953a019c6c9d0dc7e90ea2fcf639325460e8e11 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 1092667 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2441147 Reviewed-by: Daniel Murphy <dmurph@chromium.org> Commit-Queue: Daniel Murphy <dmurph@chromium.org> Cr-Commit-Position: refs/heads/master@{#812127}
6290db1
to
76f5ec1
Compare
I just responded to all comments, ready for review :) |
FYI this was discussed at TPAC 2020, minuted here: https://www.w3.org/2020/10/26-webapps-minutes.html#t07 (I wasn't present.) Just wanted to address:
There is a detailed discussion on this here. We explicitly didn't want to support non-legacy values in
If we let developers put the new values in either That is why we're proposing to only allow the new values in |
I'm OK with not allowing new values in |
FYI @marcoscaceres today is the last day for us to bikeshed on the name :) |
I think that wouldn't be good for compat until all browsers are implementing This isn't getting a lot of attention, and I'd like to set a deadline to land it in the Manifest spec. If not, I think we should move this into a WICG repo so that we can point to it without having to refer to this PR. @NotWoods are you planning to implement this in Gecko? If so, I think we can take that as a sign of multi-vendor interest, and land this as-is. If we only have Chromium on board, then we should move it into WICG. Should we set a deadline of EOD Friday November 20, to get approvals on this PR? |
@marcoscaceres is usually the one who handles Gecko implementation, I'll defer to him. I'm OK with adjusting the requirements later, especially since installablity criteria is not defined in the spec anyways. |
index.html
Outdated
@@ -1374,6 +1449,25 @@ <h3> | |||
developer's preferred <a>display mode</a> for the web application. | |||
</p> | |||
</section> | |||
<section> | |||
<h3> | |||
<code>display_override</code> member |
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.
<code>display_override</code> member | |
`display_override` member |
If this is a multi-os / multi-browser feature, I think it might be ok to take this on. Can someone verify for me that this works across, say, Chrome (OS?) and Edge with Windows integration? For a feature such as this, I think we can probably argue that OS level integration counts enough as an independent implementation... although we should look at if the Edge/Chrome implementations are truly independent. @dmurph, please note that I've removed all the WebIDL from the spec, so this needs some editorial cleanup to get it align with the new stuff in the spec. |
So this patch is a bit old - I've since put the spec language in here: and it looks like the references there are now broken too lol. Cost of having an incubations spec I guess. If we feel like this should live in w3c/manifest, then I'm super happy to move it into here. We haven't received signals from FF or Safari, and IDK how Edge is being treated now that they are based on Chromium. |
Yeah, it would be good to get another engine onboard before we add this. |
Hola! Saw this discussion and wanted to ping interested parties as I am looking into adding the "window-controls-overlay" into the display-override, but I am a bit confused as to why this is not in the manifest per se? Should we consider again adding display override to the main spec? What are the parameters that are being taken into account for not allowing it in? I'm thinking it might be confusing to have display in but display-override not, even though their functionalities are highly linked. Also, similar to @NotWoods , it might be confusing for new devs (tbf it is confusing for me atm)? To followup on @dmurph 's comments, I have checked with FF and Webkit on positions for WCO and have not gotten any signs of life from either, and I am concerned that for many upcoming features related to installed Web apps on desktop everything will be thrown into the 'incubations' bag? I kinda get that it is because there is no other implementation(-ish?), but oddly enough, unless I am mistaken it seems that |
Adding manual WPT tests for new display-override field. PR to add new field to manifest spec is here: w3c/manifest#932 Bug: 1149993 Change-Id: I670c69737e337a4c13e35672daa93e703f97b84e
Adding manual WPT tests for new display-override field. PR to add new field to manifest spec is here: w3c/manifest#932 Bug: 1149993 Change-Id: I670c69737e337a4c13e35672daa93e703f97b84e
Adding manual WPT tests for new display-override field. PR to add new field to manifest spec is here: w3c/manifest#932 Bug: 1149993 Change-Id: I670c69737e337a4c13e35672daa93e703f97b84e
Adding manual WPT tests for new display-override field. PR to add new field to manifest spec is here: w3c/manifest#932 Bug: 1149993 Change-Id: I670c69737e337a4c13e35672daa93e703f97b84e Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3166014 Commit-Queue: Mike Jackson <mjackson@microsoft.com> Reviewed-by: Daniel Murphy <dmurph@chromium.org> Cr-Commit-Position: refs/heads/main@{#922341}
Adding manual WPT tests for new display-override field. PR to add new field to manifest spec is here: w3c/manifest#932 Bug: 1149993 Change-Id: I670c69737e337a4c13e35672daa93e703f97b84e Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3166014 Commit-Queue: Mike Jackson <mjackson@microsoft.com> Reviewed-by: Daniel Murphy <dmurph@chromium.org> Cr-Commit-Position: refs/heads/main@{#922341}
Adding manual WPT tests for new display-override field. PR to add new field to manifest spec is here: w3c/manifest#932 Bug: 1149993 Change-Id: I670c69737e337a4c13e35672daa93e703f97b84e Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3166014 Commit-Queue: Mike Jackson <mjackson@microsoft.com> Reviewed-by: Daniel Murphy <dmurph@chromium.org> Cr-Commit-Position: refs/heads/main@{#922341}
…sts, a=testonly Automatic update from web-platform-tests dpwas: Add display_override tentative tests Adding manual WPT tests for new display-override field. PR to add new field to manifest spec is here: w3c/manifest#932 Bug: 1149993 Change-Id: I670c69737e337a4c13e35672daa93e703f97b84e Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3166014 Commit-Queue: Mike Jackson <mjackson@microsoft.com> Reviewed-by: Daniel Murphy <dmurph@chromium.org> Cr-Commit-Position: refs/heads/main@{#922341} -- wpt-commits: 351e54723427bcdcf0c1e75bcf10a5d221cf86ce wpt-pr: 30826
…sts, a=testonly Automatic update from web-platform-tests dpwas: Add display_override tentative tests Adding manual WPT tests for new display-override field. PR to add new field to manifest spec is here: w3c/manifest#932 Bug: 1149993 Change-Id: I670c69737e337a4c13e35672daa93e703f97b84e Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3166014 Commit-Queue: Mike Jackson <mjackson@microsoft.com> Reviewed-by: Daniel Murphy <dmurph@chromium.org> Cr-Commit-Position: refs/heads/main@{#922341} -- wpt-commits: 351e54723427bcdcf0c1e75bcf10a5d221cf86ce wpt-pr: 30826
…sts, a=testonly Automatic update from web-platform-tests dpwas: Add display_override tentative tests Adding manual WPT tests for new display-override field. PR to add new field to manifest spec is here: w3c/manifest#932 Bug: 1149993 Change-Id: I670c69737e337a4c13e35672daa93e703f97b84e Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3166014 Commit-Queue: Mike Jackson <mjackson@microsoft.com> Reviewed-by: Daniel Murphy <dmurph@chromium.org> Cr-Commit-Position: refs/heads/main@{#922341} -- wpt-commits: 351e54723427bcdcf0c1e75bcf10a5d221cf86ce wpt-pr: 30826
…sts, a=testonly Automatic update from web-platform-tests dpwas: Add display_override tentative tests Adding manual WPT tests for new display-override field. PR to add new field to manifest spec is here: w3c/manifest#932 Bug: 1149993 Change-Id: I670c69737e337a4c13e35672daa93e703f97b84e Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3166014 Commit-Queue: Mike Jackson <mjackson@microsoft.com> Reviewed-by: Daniel Murphy <dmurph@chromium.org> Cr-Commit-Position: refs/heads/main@{#922341} -- wpt-commits: 351e54723427bcdcf0c1e75bcf10a5d221cf86ce wpt-pr: 30826
Adding manual WPT tests for new display-override field. PR to add new field to manifest spec is here: w3c/manifest#932 Bug: 1149993 Change-Id: I670c69737e337a4c13e35672daa93e703f97b84e Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3166014 Commit-Queue: Mike Jackson <mjackson@microsoft.com> Reviewed-by: Daniel Murphy <dmurph@chromium.org> Cr-Commit-Position: refs/heads/main@{#922341}
Adding manual WPT tests for new display-override field. PR to add new field to manifest spec is here: w3c/manifest#932 Bug: 1149993 Change-Id: I670c69737e337a4c13e35672daa93e703f97b84e Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3166014 Commit-Queue: Mike Jackson <mjackson@microsoft.com> Reviewed-by: Daniel Murphy <dmurph@chromium.org> Cr-Commit-Position: refs/heads/main@{#922341} NOKEYCHECK=True GitOrigin-RevId: 7e650a5a28936cfe05fe9231614f056fd011bc12
Status
Since there were no other implementers that could comment on this, it has been moved into the manifest-incubations spec location.
Pull Request
Closes #???
I'm not sure there is one, but it is related to #856 and could close that - but that issue also goes over some other pieces that this change doesn't solve for yet.
This change (choose at least one, delete ones that don't apply):
Implementation commitment (delete if not making normative changes):
If change is normative, and it adds or changes a member:
I have not done this yet - should I do this before this request is filed?
Commit message:
Adds the "display_override" manifest field.
This field allows the developer to override the "display" field with a custom fallback chain. It is designed to be both future- and backwards-compatible, and is basically just a logic/algorithm change on how a user agent should determine the requested display mode.
This change also creates an explicit algorithm for determining the display mode, and provides a few new examples using "display_override".
See explainer here:
https://github.com/WICG/display-override/blob/master/explainer.md
TAG Review:
w3ctag/design-reviews#530
Webkit position request:
https://lists.webkit.org/pipermail/webkit-dev/2020-October/031435.html
Mozilla position request:
mozilla/standards-positions#447
Person merging, please make sure that commits are squashed with one of the following as a commit message prefix:
Preview | Diff