Closed Bug 1422225 Opened 7 years ago Closed 6 years ago

Implement syntax improvements for media queries from level 4 specification

Categories

(Core :: CSS Parsing and Computation, enhancement, P3)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: florian, Assigned: emilio)

References

(Blocks 2 open bugs)

Details

(Keywords: dev-doc-complete)

Attachments

(7 files)

Bug 1422225: Allow parsing operators in media feature expressions.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1422225: Rename Expression to MediaFeatureExpression.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1422225: Add code to parse media conditions.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1422225: Add serialization code for MediaCondition.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1422225: Evaluate MediaConditions, and glue it all together.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1422225: Test updates.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1422225: Error reporting fixes.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
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) {   … }
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P1
Priority: P1 → P3
Flags: needinfo?(emilio)
Depends on: 1468846
Assignee: nobody → emilio
Flags: needinfo?(emilio)
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.
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...
We probably shouldn't land this before merge day.
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #11) > We probably shouldn't land this before merge day. Agreed.
> 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.
(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
Depends on: 1469173
Depends on: 1469174
Blocks: 1469174
No longer depends on: 1469174
Blocks: 1469173
No longer depends on: 1469173
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.
Attachment #8985758 - Flags: review?(xidorn+moz) → 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`...
Attachment #8985759 - Flags: review?(xidorn+moz) → 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".
Attachment #8985760 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8985761 [details] Bug 1422225: Add serialization code for MediaCondition. https://reviewboard.mozilla.org/r/251298/#review258122
Attachment #8985761 - Flags: review?(xidorn+moz) → 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`.
Attachment #8985762 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8985763 [details] Bug 1422225: Test updates. https://reviewboard.mozilla.org/r/251302/#review258212
Attachment #8985763 - Flags: review?(xidorn+moz) → review+
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...
Depends on: 1469957
I did find a less intrusive way for now, will consider changing it later, probably when implementing <general-enclosed>.
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.
Attachment #8986843 - Flags: review?(xidorn+moz)
(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.
Flags: needinfo?(xidorn+moz)
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.
Flags: needinfo?(xidorn+moz)
Ah, I see, makes sense.
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.
Attachment #8986843 - Flags: review?(xidorn+moz) → review+
(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 :).
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.
(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! :)
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
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/11664 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
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.
Flags: needinfo?(emilio)
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!
Flags: needinfo?(emilio)
(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.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: