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

[mediaqueries-5] Move the definition of display-mode back to APPMANIFEST. Closes #7306. #7307

Closed
wants to merge 10 commits into from

Conversation

mgiuca
Copy link
Contributor

@mgiuca mgiuca commented May 25, 2022

This text was moved out of the Manifest spec into CSS mediaqueries-5 in
#6343, along with the display-mode media feature. The
actual definition of display mode belongs in Manifest, while the
display-mode media feature remains here.

Added some more text explaining how the display-mode media feature works
given that this is in a separate spec to where web apps are defined.

@aarongustafson
Copy link

aarongustafson commented May 25, 2022

The Manifest spec is aligned and that corresponding PR can be merged at the same time.

@marcoscaceres
Copy link
Member

It would be good if we then linked the display-mode values back to the Web Manifest spec (so people can easily look up what each one means). We've updated the other PR to export them.

@mgiuca
Copy link
Contributor Author

mgiuca commented May 26, 2022

Good idea. Is this too verbose? (Explicitly listing each one and linking it to the manifest) This way there is a separate explicit definition of "the standalone display-mode MQ feature" (defined in CSS), distinct from the definition of "the standalone display mode" (defined in Manifest).

Also, I can't really verify that this linking will work... right now when I build it, it links to the CSS spec, but I assume once both of these PRs land, those links will go back to the Manifest spec.

@mgiuca
Copy link
Contributor Author

mgiuca commented May 26, 2022

@frivoal for review, please when you get a chance.

@aarongustafson
Copy link

In ImageResource we just reference the sizes attribute in HTML and don't get in to acceptable values and such. I'd leave that the referenced spec to define.

@mgiuca
Copy link
Contributor Author

mgiuca commented May 26, 2022

In ImageResource we just reference the sizes attribute in HTML and don't get in to acceptable values and such. I'd leave that the referenced spec to define.

I'm not quite clear what you're proposing? Are you saying I should revert the last commit (disagree with Marcos)? Or some middle ground.

FWIW the intention here is that we're defining two different things: the "display modes" which are the modes you can request in the Manifest, and the "display-mode media query values" which are syntactic values that appear in CSS. We should explicitly define the meaning of each of those syntactic words to be the corresponding word in Manifest.

@marcoscaceres marcoscaceres requested a review from frivoal May 26, 2022 07:34
This media feature reflects the actual display mode that is being used on the current browsing
context, not necessarily the one that was requested in the web app manifest (if any).

On normal web pages, 'display-mode' will have a value of ''browser''. Only pages appearing in the
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure we should say anything about "normal web pages" (unless that's actually a thing?).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I wasn't really sure how to express this, but I am trying to simply say (since we're in the context of CSS and not the manifest) that "normally" (on the "drive-by web") this will be "browser".

I've reworded it and put it in a note. The non-normative context I think allows me to talk about "web browsers" as a special class of user agent, which I wouldn't normally talk about in normative text.

@aarongustafson
Copy link

I'm not quite clear what you're proposing? Are you saying I should revert the last commit (disagree with Marcos)? Or some middle ground.

FWIW the intention here is that we're defining two different things: the "display modes" which are the modes you can request in the Manifest, and the "display-mode media query values" which are syntactic values that appear in CSS. We should explicitly define the meaning of each of those syntactic words to be the corresponding word in Manifest.

Sorry, I should have been more detailed. What I was saying was that I'm not sure the values are as necessary in the CSS spec as they should be bound to the values in the Manifest, right? For instance, when display_override lands, I’d expect the value that is ultimately applied in the browser would be the one popped through to the media query. When that lands, we wouldn't need to update the Media Query spec to include it if the spec references the acceptable values in the Manifest.

@mgiuca
Copy link
Contributor Author

mgiuca commented May 27, 2022

Sorry, I should have been more detailed. What I was saying was that I'm not sure the values are as necessary in the CSS spec as they should be bound to the values in the Manifest, right? For instance, when display_override lands, I’d expect the value that is ultimately applied in the browser would be the one popped through to the media query. When that lands, we wouldn't need to update the Media Query spec to include it if the spec references the acceptable values in the Manifest.

So what you're saying is that we shouldn't duplicate the list of possible values, and instead just say "it can be any value from the "display mode" list in the manifest"?

I think avoiding that duplication would be ideal, but my feeling from reading the CSSMQ spec is that this is formally defining the CSS syntax and we can't just delete the blue box and put in a hand-wave "see the list from the manifest". So we do need to duplicate the list of possible values. I think it's OK: adding new display modes will be pretty rare (we'll add maybe 2 in the next couple of years) so we will just be sure to add an entry here when we do.

@mgiuca mgiuca force-pushed the move-display-mode-back branch from 053a0fe to 609216b Compare November 4, 2022 02:47
@mgiuca mgiuca force-pushed the move-display-mode-back branch from 609216b to fe67167 Compare November 16, 2023 06:04
@mgiuca
Copy link
Contributor Author

mgiuca commented Nov 20, 2023

Hi CSS folks, including @frivoal .

I have finally gotten around to updating this PR with the changes discussed around this time last year. Apologies for the slow turnaround on this - this was disrupted at the start of this year and been on my backburner since then.

I believe my edits have captured the essence of what was requested in the CSSWG meeting from 17 Nov, 2022.

A brief recap:

  • In December 2021, the definition of "display mode" and its corresponding values was moved from the Manifest spec to the Media Queries spec. This is because it was defined as both a JSON value in the Manifest and a media query feature.
  • I argued that the definition should be in the Manifest spec, as the display mode is a core concept (or rather, the core concept) defined by the web app manifest, and the media query was just a convenience feature for developers. I proposed two PRs: this one, and the corresponding one in the Manifest spec to put it back.
  • In November 2022, the CSSWG agreed in principle to move the definition back, but argued that because "fullscreen" is triggered by not only the manifest "display": "fullscreen" but also by any other type of fullscreen, it isn't proper to have that display mode defined in Manifest when the media query works independent of the manifest.

So for today's update to the PR, I have:

  1. Largely rewritten the PR, so that rather than saying "this MQ reflects the display mode from the Manifest", it's more neutral: it says that it reflects the way the page is being presented to the user, with the Manifest display mode being one of the ways it can be modified, various ways of entering fullscreen being another, and "browser" being the default.
  2. Added an explicit definition of "browser" and "fullscreen" modes here in CSS. The "fullscreen" one is neutral as to how it can be triggered (manifest, requestFullscreen or user control). The other two modes are defined in terms of the Manifest spec, as they can only be entered from an application context.
  3. Added an example.

There are two main aspects that I wish to keep in the Manifest spec:

  • The definition of the term "display mode" itself, since that is the core concept which that spec revolves around.
  • All the prose text describing display modes other than browser and fullscreen (the two modes which can be triggered outside of an application context). (i.e. I do not want there to be prose in CSS-MQ describing the standalone and fullscreen display modes). This is for future proofing: we intend to add more display modes going forward (two new modes, window-controls-overlay and tabbed are explicitly planned), and we want the process of adding new display modes to be that you define their meaning in full in the Manifest spec, and then simply add their name to the Media-Queries spec, linking back to the Manifest definition, and not duplicating any prose.

Please take a look and let me know if this is going to be discussed at a CSSWG meeting.

Cheers.

@mgiuca
Copy link
Contributor Author

mgiuca commented Feb 26, 2024

Hi @frivoal . Friendly ping, would you be able to review the revised pull request or re-raise it at a WG meeting, whatever the process may be? I believe the last update satisfied your feedback.

mgiuca and others added 7 commits February 26, 2024 16:11
.

This text was moved out of the Manifest spec into CSS mediaqueries-5 in
w3c#6343, along with the display-mode media feature. The
actual definition of display mode belongs in Manifest, while the
display-mode media feature remains here.

Added some more text explaining how the display-mode media feature works
given that this is in a separate spec to where web apps are defined.
Co-authored-by: Marcos Cáceres <marcos@marcosc.com>
- No longer solely based on manifest display modes, since browser and fullscreen are meaningful independent of manifest.
- Added a solid definition for browser and fullscreen rather than relying on that of the manifest spec.
- Added an example.
@mgiuca
Copy link
Contributor Author

mgiuca commented Feb 26, 2024

Oh no, I just saw the merge conflict. #9920 has added a new value to CSS display-mode (but not Manifest display-mode). This makes disentangling the two more complex.

I will take another pass at this now.

beaufortfrancois and others added 3 commits February 26, 2024 16:14
Note that "web application" is incorrect as this is not a web app display mode.
This changes the text to "browsing context" to be consistent with the
other changes made in this PR.
This now accounts for the fact that there are now more than two
non-application-context display modes.
@mgiuca mgiuca force-pushed the move-display-mode-back branch from 6552d10 to 34ae1b6 Compare February 26, 2024 05:35
@mgiuca
Copy link
Contributor Author

mgiuca commented Feb 26, 2024

Updated. I have reworked things so that we now have a separation between the "display mode" in the manifest (which defines the four display modes) and the values of the "display-mode" MQ (which now has five possible values).

The text in this PR is now clear on the fact that display-mode can represent the display mode for an application context (like standalone or minimal-ui), or it can also reflect other non-application display modes (like fullscreen or picture-in-picture).

This is a bit trickier now that there isn't a 1:1 mapping between "manifest display modes" and "MQ display modes" but I think this text makes sense. I am similarly making small non-normative changes on the Manifest side so that it's clearer.

@frivoal @tabatkins @beaufortfrancois FYI

@marcoscaceres
Copy link
Member

@frivoal, @mgiuca, can this be merged yet? What's the latest?

@grorg grorg closed this in e8cc514 Apr 12, 2024
@grorg
Copy link
Contributor

grorg commented Apr 12, 2024

Landed this as shown above.

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.

5 participants