From 2d2e84aad59329f6d46b1734c8eefe0a450b4a96 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Thu, 21 Jun 2018 19:35:35 +0200 Subject: [PATCH] style: Error reporting fixes for media queries. Do it so that we always try to evaluate the media expression and the modern syntax last, so that the most specific error message comes up. Bug: 1422225 Reviewed-by: xidorn MozReview-Commit-ID: 2tqdAsWh6Kh --- components/style/gecko/media_queries.rs | 31 ++++---------- .../style/media_queries/media_condition.rs | 9 ++--- components/style/media_queries/media_query.rs | 40 +++++++------------ components/style_traits/lib.rs | 4 +- 4 files changed, 28 insertions(+), 56 deletions(-) diff --git a/components/style/gecko/media_queries.rs b/components/style/gecko/media_queries.rs index a6322842f20..9c5ef4d71a1 100644 --- a/components/style/gecko/media_queries.rs +++ b/components/style/gecko/media_queries.rs @@ -7,7 +7,7 @@ use app_units::AU_PER_PX; use app_units::Au; use context::QuirksMode; -use cssparser::{BasicParseErrorKind, Parser, RGBA, Token}; +use cssparser::{Parser, RGBA, Token}; use euclid::Size2D; use euclid::TypedScale; use gecko::values::{convert_nscolor_to_rgba, convert_rgba_to_nscolor}; @@ -292,8 +292,8 @@ enum RangeOrOperator { Operator(Operator), } -/// A range expression for gecko contains a reference to the media feature, the -/// value the media query contained, and the range to evaluate. +/// A feature expression for gecko contains a reference to the media feature, +/// the value the media query contained, and the range to evaluate. #[derive(Clone, Debug, MallocSizeOf)] pub struct MediaFeatureExpression { feature: &'static nsMediaFeature, @@ -641,21 +641,13 @@ impl MediaFeatureExpression { context: &ParserContext, input: &mut Parser<'i, 't>, ) -> Result> { - input.expect_parenthesis_block().map_err(|err| { - err.location.new_custom_error(match err.kind { - BasicParseErrorKind::UnexpectedToken(t) => { - StyleParseErrorKind::ExpectedIdentifier(t) - }, - _ => StyleParseErrorKind::UnspecifiedError, - }) - })?; - + input.expect_parenthesis_block()?; input.parse_nested_block(|input| { Self::parse_in_parenthesis_block(context, input) }) } - /// Parse a media range expression where we've already consumed the + /// Parse a media feature expression where we've already consumed the /// parenthesis. pub fn parse_in_parenthesis_block<'i, 't>( context: &ParserContext, @@ -666,14 +658,7 @@ impl MediaFeatureExpression { let range; { let location = input.current_source_location(); - let ident = input.expect_ident().map_err(|err| { - err.location.new_custom_error(match err.kind { - BasicParseErrorKind::UnexpectedToken(t) => { - StyleParseErrorKind::ExpectedIdentifier(t) - }, - _ => StyleParseErrorKind::UnspecifiedError, - }) - })?; + let ident = input.expect_ident()?; let mut flags = 0; @@ -768,7 +753,7 @@ impl MediaFeatureExpression { Some(range) => { if operator.is_some() { return Err(input.new_custom_error( - StyleParseErrorKind::MediaQueryExpectedFeatureValue + StyleParseErrorKind::MediaQueryUnexpectedOperator )); } Some(RangeOrOperator::Range(range)) @@ -778,7 +763,7 @@ impl MediaFeatureExpression { Some(operator) => { if !feature_allows_ranges { return Err(input.new_custom_error( - StyleParseErrorKind::MediaQueryExpectedFeatureValue + StyleParseErrorKind::MediaQueryUnexpectedOperator )); } Some(RangeOrOperator::Operator(operator)) diff --git a/components/style/media_queries/media_condition.rs b/components/style/media_queries/media_condition.rs index c2c29c129ca..e56f02ff071 100644 --- a/components/style/media_queries/media_condition.rs +++ b/components/style/media_queries/media_condition.rs @@ -163,12 +163,11 @@ impl MediaCondition { ) -> Result> { input.parse_nested_block(|input| { // Base case. - if let Ok(expr) = input.try(|i| MediaFeatureExpression::parse_in_parenthesis_block(context, i)) { - return Ok(MediaCondition::Feature(expr)); + if let Ok(inner) = input.try(|i| Self::parse(context, i)) { + return Ok(MediaCondition::InParens(Box::new(inner))) } - - let inner = Self::parse(context, input)?; - Ok(MediaCondition::InParens(Box::new(inner))) + let expr = MediaFeatureExpression::parse_in_parenthesis_block(context, input)?; + Ok(MediaCondition::Feature(expr)) }) } diff --git a/components/style/media_queries/media_query.rs b/components/style/media_queries/media_query.rs index a2f4f9bea70..089fc9412b2 100644 --- a/components/style/media_queries/media_query.rs +++ b/components/style/media_queries/media_query.rs @@ -9,7 +9,6 @@ use Atom; use cssparser::Parser; use parser::ParserContext; -use selectors::parser::SelectorParseErrorKind; use std::fmt::{self, Write}; use str::string_as_ascii_lowercase; use style_traits::{CssWriter, ParseError, ToCss}; @@ -125,34 +124,23 @@ impl MediaQuery { pub fn parse<'i, 't>( context: &ParserContext, input: &mut Parser<'i, 't>, - ) -> Result> { - if let Ok(condition) = input.try(|i| MediaCondition::parse(context, i)) { - return Ok(Self { - qualifier: None, - media_type: MediaQueryType::All, - condition: Some(condition), - }) - } + ) -> Result> { + let (qualifier, explicit_media_type) = input.try(|input| -> Result<_, ()> { + let qualifier = input.try(Qualifier::parse).ok(); + let ident = input.expect_ident().map_err(|_| ())?; + let media_type = MediaQueryType::parse(&ident)?; + Ok((qualifier, Some(media_type))) + }).unwrap_or_default(); - let qualifier = input.try(Qualifier::parse).ok(); - let media_type = { - let location = input.current_source_location(); - let ident = input.expect_ident()?; - match MediaQueryType::parse(&ident) { - Ok(t) => t, - Err(..) => return 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 }; - let condition = - if input.try(|i| i.expect_ident_matching("and")).is_ok() { - Some(MediaCondition::parse_disallow_or(context, input)?) - } else { - None - }; - + let media_type = explicit_media_type.unwrap_or(MediaQueryType::All); Ok(Self { qualifier, media_type, condition }) } } diff --git a/components/style_traits/lib.rs b/components/style_traits/lib.rs index b1eaf4e086f..737450b5e05 100644 --- a/components/style_traits/lib.rs +++ b/components/style_traits/lib.rs @@ -106,12 +106,12 @@ pub enum StyleParseErrorKind<'i> { PropertyDeclarationValueNotExhausted, /// An unexpected dimension token was encountered. UnexpectedDimension(CowRcStr<'i>), - /// Expected identifier not found. - ExpectedIdentifier(Token<'i>), /// Missing or invalid media feature name. MediaQueryExpectedFeatureName(CowRcStr<'i>), /// Missing or invalid media feature value. MediaQueryExpectedFeatureValue, + /// A media feature range operator was not expected. + MediaQueryUnexpectedOperator, /// min- or max- properties must have a value. RangedExpressionWithNoValue, /// A function was encountered that was not expected.