From a0998d30d4f9c0c3c25a96e807e28d1bffea48f8 Mon Sep 17 00:00:00 2001 From: Michael Nelson Date: Wed, 22 Feb 2017 20:58:33 +1100 Subject: [PATCH 1/2] Update PropertyDeclarationBlock::to_css so that the iteration of possible shorthands matches the linked spec. Previously substep 5 attempted to serialize the complete shorthand declaration and substep 6 skipped to the next shorthand only if the current shorthand was not serialized, but this did not catch empty serializations. The spec on the other hand specifically says that the *value* should be evaluated first and if the value is empty substep 6 should skip to the next shorthand - which is what happens now. To do this required some refactoring which mostly simplifies the code. Specifically: - append_declaration_value was refactored so that importance is not required as a arg (by moving it to the end of append_serialization) and is_overflow_with_name was removed as an arg also (initially I refactored it elsewhere, but it turns out it's no longer required at all - more below). With these changes, append_declaration_value can be used within the algorithm for to_css to obtain just the value for substep 5. - Substeps 7 and 8 of the algorithm become explicit (they were implicit before) by passing the value, shorthand and importance to append_serialization. - serialize_shorthand_to_buffer is no longer required (as the algorithm serializes the value first instead, as per the spec. A surprising result of this was that I could also remove a lot of code handling the special case of the overflow properties serialization. This is because the overflow's LonghandToCss implementation of to_css_declared already does the right thing according to the spec - it writes the single value if both overflow-x and -y are equal, and writes nothing otherwise - so that the algorithm now skips that shorthand instead rendering the longhands. --- .../style/properties/declaration_block.rs | 68 +++++++++---------- .../style/properties/properties.mako.rs | 57 ++-------------- .../style/properties/shorthand/box.mako.rs | 36 ---------- .../properties/shorthand/serialize.mako.rs | 16 +---- 4 files changed, 36 insertions(+), 141 deletions(-) diff --git a/components/style/properties/declaration_block.rs b/components/style/properties/declaration_block.rs index f9681e68361..7f28235917b 100644 --- a/components/style/properties/declaration_block.rs +++ b/components/style/properties/declaration_block.rs @@ -128,10 +128,9 @@ impl PropertyDeclarationBlock { // Step 1.2.3 // We don't print !important when serializing individual properties, // so we treat this as a normal-importance property - let importance = Importance::Normal; match shorthand.get_shorthand_appendable_value(list) { Some(appendable_value) => - append_declaration_value(dest, appendable_value, importance, false), + append_declaration_value(dest, appendable_value), None => return Ok(()), } } @@ -343,23 +342,28 @@ impl ToCss for PropertyDeclarationBlock { Importance::Normal }; - // Substep 5 - let was_serialized = - try!( - shorthand.serialize_shorthand_to_buffer( - dest, - current_longhands.iter().cloned(), - &mut is_first_serialization, - importance - ) - ); - // If serialization occured, Substep 7 & 8 will have been completed + // Substep 5 - Let value be the result of invoking serialize a CSS + // value of current longhands. + let mut value = String::from(""); + match shorthand.get_shorthand_appendable_value(current_longhands.iter().cloned()) { + None => continue, + Some(appendable_value) => { + // Also demo test of overflow with important - I think it'll not + // be correct. + try!(append_declaration_value(&mut value, appendable_value)); + } + } // Substep 6 - if !was_serialized { - continue; + if value == "" { + continue } + // Substeps 7 and 8 + try!(append_serialization::>, _>( + dest, &shorthand, AppendableValue::Css(&value), importance, + &mut is_first_serialization)); + for current_longhand in current_longhands { // Substep 9 already_serialized.push(current_longhand.id()); @@ -434,31 +438,22 @@ fn handle_first_serialization(dest: &mut W, /// Append a given kind of appendable value to a serialization. pub fn append_declaration_value<'a, W, I>(dest: &mut W, - appendable_value: AppendableValue<'a, I>, - importance: Importance, - is_overflow_with_name: bool) + appendable_value: AppendableValue<'a, I>) -> fmt::Result where W: fmt::Write, I: Iterator, { match appendable_value { + // Should be able to use this enum to pass the already serialized css? AppendableValue::Css(css) => { try!(write!(dest, "{}", css)) }, AppendableValue::Declaration(decl) => { try!(decl.to_css(dest)); - }, - AppendableValue::DeclarationsForShorthand(shorthand, decls) => { - if is_overflow_with_name { - try!(shorthand.overflow_longhands_to_css(decls, dest)); - } else { - try!(shorthand.longhands_to_css(decls, dest)); - } - } - } - - if importance.important() { - try!(write!(dest, " !important")); + }, + AppendableValue::DeclarationsForShorthand(shorthand, decls) => { + try!(shorthand.longhands_to_css(decls, dest)); + } } Ok(()) @@ -477,12 +472,6 @@ pub fn append_serialization<'a, W, I, N>(dest: &mut W, { try!(handle_first_serialization(dest, is_first_serialization)); - // Overflow does not behave like a normal shorthand. When overflow-x and overflow-y are not of equal - // values, they no longer use the shared property name "overflow" and must be handled differently - if shorthands::is_overflow_shorthand(&appendable_value) { - return append_declaration_value(dest, appendable_value, importance, true); - } - try!(property_name.to_css(dest)); try!(dest.write_char(':')); @@ -500,7 +489,12 @@ pub fn append_serialization<'a, W, I, N>(dest: &mut W, &AppendableValue::DeclarationsForShorthand(..) => try!(write!(dest, " ")) } - try!(append_declaration_value(dest, appendable_value, importance, false)); + try!(append_declaration_value(dest, appendable_value)); + + if importance.important() { + try!(write!(dest, " !important")); + } + write!(dest, ";") } diff --git a/components/style/properties/properties.mako.rs b/components/style/properties/properties.mako.rs index e0623fcf780..51c1b8d8432 100644 --- a/components/style/properties/properties.mako.rs +++ b/components/style/properties/properties.mako.rs @@ -13,7 +13,7 @@ use std::borrow::Cow; use std::boxed::Box as StdBox; use std::collections::HashSet; -use std::fmt::{self, Write}; +use std::fmt; use std::sync::Arc; use app_units::Au; @@ -549,59 +549,10 @@ impl ShorthandId { } } - /// Overflow does not behave like a normal shorthand. When overflow-x and - /// overflow-y are not of equal values, they no longer use the shared - /// property name "overflow". + /// Find an return an appendable value for the given declarations. /// - /// We use this function as a special-case for that. - pub fn overflow_longhands_to_css<'a, W, I>(&self, - declarations: I, - dest: &mut W) - -> fmt::Result - where W: fmt::Write, - I: Iterator, - { - match *self { - ShorthandId::Overflow => { - match shorthands::overflow::LonghandsToSerialize::from_iter(declarations) { - Ok(longhands) => longhands.to_css_declared_with_name(dest), - Err(_) => Err(fmt::Error) - } - }, - _ => Err(fmt::Error) - } - } - - /// Serializes the possible shorthand name with value to input buffer given - /// a list of longhand declarations. - /// - /// On success, returns true if the shorthand value is written, or false if - /// no shorthand value is present. - pub fn serialize_shorthand_to_buffer<'a, W, I>(self, - dest: &mut W, - declarations: I, - is_first_serialization: &mut bool, - importance: Importance) - -> Result - where W: Write, - I: IntoIterator, - I::IntoIter: Clone, - { - match self.get_shorthand_appendable_value(declarations) { - None => Ok(false), - Some(appendable_value) => { - append_serialization( - dest, - &self, - appendable_value, - importance, - is_first_serialization - ).and_then(|_| Ok(true)) - } - } - } - - fn get_shorthand_appendable_value<'a, I>(self, + /// Returns the optional appendable value. + pub fn get_shorthand_appendable_value<'a, I>(self, declarations: I) -> Option> where I: IntoIterator, diff --git a/components/style/properties/shorthand/box.mako.rs b/components/style/properties/shorthand/box.mako.rs index 1120c8eac5a..9f23b743a27 100644 --- a/components/style/properties/shorthand/box.mako.rs +++ b/components/style/properties/shorthand/box.mako.rs @@ -34,42 +34,6 @@ } Ok(()) } - - // Overflow does not behave like a normal shorthand. When overflow-x and overflow-y are not of equal - // values, they no longer use the shared property name "overflow". - // Other shorthands do not include their name in the to_css method - pub fn to_css_declared_with_name(&self, dest: &mut W) -> fmt::Result where W: fmt::Write { - let x_and_y_equal = match (self.overflow_x, self.overflow_y) { - (&DeclaredValue::Value(ref x_value), &DeclaredValue::Value(ref y_container)) => { - *x_value == y_container.0 - }, - (_, &DeclaredValue::WithVariables(_)) | - (&DeclaredValue::WithVariables(_), _) => { - // We don't serialize shorthands with variables - return dest.write_str(""); - }, - (&DeclaredValue::Initial, &DeclaredValue::Initial) => true, - (&DeclaredValue::Inherit, &DeclaredValue::Inherit) => true, - (&DeclaredValue::Unset, &DeclaredValue::Unset) => true, - _ => false - }; - - if x_and_y_equal { - try!(write!(dest, "overflow")); - try!(write!(dest, ": ")); - try!(self.overflow_x.to_css(dest)); - } else { - try!(write!(dest, "overflow-x")); - try!(write!(dest, ": ")); - try!(self.overflow_x.to_css(dest)); - try!(write!(dest, "; ")); - - try!(write!(dest, "overflow-y: ")); - try!(self.overflow_y.to_css(dest)); - } - - write!(dest, ";") - } } diff --git a/components/style/properties/shorthand/serialize.mako.rs b/components/style/properties/shorthand/serialize.mako.rs index 467542356c6..017e77586eb 100644 --- a/components/style/properties/shorthand/serialize.mako.rs +++ b/components/style/properties/shorthand/serialize.mako.rs @@ -2,7 +2,7 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ -use properties::{AppendableValue, DeclaredValue, PropertyDeclaration, ShorthandId}; +use properties::DeclaredValue; use style_traits::ToCss; use values::specified::{BorderStyle, CSSColor}; use std::fmt; @@ -91,17 +91,3 @@ fn serialize_directional_border(dest: &mut W, _ => Ok(()) } } - - -#[allow(missing_docs)] -pub fn is_overflow_shorthand<'a, I>(appendable_value: &AppendableValue<'a, I>) -> bool - where I: Iterator -{ - if let AppendableValue::DeclarationsForShorthand(shorthand, _) = *appendable_value { - if let ShorthandId::Overflow = shorthand { - return true; - } - } - - false -} From bcafe21dc9998c75f39e6a37f04395acd462b0fa Mon Sep 17 00:00:00 2001 From: Michael Nelson Date: Wed, 22 Feb 2017 21:43:35 +1100 Subject: [PATCH 2/2] Animation shorthand should be serialized only when the value lists have the same length. Fixes #15398 The previous commit did most of the work here of updating the algorithm to skip shorthands if a shorthand value was an empty string. This commit just updates animations LonghandToSerialize's implementation of to_css_declared to write an empty string value if the list lengths differ (and updates the test to match). --- .../style/properties/declaration_block.rs | 18 +++++----- .../style/properties/properties.mako.rs | 2 +- .../style/properties/shorthand/box.mako.rs | 32 ++++++++--------- tests/unit/style/properties/serialization.rs | 34 +++++++++---------- 4 files changed, 40 insertions(+), 46 deletions(-) diff --git a/components/style/properties/declaration_block.rs b/components/style/properties/declaration_block.rs index 7f28235917b..bde41fc6d8e 100644 --- a/components/style/properties/declaration_block.rs +++ b/components/style/properties/declaration_block.rs @@ -344,23 +344,21 @@ impl ToCss for PropertyDeclarationBlock { // Substep 5 - Let value be the result of invoking serialize a CSS // value of current longhands. - let mut value = String::from(""); + let mut value = String::new(); match shorthand.get_shorthand_appendable_value(current_longhands.iter().cloned()) { None => continue, Some(appendable_value) => { - // Also demo test of overflow with important - I think it'll not - // be correct. try!(append_declaration_value(&mut value, appendable_value)); } } // Substep 6 - if value == "" { + if value.is_empty() { continue } // Substeps 7 and 8 - try!(append_serialization::>, _>( + try!(append_serialization::>, _>( dest, &shorthand, AppendableValue::Css(&value), importance, &mut is_first_serialization)); @@ -417,7 +415,8 @@ pub enum AppendableValue<'a, I> /// FIXME: This needs more docs, where are the shorthands expanded? We print /// the property name before-hand, don't we? DeclarationsForShorthand(ShorthandId, I), - /// A raw CSS string, coming for example from a property with CSS variables. + /// A raw CSS string, coming for example from a property with CSS variables, + /// or when storing a serialized shorthand value before appending directly. Css(&'a str) } @@ -444,7 +443,6 @@ pub fn append_declaration_value<'a, W, I>(dest: &mut W, I: Iterator, { match appendable_value { - // Should be able to use this enum to pass the already serialized css? AppendableValue::Css(css) => { try!(write!(dest, "{}", css)) }, @@ -485,8 +483,10 @@ pub fn append_serialization<'a, W, I, N>(dest: &mut W, // for normal parsed values, add a space between key: and value try!(write!(dest, " ")); } - }, - &AppendableValue::DeclarationsForShorthand(..) => try!(write!(dest, " ")) + }, + // Currently append_serialization is only called with a Css or + // a Declaration AppendableValue. + &AppendableValue::DeclarationsForShorthand(..) => unreachable!() } try!(append_declaration_value(dest, appendable_value)); diff --git a/components/style/properties/properties.mako.rs b/components/style/properties/properties.mako.rs index 51c1b8d8432..c3b60b7df0e 100644 --- a/components/style/properties/properties.mako.rs +++ b/components/style/properties/properties.mako.rs @@ -549,7 +549,7 @@ impl ShorthandId { } } - /// Find an return an appendable value for the given declarations. + /// Finds and returns an appendable value for the given declarations. /// /// Returns the optional appendable value. pub fn get_shorthand_appendable_value<'a, I>(self, diff --git a/components/style/properties/shorthand/box.mako.rs b/components/style/properties/shorthand/box.mako.rs index 9f23b743a27..f2d8f6db5d9 100644 --- a/components/style/properties/shorthand/box.mako.rs +++ b/components/style/properties/shorthand/box.mako.rs @@ -279,27 +279,27 @@ macro_rules! try_parse_one { } } - // TODO: When the lengths are different, shorthand shouldn't be serialized - // at all. - use std::cmp; - let mut len = 0; - % for name in "duration timing_function delay direction fill_mode iteration_count play_state name".split(): - len = cmp::max(len, extract_value(self.animation_${name}).map(|i| i.0.len()) - .unwrap_or(0)); - % endfor - + let len = extract_value(self.animation_name).map(|i| i.0.len()).unwrap_or(0); // There should be at least one declared value if len == 0 { return dest.write_str("") } + // If any value list length is differs then we don't do a shorthand serialization + // either. + % for name in "duration timing_function delay direction fill_mode iteration_count play_state".split(): + if len != extract_value(self.animation_${name}).map(|i| i.0.len()).unwrap_or(0) { + return dest.write_str("") + } + % endfor + let mut first = true; for i in 0..len { % for name in "duration timing_function delay direction fill_mode iteration_count play_state name".split(): let ${name} = if let DeclaredValue::Value(ref arr) = *self.animation_${name} { - arr.0.get(i % arr.0.len()) + &arr.0[i] } else { - None + unreachable!() }; % endfor @@ -310,15 +310,11 @@ macro_rules! try_parse_one { } % for name in "duration timing_function delay direction fill_mode iteration_count play_state".split(): - if let Some(${name}) = ${name} { - try!(${name}.to_css(dest)); - try!(write!(dest, " ")); - } + try!(${name}.to_css(dest)); + try!(write!(dest, " ")); % endfor - if let Some(name) = name { - try!(name.to_css(dest)); - } + try!(name.to_css(dest)); } Ok(()) } diff --git a/tests/unit/style/properties/serialization.rs b/tests/unit/style/properties/serialization.rs index e54ef253059..2b0085ccf13 100644 --- a/tests/unit/style/properties/serialization.rs +++ b/tests/unit/style/properties/serialization.rs @@ -1192,27 +1192,25 @@ mod shorthand_serialization { #[test] fn serialize_multiple_animations_unequal_property_lists() { - // Currently the implementation cycles values if the lists are - // uneven. This is incorrect, in that we should serialize only - // when the lists have the same length (both here and for background - // and transition. See https://github.com/servo/servo/issues/15398 ) - let block = property_declaration_block("\ - animation-name: bounce, roll, flip, jump;\ - animation-duration: 1s, 0.2s;\ - animation-timing-function: ease-in, linear;\ - animation-delay: 0s, 1s, 0.5s;\ - animation-direction: normal;\ - animation-fill-mode: forwards, backwards;\ - animation-iteration-count: infinite, 2;\ - animation-play-state: paused, running;"); + // When the lengths of property values are different, the shorthand serialization + // should not be used. Previously the implementation cycled values if the lists were + // uneven. This is incorrect, in that we should serialize to a shorthand only when the + // lists have the same length (both here and for background and transition. See + // https://github.com/servo/servo/issues/15398 ) + let block_text = "\ + animation-name: bounce, roll, flip, jump; \ + animation-duration: 1s, 0.2s; \ + animation-timing-function: ease-in, linear; \ + animation-delay: 0s, 1s, 0.5s; \ + animation-direction: normal; \ + animation-fill-mode: forwards, backwards; \ + animation-iteration-count: infinite, 2; \ + animation-play-state: paused, running;"; + let block = property_declaration_block(block_text); let serialization = block.to_css_string(); - assert_eq!(serialization, "animation: \ - 1s ease-in 0s normal forwards infinite paused bounce, \ - 0.2s linear 1s normal backwards 2 running roll, \ - 1s ease-in 0.5s normal forwards infinite paused flip, \ - 0.2s linear 0s normal backwards 2 running jump;") + assert_eq!(serialization, block_text); } #[test]