From 4bdd7b8cc4e69f7d2c158495dda1b1c41acad267 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] Bug 1422225: Error reporting fixes. r=xidorn 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. MozReview-Commit-ID: 2tqdAsWh6Kh --- .../en-US/chrome/layout/css.properties | 2 + .../test/test_css_parse_error_smoketest.html | 3 +- servo/components/style/gecko/media_queries.rs | 31 ++++---------- .../style/media_queries/media_condition.rs | 9 ++--- .../style/media_queries/media_query.rs | 40 +++++++------------ servo/components/style_traits/lib.rs | 4 +- servo/ports/geckolib/error_reporter.rs | 12 +++--- 7 files changed, 37 insertions(+), 64 deletions(-) diff --git a/dom/locales/en-US/chrome/layout/css.properties b/dom/locales/en-US/chrome/layout/css.properties index 42542f82a28c..8019d0a491a0 100644 --- a/dom/locales/en-US/chrome/layout/css.properties +++ b/dom/locales/en-US/chrome/layout/css.properties @@ -18,6 +18,8 @@ PEUnknownAtRule=Unrecognized at-rule or error parsing at-rule ‘%1$S’. PECharsetRuleEOF=charset string in @charset rule PECharsetRuleNotString=Expected charset string but found ‘%1$S’. PEGatherMediaEOF=end of media list in @import or @media rule +PEMQUnexpectedOperator=Unexpected operator in media list. +PEMQUnexpectedToken=Unexpected token ‘%1$S’ in media list. PEGatherMediaNotComma=Expected ‘,’ in media list but found ‘%1$S’. PEGatherMediaNotIdent=Expected identifier in media list but found ‘%1$S’. PEGatherMediaReservedMediaType=Found reserved keyword ‘%1$S’ when looking for media type. diff --git a/layout/style/test/test_css_parse_error_smoketest.html b/layout/style/test/test_css_parse_error_smoketest.html index d4ffe363d70a..19f42f7b5f66 100644 --- a/layout/style/test/test_css_parse_error_smoketest.html +++ b/layout/style/test/test_css_parse_error_smoketest.html @@ -41,8 +41,9 @@ { css: "* { -webkit-text-size-adjust: 100% }", error: "Error in parsing value for ‘-webkit-text-size-adjust’. Declaration dropped." }, { css: "@media (totally-unknown-feature) {}", error: "Expected media feature name but found ‘totally-unknown-feature’." }, - { css: "@media \"foo\" {}", error: "Expected identifier in media list but found ‘\"foo\"’." }, + { css: "@media \"foo\" {}", error: "Unexpected token ‘\"foo\"’ in media list." }, { css: "@media (min-width) {}", error: "Media features with min- or max- must have a value." }, + { css: "@media (min-width >= 3px) {}", error: "Unexpected operator in media list." }, { css: "@media (device-height: -1px) {}", error: "Found invalid value for media feature." }, { css: "@media (min-width: -1px) {}", error: "Found invalid value for media feature." }, { css: "@media (min-resolution: 2) {}", error: "Found invalid value for media feature." }, diff --git a/servo/components/style/gecko/media_queries.rs b/servo/components/style/gecko/media_queries.rs index a6322842f200..9c5ef4d71a11 100644 --- a/servo/components/style/gecko/media_queries.rs +++ b/servo/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/servo/components/style/media_queries/media_condition.rs b/servo/components/style/media_queries/media_condition.rs index c2c29c129ca3..e56f02ff0712 100644 --- a/servo/components/style/media_queries/media_condition.rs +++ b/servo/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/servo/components/style/media_queries/media_query.rs b/servo/components/style/media_queries/media_query.rs index a2f4f9bea706..089fc9412b24 100644 --- a/servo/components/style/media_queries/media_query.rs +++ b/servo/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/servo/components/style_traits/lib.rs b/servo/components/style_traits/lib.rs index b1eaf4e086fe..737450b5e059 100644 --- a/servo/components/style_traits/lib.rs +++ b/servo/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. diff --git a/servo/ports/geckolib/error_reporter.rs b/servo/ports/geckolib/error_reporter.rs index 6afd88972a32..45442822a4eb 100644 --- a/servo/ports/geckolib/error_reporter.rs +++ b/servo/ports/geckolib/error_reporter.rs @@ -145,9 +145,7 @@ fn extract_error_params<'a>(err: ErrorKind<'a>) -> Option> { (Some(ErrorString::Ident(ident)), None) } - ParseErrorKind::Custom( - StyleParseErrorKind::ExpectedIdentifier(token) - ) | + ParseErrorKind::Basic(BasicParseErrorKind::UnexpectedToken(token)) | ParseErrorKind::Custom( StyleParseErrorKind::ValueError(ValueParseErrorKind::InvalidColor(token)) ) => { @@ -342,20 +340,20 @@ impl<'a> ErrorHelpers<'a> for ContextualParseError<'a> { } ContextualParseError::InvalidMediaRule(_, ref err) => { let err: &CStr = match err.kind { - ParseErrorKind::Custom(StyleParseErrorKind::ExpectedIdentifier(..)) => { - cstr!("PEGatherMediaNotIdent") - }, ParseErrorKind::Custom(StyleParseErrorKind::MediaQueryExpectedFeatureName(..)) => { cstr!("PEMQExpectedFeatureName") }, ParseErrorKind::Custom(StyleParseErrorKind::MediaQueryExpectedFeatureValue) => { cstr!("PEMQExpectedFeatureValue") }, + ParseErrorKind::Custom(StyleParseErrorKind::MediaQueryUnexpectedOperator) => { + cstr!("PEMQUnexpectedOperator") + }, ParseErrorKind::Custom(StyleParseErrorKind::RangedExpressionWithNoValue) => { cstr!("PEMQNoMinMaxWithoutValue") }, _ => { - cstr!("PEDeclDropped") + cstr!("PEMQUnexpectedToken") }, }; (err, Action::Nothing)