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] Merge error handling section into evaluating section #7595

Open
gsnedders opened this issue Aug 11, 2022 · 12 comments
Open

[mediaqueries] Merge error handling section into evaluating section #7595

gsnedders opened this issue Aug 11, 2022 · 12 comments

Comments

@gsnedders
Copy link
Member

gsnedders commented Aug 11, 2022

At this point, it's not clear what the motivation to have https://andreubotella.com/csswg-auto-build/mediaqueries-4/#error-handling as a separate section to https://andreubotella.com/csswg-auto-build/mediaqueries-4/#evaluating is.

The current split leads to some ambiguities in places:

An unknown <media-type> must be treated as not matching.

What does "not matching" mean, given "a media query is a logical expression that is either true or false"? (This also runs into #7594 given we don't define how to evaluate <media-type>.)

An unknown <mf-name> or <mf-value>, or a feature value which does not matches the value syntax for that media feature, results in the value “unknown”.

Results in the value "unknown" for what? Does it make the <media-feature> unknown, or the closest parent <media-condition> unknown, or does it make the whole <media-query> unknown? Implementations currently differ here!

A <media-query> whose value is “unknown” must be replaced with not all.

But, as above, a media query is either true or false? What does it mean to "replace" a <media-query>?

It seems like we may well want to define the rewriting to not all in Serializing Media Queries instead?

@gsnedders
Copy link
Member Author

Results in the value "unknown" for what? Does it make the <media-feature> unknown, or the closest parent <media-condition> unknown, or does it make the whole <media-query> unknown? Implementations currently differ here!

To quote @mdubet in web-platform-tests/wpt#35435:

Firefox Developer Edition (v104):

> window.matchMedia("(scan: foobar)")
> MediaQueryList { media: "not all", matches: false, onchange: null }

> window.matchMedia("(foobar: none) or (color)")
> MediaQueryList { media: "not all", matches: false, onchange: null }

In Chrome Canary:

> window.matchMedia("(scan: foobar)")
> MediaQueryList {media: '(scan: foobar)', matches: false, onchange: null}

> window.matchMedia("(foobar: none) or (color)")
> MediaQueryList {media: '(foobar: none) or (color)', matches: true, onchange: null}

@heycam heycam added the Agenda+ label Sep 29, 2022
@astearns astearns changed the title Merge error handling section into evaluating section [mediaqueries] Merge error handling section into evaluating section Oct 18, 2022
@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed [mediaqueries] Merge error handling section into evaluating section, and agreed to the following:

  • RESOLVED: Properly define resolution of media type and top-level MQ
  • RESOLVED: Clarify the spec text to reflect intended unknown mechanics
  • RESOLVED: Replace "replace by not all" with "evaluates to false", open a separate issue on serialization
The full IRC log of that discussion <fantasai> Topic: [mediaqueries] Merge error handling section into evaluating section
<fantasai> github: https://github.com//issues/7595
<fantasai> gsnedders1: We currently don't have interop with some types of errors around invalid media features
<florian> q+
<fantasai> gsnedders1: because essentially we don't define clearly how any of this is meant to work
<astearns> ack lea
<fantasai> gsnedders1: there were proposals to put this into Interop 2023
<fantasai> gsnedders1: need to know what exactly we'll be testing
<fantasai> gsnedders1: a few sections
<fantasai> gsnedders1: First, we don't define how to evaluate media type or the top-level media query at all
<fantasai> gsnedders1: and we just say that an unknown media type is treated as not matching
<fantasai> gsnedders1: whereas unknown media feature [logic]
<fantasai> gsnedders1: Suggest to define how to evaluate MQ with handwavy definition of media types matching if appropriate
<fantasai> TabAtkins: Agree
<astearns> ack florian
<fantasai> florian: I agree, media type is not properly defined, but that's not hard. Can be more specific than mentioned
<fantasai> florian: since most deprecated by now
<fantasai> florian: so only two cases where we differ from screen
<fantasai> florian: for top-level MQ, what it ought to be is fairly obvious but we should write it down
<fantasai> gsnedders1: Proposed resolution is to define how to evaluate media query and media type
<fantasai> gsnedders1: in somewhat obvious way
<fantasai> astearns: any objections?
<fantasai> RESOLVED: Properly define resolution of media type and top-level MQ
<fantasai> gsnedders1: Next, unknown media name or ???
<fantasai> gsnedders1: results in value of uknown
<fantasai> gsnedders1: but we don't say what exactly is given unknown
<fantasai> gsnedders1: an MF name or MF value doesn't have a value
<fantasai> gsnedders1: Currently FF and Chrome differ
<fantasai> gsnedders1: FF makes the entire MQ uknown if any media feature name or value is unknown
<fantasai> gsnedders1: whereas Chrome only makes that specific one unknown
<fantasai> florian: Chrome is right, the whole point of this logic is doing this
<fantasai> TabAtkins: [agrees]
<fantasai> gsnedders1: I can justify both behaviors from current spec text
<fantasai> florian: We'll clarify, the intention is 100% clear to the spec editors, so we just need to fix the tet
<fantasai> RESOLVED: Clarify the spec text to reflect intended unknown mechanics
<fantasai> gsnedders1: Final bit is, MQ whose value is unknown must be replaced with "not all"
<fantasai> gsnedders1: but not clear what it means to replace an MQ
<fantasai> gsnedders1: maybe need to define MQ serialization?
<fantasai> TabAtkins: I think that's right, but...
<fantasai> TabAtkins: I think it's a serialization issue
<fantasai> gsnedders1: I think we need to say that unknown MQ evaluates to false, and serializes to "not all"
<fantasai> florian: makes sense to me
<fantasai> emilio: We don't have this thing to support ... with generalized enclosed
<fantasai> emilio: is that a compat requirement?
<fantasai> florian: part where unknown MQ evaluates to false, that is absolutely desired intent
<fantasai> florian: whether accidentally replaced with "not all" with no effect on serialization or whether was intended to talk about serialization, then I don't know
<fantasai> gsnedders1: I think it goes back to CSS2
<TabAtkins> MQ has a bunch of legacy weirdness that I didn't replace
<fantasai> gsnedders1: It could literally just be copy-pasted from earlier level with no thinking
<fantasai> emilio: WE don't implement this update to the spec, but if we did we'd probably want to be consistent with @supports
<fantasai> emilio: and @supports when you throw something random at it, you serialize that
<fantasai> emilio: rather than a false statement
<fantasai> florian: should we say, gets evaluated as false? or does that lose something important?
<fantasai> emilio: I think so, but not sure how that maps. I think spec said this had to be some value with 3-way state...
<fantasai> florian: when you propagate unknown ...
<fantasai> emilio: That was not the desired behavior, and we changed it. Unknown just evaluates as false
<fantasai> emilio: so probably we want to be consistent, and unknown evaluates as false?
<fantasai> TabAtkins: we very specifically want @supports unknown to be false, because if you can't parse it you definitely don't support it
<fantasai> TabAtkins: but for MQ, whether you are that type of media or not, you don't know if you can't parse the MQ
<fremy> +1 to what TabAtkins just said
<fantasai> florian: I think that the "gets replaced by not all" is an ancient and imprecise way of saying evaluate as false, and we should replace this and not talk about serialization
<fantasai> gsnedders1: with prior resolution, very few if anything will result in the whole MQ being unknown
<fantasai> TabAtkins: not necessarily true, e.g. combine with and and you'll get unknown propagate up
<fantasai> gsnedders1: yes, nevermind
<fantasai> astearns: Should we resolve, or need more time?
<fantasai> emilio: Fine as long as we're all on the same page
<fantasai> florian: Instead of "replace by not all" say "evalutates to false"
<fantasai> gsnedders1: define serialization of unknown MQ to be "not all", is that still the intention?
<fantasai> florian: Let's open a separate issue for serialization, since might have compat concerns
<fantasai> florian: Want to look into that offline
<fantasai> RESOLVED: Replace "replace by not all" with "evaluates to false", open a separate issue on serialization

@tabatkins
Copy link
Member

Okay, so at least we need to preserve a distinction between "false" and "invalid" - the latter serializes as "not all" for legacy reasons, but the former is serialized properly. We don't expose MQs in any form except serialized, so whether the "not all"-ification is a a serialization thing or actually replacing the MQ is up to us. (I'd favor it being serialization.)

I now recall that I specifically carried over the "not all" behavior for things that resolve to unknown because that's what previous UAs would do to such MQs. It's not perfect; they'd also serialize (unknown) or (min-width: 0) as not all, whereas now it'll be true and just serialize as-is. That said, I'm fine with breaking compat more fully here and making unknown things always serialize properly. However, I suspect that the "unknown media type" still needs to serialize to "not all", as that's likely the most common way we're exposing this behavior today.

@gsnedders
Copy link
Member Author

I now recall that I specifically carried over the "not all" behavior for things that resolve to unknown because that's what previous UAs would do to such MQs.

The tests currently seem to use serialising not all as a test for "media query could be parsed", after @andruud's changes back in May (which match his implementation in Chrome), i.e., only expecting not all for:

A media query that does not match the grammar in the previous section must be replaced by not all during parsing.

For example, the tests assert that @media screen, (max-width) {} must serialise as @media screen, (max-width) {}.

So it seems like Chrome doesn't implement that serialising behaviour now? And it sounded on the call like @emilio was reasonably questioning whether we want to rewrite it on serialisation rather than propagating the unknown media query to the output (which to me sounds like the sensible thing to do), and if Chrome is currently shipping then I guess we have the evidence it's web compatible.

However, I suspect that the "unknown media type" still needs to serialize to "not all", as that's likely the most common way we're exposing this behavior today.

We don't seem to have tests for that, at least.

@tabatkins
Copy link
Member

Oh, that's very promising then.

@cdoublev
Copy link
Collaborator

cdoublev commented Nov 15, 2022

Minor note: there is nothing about <mf-value>s in the "wrong order" in <mf-range>, eg. (2px < width < 1px). I will consider that this case must always parse as valid but must evaluate to false (as Chrome/FF do), which is the "default" behavior, but this does not conform to the matching convention mentioned in CSS Values for clamp(), and I am not sure it should conform to this convention.

EDIT: a 3-value range in a wrong order can only be false, therefore I change my mind and I think it should be a parse error, so that the author can fix it.

@tomayac
Copy link
Contributor

tomayac commented Aug 16, 2023

Before this change, it was possible to detect non-supported media queries by checking for "not all", as developers, including myself, have documented in blog posts like this.

window.matchMedia('(prefers-color-scheme: dark)').media
// '(prefers-color-scheme: dark)'

// This check no longer works.
window.matchMedia('(hahaha-lol: not-a-thing)').media
// Before: 'not all'
// Now: '(hahaha-lol: not-a-thing)'

Is this a potential web compatibility issue, as feature-detecting code like the above now breaks?

(CC: @mathiasbynens in the context of this DevTools CL)

@tabatkins
Copy link
Member

Minor note: there is nothing about <mf-value>s in the "wrong order

Nothing particularly wrong with that. It'll never be true, but it's just a shorthand for writing the two comparisons separately, which you can always do. Making it a parse error would be unnecessarily complex, and would mean that things like env() evaluation have to happen at parse time, which seems unnecessary.

@tomayac
Copy link
Contributor

tomayac commented Aug 17, 2023

(FWIW, updated my blog post.)

@cdoublev
Copy link
Collaborator

cdoublev commented Oct 7, 2023

Can you please confirm that your intent is to apply the not all-ification only in the first case below?

  1. 1: it does not match any branch of the grammar
  2. unknown: it matches <media-type> (<ident>) and evaluates to unknown
  3. (unkown): it matches <general-enclosed> (or <mf-name> -> <mf-boolean> -> <media-feature>?) and evaluates to unknown

I would definitely expect it to apply in (this very contrived example):

styleSheet.insertRule(`@media ${matchMedia(';').media} {}}`)

@tomayac, maybe this could be a solution?

Resolved: Add an at-rule function with syntax at-rule(@keyword) or at-rule(@keyword; descriptor: value)

#2463 (comment)

For example, @supports at-rule(@media; some-feature). Media queries are not technically descriptors though, so maybe @supports media-query(some-feature).

@tomayac
Copy link
Contributor

tomayac commented Oct 9, 2023

#2463 (comment)

For example, @supports at-rule(@media; some-feature). Media queries are not technically descriptors though, so maybe @supports media-query(some-feature).

Thanks, I hadn't seen this before. I assume this would be queryable with CSS.supports('at-rule(@media; some-feature)')!? In a contrived example where (some-feature: value1 | value2), but where only value2 would be supported, could you somehow query this (with CSS or JS)? I guess more concretely I'm actually asking if CSS.supports('at-rule(@media; (prefers-color-scheme: dark)') would work, or only CSS.supports('at-rule(@media; prefers-color-scheme'). The way I read it, both should work?!

@cdoublev
Copy link
Collaborator

cdoublev commented Oct 9, 2023

Yes, I also assume it.

We can already do CSS.supports('selector(type) and selector(.class)'), so you could do CSS.supports('media-query(some-feature: value1) or media-query(some-feature: value2)') (declaration), CSS.supports('media-query(some-feature > 1px)') (range), or CSS.supports('media-query(some-feature)') (boolean).

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

No branches or pull requests

7 participants