Florian Rivoal Reporter
	Description • 7 years ago

CSS Media Quries Level 4 bring in a bunch of syntactic improvements. They make media queries much more readable and maintainable.

https://drafts.csswg.org/mediaqueries-4/#mq-syntax

Here's an example:

Before:

@media (min-width: 20em), not all and (min-height: 40em) {
  @media not all and (pointer: none) {
    …
  }
}

After:

@media ((width >= 20em) or (height < 40em)) and (pointer) {
  …
}

	Florian Rivoal Reporter
	Comment 1 • 7 years ago

This should be marked as blocking https://bugzilla.mozilla.org/show_bug.cgi?id=1312621, and either be marked as being blocked by https://bugzilla.mozilla.org/show_bug.cgi?id=1337174 or have that bug be closed in favor of this one, since this is a superset of that.

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 9 • 6 years ago

There's a further case these patches don't handle, which is the range syntax where there's a value preceding the name like:

  (50px < width < 600px)
  (40px > width)

I'm discussing that in https://github.com/w3c/csswg-drafts/issues/2791. I'm not particularly convinced this syntax is particularly useful compared to `(width > 50px) and (width < 600px)`, for example, and it is kind of a pain to implement. In any case the way these patches are structured it should be doable to make it a followup.

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 10 • 6 years ago

Another (pre-existing) divergence from the spec is the general enclosed thingie...

Per spec, `all and (width > 100px) and (unknown-feature)` should be a valid media query, but right now all browsers turn it into `not all`.

Again, this can be a followup if needed, but I found no good way of implementing this and keeping media expressions testable...

	Xidorn Quan [:xidorn] UTC+11
	Comment 11 • 6 years ago

We probably shouldn't land this before merge day.

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 12 • 6 years ago

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #11)
> We probably shouldn't land this before merge day.

Agreed.

	Florian Rivoal Reporter
	Comment 19 • 6 years ago

> I'm not particularly convinced this syntax is particularly useful

While in isolation, the difference between the two isn't that bad, when combined in more complex queries, the improvement in legibility is noticeable and welcome.

> Another (pre-existing) divergence from the spec is the general enclosed thingie...

Yes, that is an intentional departure from pre-existing behavior, for the sake of better future compatibility, and to make progressive adoption by authors easier.

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 20 • 6 years ago

(In reply to Florian Rivoal from comment #19)
> > I'm not particularly convinced this syntax is particularly useful
> 
> While in isolation, the difference between the two isn't that bad, when
> combined in more complex queries, the improvement in legibility is
> noticeable and welcome.

I agree, though I think it makes the implementation noticeably harder (and slower) as I noted in the spec issue. In any case since that would be a further enhancement, I think it's worth doing it in a followup bug.

> > Another (pre-existing) divergence from the spec is the general enclosed thingie...
> 
> Yes, that is an intentional departure from pre-existing behavior, for the
> sake of better future compatibility, and to make progressive adoption by
> authors easier.

While I see why, that means media queries would become no longer testable (there's no way to distinguish a media query parse error from a media query which just isn't matching). Given the amount of test changes that that would need, and the risk of breaking existing content that could depend on the existing behavior, I think I should probably do it in a different bug. All our existing tests (both in m-c and in wpt) rely on this heavily (see [1] for example). I don't think I have a good idea re. how to rewrite those....

I'll file bugs for both.

[1]: https://searchfox.org/mozilla-central/rev/6a27ff48dda8376d1f819c534510eca7d9af5818/layout/style/test/test_media_queries.html#83

	Xidorn Quan [:xidorn] UTC+11
	Comment 21 • 6 years ago
mozreview-review

Comment on attachment 8985758 [details]
Bug 1422225: Allow parsing operators in media feature expressions.

https://reviewboard.mozilla.org/r/251292/#review258108

r=me with the issue below addressed.

::: servo/components/style/gecko/media_queries.rs:741
(Diff revision 2)
> +
> +            let range_or_operator = match range {
> +                Some(range) => {
> +                    if operator.is_some() {
> +                        return Err(input.new_custom_error(
> +                            StyleParseErrorKind::MediaQueryExpectedFeatureValue

This error kind feels wrong. It should probably be a new error kind something like "UnexpectedOperator" or something like that.

::: servo/components/style/gecko/media_queries.rs:749
(Diff revision 2)
> +                    Some(RangeOrOperator::Range(range))
> +                }
> +                None => {
> +                    match operator {
> +                        Some(operator) => {
> +                            if operator != Operator::Equal && !feature_allows_ranges {

It seems to me that the spec doesn't allow using even equal operator for features which don't have "range" type.

The spec says:
> Media features with a “range” type can be alternately written in a range context

but doesn't mention anything else for other, and operator can only appear in "range context".

So I believe you need to reject when `feature_allows_ranges` is false regardless of whether the operator is equal.

::: servo/components/style/gecko/media_queries.rs:751
(Diff revision 2)
> +                None => {
> +                    match operator {
> +                        Some(operator) => {
> +                            if operator != Operator::Equal && !feature_allows_ranges {
> +                                return Err(input.new_custom_error(
> +                                    StyleParseErrorKind::MediaQueryExpectedFeatureValue

Again, the error kind seems wrong.

	Xidorn Quan [:xidorn] UTC+11
	Comment 22 • 6 years ago
mozreview-review

Comment on attachment 8985759 [details]
Bug 1422225: Rename Expression to MediaFeatureExpression.

https://reviewboard.mozilla.org/r/251294/#review258112

::: servo/components/style/gecko/media_queries.rs:274
(Diff revision 2)
>  enum RangeOrOperator {
>      Range(Range),
>      Operator(Operator),
>  }
>  
> -/// A expression for gecko contains a reference to the media feature, the value
> +/// A range expression for gecko contains a reference to the media feature, the

This is not necessarily a "range expression". You should keep using "expression" or change it to "media feature expression".

Maybe the type should be just `MediaExpression` given we have `MediaExpressionValue`...

	Xidorn Quan [:xidorn] UTC+11
	Comment 23 • 6 years ago
mozreview-review

Comment on attachment 8985760 [details]
Bug 1422225: Add code to parse media conditions.

https://reviewboard.mozilla.org/r/251296/#review258114

::: servo/components/style/gecko/media_queries.rs:637
(Diff revision 2)
>          input.parse_nested_block(|input| {
> +            Self::parse_in_parenthesis_block(context, input)
> +        })
> +    }
> +
> +    /// Parse a media range expression where we've already consumed the

"media feature expression" or just "media expression".

	Xidorn Quan [:xidorn] UTC+11
	Comment 24 • 6 years ago
mozreview-review

Comment on attachment 8985761 [details]
Bug 1422225: Add serialization code for MediaCondition.

https://reviewboard.mozilla.org/r/251298/#review258122

	Xidorn Quan [:xidorn] UTC+11
	Comment 25 • 6 years ago
mozreview-review

Comment on attachment 8985762 [details]
Bug 1422225: Evaluate MediaConditions, and glue it all together.

https://reviewboard.mozilla.org/r/251300/#review258206

::: servo/components/style/media_queries/media_condition.rs:26
(Diff revision 2)
>  pub enum Operator {
>      And,
>      Or,
>  }
>  
> +/// Whether to allow an `or` condition or not during parsing.

You should make it clear here and in the document of `parse_disallow_or` that this only applies to the top level, and nested parsing always allows `or`.

	Xidorn Quan [:xidorn] UTC+11
	Comment 26 • 6 years ago
mozreview-review

Comment on attachment 8985763 [details]
Bug 1422225: Test updates.

https://reviewboard.mozilla.org/r/251302/#review258212

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 27 • 6 years ago

I just noticed that this breaks the error reporting tests for media queries, and I don't find a great way of fixing it without changing how error reporting works...

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 28 • 6 years ago

I did find a less intrusive way for now, will consider changing it later, probably when implementing <general-enclosed>.

	Xidorn Quan [:xidorn] UTC+11
	Comment 36 • 6 years ago
mozreview-review

Comment on attachment 8986843 [details]
Bug 1422225: Error reporting fixes.

https://reviewboard.mozilla.org/r/252092/#review258642

::: servo/components/style/media_queries/media_query.rs:129
(Diff revision 1)
>      /// Returns an error if any of the expressions is unknown.
>      pub fn parse<'i, 't>(
>          context: &ParserContext,
>          input: &mut Parser<'i, 't>,
> -    ) -> Result<MediaQuery, ParseError<'i>> {
> -        if let Ok(condition) = input.try(|i| MediaCondition::parse(context, i)) {
> +    ) -> Result<Self, ParseError<'i>> {
> +        if let Ok(query) = input.try(|i| Self::parse_legacy(context, i)) {

First of all, I don't think media type is something "legacy" at all. It is still one of the most important condition in media query.

And this leads me to question the structure of the whole parsing function here.

Something like
```rust
let (qualifier, explicit_media_type) = input.try(|i| {
    let qualifier = i.try(Qualifier::parse).ok();
    let location = i.current_source_location();
    let ident = i.expect_ident()?;
    (qualifier, Some((ident, location)))
}).unwrap_or((None, None));
let media_type = explicit_media_type.map_or(
    Ok(MediaQueryType::All),
    |(ident, location)| {
        MediaQueryType::parse(&ident).map_err(|_| {
            location.new_custom_error(
                SelectorParseErrorKind::UnexpectedIdent(ident.clone())
            )
        })
    }
)?;
let condition = if explicit_media_type.is_none() {
    Some(MediaCondition::parse(context, input)?)
} else {
    if input.try(|i| i.expect_ident_matching("and")).is_ok() {
        Some(MediaCondition::parse_disallow_or(context, input)?)
    } else {
        None
    }
};
Ok(Self { qualifier, media_type, condition })
```
should be better on both error reporting and code semantics.

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 37 • 6 years ago

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #36)
> Comment on attachment 8986843 [details]
> Bug 1422225: Error reporting fixes.
> 
> https://reviewboard.mozilla.org/r/252092/#review258642
> 
> ::: servo/components/style/media_queries/media_query.rs:129
> (Diff revision 1)
> >      /// Returns an error if any of the expressions is unknown.
> >      pub fn parse<'i, 't>(
> >          context: &ParserContext,
> >          input: &mut Parser<'i, 't>,
> > -    ) -> Result<MediaQuery, ParseError<'i>> {
> > -        if let Ok(condition) = input.try(|i| MediaCondition::parse(context, i)) {
> > +    ) -> Result<Self, ParseError<'i>> {
> > +        if let Ok(query) = input.try(|i| Self::parse_legacy(context, i)) {
> 
> First of all, I don't think media type is something "legacy" at all. It is
> still one of the most important condition in media query.
> 
> And this leads me to question the structure of the whole parsing function
> here.
> 
> Something like
> ```rust
> let (qualifier, explicit_media_type) = input.try(|i| {
>     let qualifier = i.try(Qualifier::parse).ok();
>     let location = i.current_source_location();
>     let ident = i.expect_ident()?;
>     (qualifier, Some((ident, location)))
> }).unwrap_or((None, None));
> let media_type = explicit_media_type.map_or(
>     Ok(MediaQueryType::All),
>     |(ident, location)| {
>         MediaQueryType::parse(&ident).map_err(|_| {
>             location.new_custom_error(
>                 SelectorParseErrorKind::UnexpectedIdent(ident.clone())
>             )
>         })
>     }
> )?;
> let condition = if explicit_media_type.is_none() {
>     Some(MediaCondition::parse(context, input)?)
> } else {
>     if input.try(|i| i.expect_ident_matching("and")).is_ok() {
>         Some(MediaCondition::parse_disallow_or(context, input)?)
>     } else {
>         None
>     }
> };
> Ok(Self { qualifier, media_type, condition })
> ```
> should be better on both error reporting and code semantics.

I agree with the naming of the function being unfortunate, but I did do that and kind of disagree with the rest because of the following condition:

  @media not (foo: bar)

Would get parsed as:

  qualifier: Some(Not),
  type: All,
  condition: Some((foo: bar)),

instead of the not being part of the condition, which doesn't quite match what i'd expect to get parsed. But I guess it's fine. If you think with the not weirdness it's still not worth I can do something like that.

	Xidorn Quan [:xidorn] UTC+11
	Comment 38 • 6 years ago

No, that would not be that case. 

> @media not (foo: bar)
would get parsed in the first part  into
> (qualifier, explicit_media_type) = (None, None)
because expect_ident would fail.

This then triggers the explicit_media_type.is_none() branch to parse the whole "not (foo: bar)" as MediaCondition.

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 39 • 6 years ago

Ah, I see, makes sense.

	Xidorn Quan [:xidorn] UTC+11
	Comment 41 • 6 years ago
mozreview-review

Comment on attachment 8986843 [details]
Bug 1422225: Error reporting fixes.

https://reviewboard.mozilla.org/r/252092/#review258654

::: servo/components/style/media_queries/media_query.rs:131
(Diff revision 2)
> -            })
> -        }
> -
> -        let qualifier = input.try(Qualifier::parse).ok();
> +            let qualifier = input.try(Qualifier::parse).ok();
> -        let media_type = {
> -            let location = input.current_source_location();
> +            let ident = input.expect_ident().map_err(|_| ())?;
> +            let media_type = MediaQueryType::parse(&ident)?;

The trick in my suggestion is that we parse the media query type in a separate step so that we have better error reporting for cases like `not and` with a reasonable location.

Embedding it into the same try block would regress that, so I would still prefer my approach, but it's probably not too bad.

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 42 • 6 years ago

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #41)
> The trick in my suggestion is that we parse the media query type in a
> separate step so that we have better error reporting for cases like `not
> and` with a reasonable location.
> 
> Embedding it into the same try block would regress that, so I would still
> prefer my approach, but it's probably not too bad.

Would it, though? It'd report the same "unexpected token: 'and'" message, unless I keep the "ExpectedIdent" error, or add a more specific ExpectedMediaQueryType error, or something like that.

Note that I made the basic UnexpectedToken error be reported, which gives nice errors, and nicer code as well IMO :).

	Xidorn Quan [:xidorn] UTC+11
	Comment 43 • 6 years ago

Hmmm... okay. Then that's probably fine, and I guess your commit message can be revised since I don't see how the error reporting is regressed now.

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 44 • 6 years ago

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #43)
> Hmmm... okay. Then that's probably fine, and I guess your commit message can
> be revised since I don't see how the error reporting is regressed now.

Yup, that's right, good point. Thanks for pointing me to how to fix that! :)

	Pulsebot
	Comment 45 • 6 years ago

Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/mozilla-inbound/rev/84d2cf2f3712
Allow parsing operators in media feature expressions. r=xidorn
https://hg.mozilla.org/integration/mozilla-inbound/rev/e3d921c2cbde
Rename Expression to MediaFeatureExpression. r=xidorn
https://hg.mozilla.org/integration/mozilla-inbound/rev/6510e60ba0da
Add code to parse media conditions. r=xidorn
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb0e34acdf4b
Add serialization code for MediaCondition. r=xidorn
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b28e45d0d04
Evaluate MediaConditions, and glue it all together. r=xidorn
https://hg.mozilla.org/integration/mozilla-inbound/rev/24575413384a
Test updates. r=xidorn
https://hg.mozilla.org/integration/mozilla-inbound/rev/d4ef1878e5b7
Error reporting fixes. r=xidorn

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 46 • 6 years ago

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/11664 for changes under testing/web-platform/tests

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 47 • 6 years ago

Can't merge web-platform-tests PR due to failing upstream checks:
Github PR https://github.com/web-platform-tests/wpt/pull/11664
* continuous-integration/travis-ci/pr (https://travis-ci.org/web-platform-tests/wpt/builds/396579370?utm_source=github_status&utm_medium=notification)

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 48 • 6 years ago

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

	Eliza Balazs [:ebalazs_]
	Comment 49 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/84d2cf2f3712
https://hg.mozilla.org/mozilla-central/rev/e3d921c2cbde
https://hg.mozilla.org/mozilla-central/rev/6510e60ba0da
https://hg.mozilla.org/mozilla-central/rev/bb0e34acdf4b
https://hg.mozilla.org/mozilla-central/rev/5b28e45d0d04
https://hg.mozilla.org/mozilla-central/rev/24575413384a
https://hg.mozilla.org/mozilla-central/rev/d4ef1878e5b7

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 50 • 6 years ago

Upstream PR merged

	Rachel Andrew
	Comment 53 • 6 years ago

I have done the documentation work for this bug.

Updated the table and also added a section about range contexts here: https://developer.mozilla.org/en-US/docs/Web/CSS/Media_Queries/Using_media_queries#Syntax_improvements_in_Level_4

Added two examples here: https://developer.mozilla.org/en-US/docs/Web/CSS/@media

I'm appreciate a review of those docs, please let me know if you see any problem or think there is something else we should cover here.

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 54 • 6 years ago

It may be nice to point out that `or` conditions can also be used now and such, that expressions can be nested, and similarly that negations can now apply to sub expressions.

Other than that looks great, thanks a lot Rachel!

	Marat Tanalin
	Comment 55 • 6 years ago

(In reply to Rachel Andrew from comment #53)
> I'm appreciate a review of those docs, please let me know if you see any
> problem or think there is something else we should cover here.

It makes sense to note in docs that the new syntax for ranges is not just a new syntax or syntax sugar for an existing feature, but a _new feature_.

It allows what was _not possible_ at all with the previously available `min-`/`max-` syntax.

For example:

	@media (width < 600px) {}

means lower than 600px, _not including_. So we can now finally have _contiguous_ ranges with _no gaps_ and _no intersections_ at the same time:

	@media (width < 600px) {
		BODY {background: green; }
	}
	/* There are no no-styling gaps in between, ranges are 100% gapless. */
	@media (width >= 600px) {
		BODY {background: blue; }
	}

Previously we were forced to use _nested_ ranges to prevent gaps, so it was then necessary to _unset_ some inherited styles unneeded in subsequent ranges.