From 97ef40ee88b19ab1a85448cd83581da92f8a2618 Mon Sep 17 00:00:00 2001 From: Jeremy Chen Date: Fri, 9 Jun 2017 03:38:23 +0000 Subject: [PATCH 1/2] stylo: make font-variant-* longhands parsing more tolerant. Avoid early returning Err() in parsers, so we could let the the caller of the parsers to handle the rest of input, and return error if it requires parsing entirely. The point is let returning Err() stay inside input.try(), so we can count on input.try() to restore the position when parsing invalid idents. From gecko bug: Bug 1356134 (https://bugzilla.mozilla.org/show_bug.cgi?id=1356134) --- .../style/properties/longhand/font.mako.rs | 75 ++++++++----------- 1 file changed, 33 insertions(+), 42 deletions(-) diff --git a/components/style/properties/longhand/font.mako.rs b/components/style/properties/longhand/font.mako.rs index ed0da5f44c7..023caef6469 100644 --- a/components/style/properties/longhand/font.mako.rs +++ b/components/style/properties/longhand/font.mako.rs @@ -1376,21 +1376,21 @@ ${helpers.single_keyword_system("font-kerning", return Ok(SpecifiedValue::Value(result)) } - while let Ok(ident) = input.try(|input| input.expect_ident()) { - let flag = match_ignore_ascii_case! { &ident, - "stylistic" => Some(STYLISTIC), - "historical-forms" => Some(HISTORICAL_FORMS), - "styleset" => Some(STYLESET), - "character-variant" => Some(CHARACTER_VARIANT), - "swash" => Some(SWASH), - "ornaments" => Some(ORNAMENTS), - "annotation" => Some(ANNOTATION), - _ => None, - }; - let flag = match flag { - Some(flag) if !result.intersects(flag) => flag, - _ => return Err(SelectorParseError::UnexpectedIdent(ident).into()), - }; + while let Ok(flag) = input.try(|input| { + Ok(match_ignore_ascii_case! { &input.expect_ident().map_err(|_| ())?, + "stylistic" => STYLISTIC, + "historical-forms" => HISTORICAL_FORMS, + "styleset" => STYLESET, + "character-variant" => CHARACTER_VARIANT, + "swash" => SWASH, + "ornaments" => ORNAMENTS, + "annotation" => ANNOTATION, + _ => return Err(()), + }) + }) { + if result.intersects(flag) { + return Err(StyleParseError::UnspecifiedError.into()) + } result.insert(flag); } @@ -1406,9 +1406,9 @@ ${helpers.single_keyword_system("font-kerning", macro_rules! exclusive_value { (($value:ident, $set:expr) => $ident:ident) => { if $value.intersects($set) { - None + return Err(()) } else { - Some($ident) + $ident } } } @@ -1521,8 +1521,8 @@ macro_rules! exclusive_value { return Ok(SpecifiedValue::Value(result)) } - while let Ok(ident) = input.try(|input| input.expect_ident()) { - let flag = match_ignore_ascii_case! { &ident, + while let Ok(flag) = input.try(|input| { + Ok(match_ignore_ascii_case! { &input.expect_ident().map_err(|_| ())?, "jis78" => exclusive_value!((result, ${east_asian_variant_values}) => JIS78), "jis83" => @@ -1541,12 +1541,9 @@ macro_rules! exclusive_value { exclusive_value!((result, ${east_asian_width_values}) => PROPORTIONAL_WIDTH), "ruby" => exclusive_value!((result, RUBY) => RUBY), - _ => None, - }; - let flag = match flag { - Some(flag) => flag, - None => return Err(SelectorParseError::UnexpectedIdent(ident).into()), - }; + _ => return Err(()), + }) + }) { result.insert(flag); } @@ -1681,8 +1678,8 @@ macro_rules! exclusive_value { return Ok(SpecifiedValue::Value(NONE)) } - while let Ok(ident) = input.try(|input| input.expect_ident()) { - let flag = match_ignore_ascii_case! { &ident, + while let Ok(flag) = input.try(|input| { + Ok(match_ignore_ascii_case! { &input.expect_ident().map_err(|_| ())?, "common-ligatures" => exclusive_value!((result, ${common_lig_values}) => COMMON_LIGATURES), "no-common-ligatures" => @@ -1699,12 +1696,9 @@ macro_rules! exclusive_value { exclusive_value!((result, ${contextual_alt_values}) => CONTEXTUAL), "no-contextual" => exclusive_value!((result, ${contextual_alt_values}) => NO_CONTEXTUAL), - _ => None, - }; - let flag = match flag { - Some(flag) => flag, - None => return Err(SelectorParseError::UnexpectedIdent(ident).into()), - }; + _ => return Err(()), + }) + }) { result.insert(flag); } @@ -1832,14 +1826,14 @@ macro_rules! exclusive_value { return Ok(SpecifiedValue::Value(result)) } - while let Ok(ident) = input.try(|input| input.expect_ident()) { - let flag = match_ignore_ascii_case! { &ident, + while let Ok(flag) = input.try(|input| { + Ok(match_ignore_ascii_case! { &input.expect_ident().map_err(|_| ())?, "ordinal" => exclusive_value!((result, ORDINAL) => ORDINAL), "slashed-zero" => exclusive_value!((result, SLASHED_ZERO) => SLASHED_ZERO), "lining-nums" => - exclusive_value!((result, ${numeric_figure_values}) => LINING_NUMS ), + exclusive_value!((result, ${numeric_figure_values}) => LINING_NUMS), "oldstyle-nums" => exclusive_value!((result, ${numeric_figure_values}) => OLDSTYLE_NUMS), "proportional-nums" => @@ -1850,12 +1844,9 @@ macro_rules! exclusive_value { exclusive_value!((result, ${numeric_fraction_values}) => DIAGONAL_FRACTIONS), "stacked-fractions" => exclusive_value!((result, ${numeric_fraction_values}) => STACKED_FRACTIONS), - _ => None, - }; - let flag = match flag { - Some(flag) => flag, - None => return Err(SelectorParseError::UnexpectedIdent(ident).into()), - }; + _ => return Err(()), + }) + }) { result.insert(flag); } From c7c753867b18dbf66108b52dd9791b23e85f0a09 Mon Sep 17 00:00:00 2001 From: Jeremy Chen Date: Mon, 5 Jun 2017 13:46:22 +0000 Subject: [PATCH 2/2] stylo: support font-variant shorthand. We still count on Bug 1356124 to fix font-variant-alternates longhand, so we can get font-variant shorthand work properly. Some comments about Bug 1356124 have been removed, since this shorthand code should just work once we fix Bug 1356124. From gecko bug: Bug 1356134 (https://bugzilla.mozilla.org/show_bug.cgi?id=1356134) --- .../style/properties/longhand/font.mako.rs | 4 + .../style/properties/shorthand/font.mako.rs | 125 ++++++++++++------ 2 files changed, 86 insertions(+), 43 deletions(-) diff --git a/components/style/properties/longhand/font.mako.rs b/components/style/properties/longhand/font.mako.rs index 023caef6469..be7a8d1e168 100644 --- a/components/style/properties/longhand/font.mako.rs +++ b/components/style/properties/longhand/font.mako.rs @@ -1653,6 +1653,10 @@ macro_rules! exclusive_value { pub fn get_initial_specified_value() -> SpecifiedValue { SpecifiedValue::Value(VariantLigatures::empty()) } + #[inline] + pub fn get_none_specified_value() -> SpecifiedValue { + SpecifiedValue::Value(NONE) + } /// normal | none | /// [ || diff --git a/components/style/properties/shorthand/font.mako.rs b/components/style/properties/shorthand/font.mako.rs index 009282a1b91..993f7ffd752 100644 --- a/components/style/properties/shorthand/font.mako.rs +++ b/components/style/properties/shorthand/font.mako.rs @@ -232,66 +232,105 @@ ${'font-variant-numeric' if product == 'gecko' or data.testing else ''} ${'font-variant-position' if product == 'gecko' or data.testing else ''}" spec="https://drafts.csswg.org/css-fonts-3/#propdef-font-variant"> - use properties::longhands::font_variant_caps; <% gecko_sub_properties = "alternates east_asian ligatures numeric position".split() %> - % if product == "gecko" or data.testing: - % for prop in gecko_sub_properties: - use properties::longhands::font_variant_${prop}; - % endfor - % endif + <% + sub_properties = ["caps"] + if product == "gecko" or data.testing: + sub_properties += gecko_sub_properties + %> + +% for prop in sub_properties: + use properties::longhands::font_variant_${prop}; +% endfor pub fn parse_value<'i, 't>(context: &ParserContext, input: &mut Parser<'i, 't>) -> Result> { - let mut nb_normals = 0; - let mut caps = None; - loop { - // Special-case 'normal' because it is valid in each of - // all sub properties. - // Leaves the values to None, 'normal' is the initial value for each of them. - if input.try(|input| input.expect_ident_matching("normal")).is_ok() { - nb_normals += 1; - continue; - } - if caps.is_none() { - if let Ok(value) = input.try(|input| font_variant_caps::parse(context, input)) { - caps = Some(value); - continue + % for prop in sub_properties: + let mut ${prop} = None; + % endfor + + if input.try(|input| input.expect_ident_matching("normal")).is_ok() { + // Leave the values to None, 'normal' is the initial value for all the sub properties. + } else if input.try(|input| input.expect_ident_matching("none")).is_ok() { + // The 'none' value sets 'font-variant-ligatures' to 'none' and resets all other sub properties + // to their initial value. + % if product == "gecko" or data.testing: + ligatures = Some(font_variant_ligatures::get_none_specified_value()); + % endif + } else { + let mut has_custom_value: bool = false; + loop { + if input.try(|input| input.expect_ident_matching("normal")).is_ok() || + input.try(|input| input.expect_ident_matching("none")).is_ok() { + return Err(StyleParseError::UnspecifiedError.into()) } + % for prop in sub_properties: + if ${prop}.is_none() { + if let Ok(value) = input.try(|i| font_variant_${prop}::parse(context, i)) { + has_custom_value = true; + ${prop} = Some(value); + continue + } + } + % endfor + + break + } + + if !has_custom_value { + return Err(StyleParseError::UnspecifiedError.into()) } - break - } - #[inline] - fn count(opt: &Option) -> u8 { - if opt.is_some() { 1 } else { 0 } - } - let count = count(&caps) + nb_normals; - if count == 0 || count > 1 { - return Err(StyleParseError::UnspecifiedError.into()) } + Ok(expanded! { - font_variant_caps: unwrap_or_initial!(font_variant_caps, caps), - // FIXME: Bug 1356134 - parse all sub properties. - % if product == "gecko" or data.testing: - % for name in gecko_sub_properties: - font_variant_${name}: font_variant_${name}::get_initial_specified_value(), - % endfor - % endif + % for prop in sub_properties: + font_variant_${prop}: unwrap_or_initial!(font_variant_${prop}, ${prop}), + % endfor }) } impl<'a> ToCss for LonghandsToSerialize<'a> { + #[allow(unused_assignments)] fn to_css(&self, dest: &mut W) -> fmt::Result where W: fmt::Write { - % if product == "gecko" or data.testing: - % for name in gecko_sub_properties: - // FIXME: Bug 1356134 - handle all sub properties. - if self.font_variant_${name} != &font_variant_${name}::get_initial_specified_value() { - return Ok(()); + let has_none_ligatures = + % if product == "gecko" or data.testing: + self.font_variant_ligatures == &font_variant_ligatures::get_none_specified_value(); + % else: + false; + % endif + + const TOTAL_SUBPROPS: usize = ${len(sub_properties)}; + let mut nb_normals = 0; + % for prop in sub_properties: + if self.font_variant_${prop} == &font_variant_${prop}::get_initial_specified_value() { + nb_normals += 1; } % endfor - % endif - self.font_variant_caps.to_css(dest)?; + + if nb_normals > 0 && nb_normals == TOTAL_SUBPROPS { + dest.write_str("normal")?; + } else if has_none_ligatures { + if nb_normals == TOTAL_SUBPROPS - 1 { + // Serialize to 'none' if 'font-variant-ligatures' is set to 'none' and all other + // font feature properties are reset to their initial value. + dest.write_str("none")?; + } else { + return Ok(()) + } + } else { + let mut has_any = false; + % for prop in sub_properties: + if self.font_variant_${prop} != &font_variant_${prop}::get_initial_specified_value() { + if has_any { + dest.write_str(" ")?; + } + has_any = true; + self.font_variant_${prop}.to_css(dest)?; + } + % endfor + } Ok(()) }