From 11469218661fe3076b255eba35c2b0736dcce500 Mon Sep 17 00:00:00 2001 From: Simon Sapin Date: Tue, 25 Apr 2017 09:14:32 +0200 Subject: [PATCH] Keep custom-ident and string separate in animation/keyframes name. --- components/script/dom/csskeyframesrule.rs | 14 +++--- components/style/animation.rs | 19 +++++--- components/style/gecko_string_cache/mod.rs | 6 +++ components/style/matching.rs | 2 +- components/style/properties/gecko.mako.rs | 16 +++++-- .../style/properties/longhand/box.mako.rs | 45 ++++++++--------- .../style/properties/properties.mako.rs | 2 +- components/style/stylesheets.rs | 16 +++---- components/style/stylist.rs | 4 +- components/style/values/mod.rs | 48 ++++++++++++++++++- tests/unit/style/lib.rs | 2 +- tests/unit/style/parsing/animation.rs | 12 ++--- tests/unit/style/stylesheets.rs | 4 +- 13 files changed, 123 insertions(+), 67 deletions(-) diff --git a/components/script/dom/csskeyframesrule.rs b/components/script/dom/csskeyframesrule.rs index 5a03dc41658..a3715e914ed 100644 --- a/components/script/dom/csskeyframesrule.rs +++ b/components/script/dom/csskeyframesrule.rs @@ -5,7 +5,7 @@ use cssparser::Parser; use dom::bindings::codegen::Bindings::CSSKeyframesRuleBinding; use dom::bindings::codegen::Bindings::CSSKeyframesRuleBinding::CSSKeyframesRuleMethods; -use dom::bindings::error::{Error, ErrorResult}; +use dom::bindings::error::ErrorResult; use dom::bindings::inheritance::Castable; use dom::bindings::js::{MutNullableJS, Root}; use dom::bindings::reflector::{DomObject, reflect_dom_object}; @@ -20,7 +20,7 @@ use std::sync::Arc; use style::keyframes::{Keyframe, KeyframeSelector}; use style::shared_lock::{Locked, ToCssWithGuard}; use style::stylesheets::KeyframesRule; -use style::values::CustomIdent; +use style::values::KeyframesName; #[dom_struct] pub struct CSSKeyframesRule { @@ -107,15 +107,15 @@ impl CSSKeyframesRuleMethods for CSSKeyframesRule { // https://drafts.csswg.org/css-animations/#dom-csskeyframesrule-name fn Name(&self) -> DOMString { let guard = self.cssrule.shared_lock().read(); - DOMString::from(&*self.keyframesrule.read_with(&guard).name.0) + DOMString::from(&**self.keyframesrule.read_with(&guard).name.as_atom()) } // https://drafts.csswg.org/css-animations/#dom-csskeyframesrule-name fn SetName(&self, value: DOMString) -> ErrorResult { - // https://github.com/w3c/csswg-drafts/issues/801 - // Setting this property to a CSS-wide keyword or `none` will - // throw a Syntax Error. - let name = CustomIdent::from_ident(value.into(), &["none"]).map_err(|()| Error::Syntax)?; + // Spec deviation: https://github.com/w3c/csswg-drafts/issues/801 + // Setting this property to a CSS-wide keyword or `none` does not throw, + // it stores a value that serializes as a quoted string. + let name = KeyframesName::from_ident(value.into()); let mut guard = self.cssrule.shared_lock().write(); self.keyframesrule.write_with(&mut guard).name = name; Ok(()) diff --git a/components/style/animation.rs b/components/style/animation.rs index 604f9ee62bf..2229d485591 100644 --- a/components/style/animation.rs +++ b/components/style/animation.rs @@ -464,14 +464,20 @@ pub fn maybe_start_animations(context: &SharedStyleContext, let box_style = new_style.get_box(); for (i, name) in box_style.animation_name_iter().enumerate() { + let name = if let Some(atom) = name.as_atom() { + atom + } else { + continue + }; + debug!("maybe_start_animations: name={}", name); let total_duration = box_style.animation_duration_mod(i).seconds(); if total_duration == 0. { continue } - if let Some(ref anim) = context.stylist.animations().get(&name.0 .0) { - debug!("maybe_start_animations: animation {} found", name.0 .0); + if let Some(ref anim) = context.stylist.animations().get(name) { + debug!("maybe_start_animations: animation {} found", name); // If this animation doesn't have any keyframe, we can just continue // without submitting it to the compositor, since both the first and @@ -506,7 +512,7 @@ pub fn maybe_start_animations(context: &SharedStyleContext, new_animations_sender - .send(Animation::Keyframes(node, name.0 .0.clone(), KeyframesAnimationState { + .send(Animation::Keyframes(node, name.clone(), KeyframesAnimationState { started_at: animation_start, duration: duration as f64, delay: delay as f64, @@ -584,9 +590,10 @@ pub fn update_style_for_animation(context: &SharedStyleContext, debug_assert!(!animation.steps.is_empty()); - let maybe_index = style.get_box() - .animation_name_iter() - .position(|animation_name| *name == animation_name.0 .0); + let maybe_index = style + .get_box() + .animation_name_iter() + .position(|animation_name| Some(name) == animation_name.as_atom()); let index = match maybe_index { Some(index) => index, diff --git a/components/style/gecko_string_cache/mod.rs b/components/style/gecko_string_cache/mod.rs index 82589b645ea..b612c027c45 100644 --- a/components/style/gecko_string_cache/mod.rs +++ b/components/style/gecko_string_cache/mod.rs @@ -163,6 +163,12 @@ impl WeakAtom { } } + /// Returns whether this atom is the empty string. + #[inline] + pub fn is_empty(&self) -> bool { + self.len() == 0 + } + /// Returns the atom as a mutable pointer. #[inline] pub fn as_ptr(&self) -> *mut nsIAtom { diff --git a/components/style/matching.rs b/components/style/matching.rs index cbaa6d12afb..201f6c50878 100644 --- a/components/style/matching.rs +++ b/components/style/matching.rs @@ -560,7 +560,7 @@ trait PrivateMatchMethods: TElement { pseudo: Option<&PseudoElement>) -> bool { let ref new_box_style = new_values.get_box(); let has_new_animation_style = new_box_style.animation_name_count() >= 1 && - new_box_style.animation_name_at(0).0 .0.len() != 0; + new_box_style.animation_name_at(0).0.is_some(); let has_animations = self.has_css_animations(pseudo); old_values.as_ref().map_or(has_new_animation_style, |ref old| { diff --git a/components/style/properties/gecko.mako.rs b/components/style/properties/gecko.mako.rs index b74bb9e8cd3..57ebd1114ff 100644 --- a/components/style/properties/gecko.mako.rs +++ b/components/style/properties/gecko.mako.rs @@ -62,7 +62,7 @@ use std::ptr; use std::sync::Arc; use std::cmp; use values::computed::ToComputedValue; -use values::{Either, Auto, CustomIdent}; +use values::{Either, Auto, KeyframesName}; use computed_values::border_style; pub mod style_structs { @@ -2202,16 +2202,22 @@ fn static_assert() { self.gecko.mAnimationNameCount = v.0.len() as u32; for (servo, gecko) in v.0.into_iter().zip(self.gecko.mAnimations.iter_mut()) { // TODO This is inefficient. We should fix this in bug 1329169. - gecko.mName.assign(servo.0 .0.as_slice()); + gecko.mName.assign(match servo.0 { + Some(ref name) => name.as_atom().as_slice(), + None => &[], // Empty string for 'none' + }); } } pub fn animation_name_at(&self, index: usize) -> longhands::animation_name::computed_value::SingleComputedValue { - use Atom; use properties::longhands::animation_name::single_value::SpecifiedValue as AnimationName; // XXX: Is there any effective ways? - AnimationName(CustomIdent(Atom::from( - String::from_utf16_lossy(&self.gecko.mAnimations[index].mName[..])))) + let atom = &self.gecko.mAnimations[index].mName; + if atom.is_empty() { + AnimationName(None) + } else { + AnimationName(Some(KeyframesName::from_ident(atom.to_string()))) + } } pub fn copy_animation_name_from(&mut self, other: &Self) { unsafe { self.gecko.mAnimations.ensure_len(other.gecko.mAnimations.len()) }; diff --git a/components/style/properties/longhand/box.mako.rs b/components/style/properties/longhand/box.mako.rs index e5aae336283..2806a04e803 100644 --- a/components/style/properties/longhand/box.mako.rs +++ b/components/style/properties/longhand/box.mako.rs @@ -796,7 +796,7 @@ ${helpers.single_keyword("overflow-x", "visible hidden scroll auto", use std::ops::Deref; use style_traits::ToCss; use values::computed::ComputedValueAsSpecified; - use values::{HasViewportPercentage, CustomIdent}; + use values::{HasViewportPercentage, KeyframesName}; pub mod computed_value { pub use super::SpecifiedValue as T; @@ -804,7 +804,14 @@ ${helpers.single_keyword("overflow-x", "visible hidden scroll auto", #[derive(Clone, Debug, Hash, Eq, PartialEq)] #[cfg_attr(feature = "servo", derive(HeapSizeOf))] - pub struct SpecifiedValue(pub CustomIdent); + pub struct SpecifiedValue(pub Option); + + impl SpecifiedValue { + /// As an Atom + pub fn as_atom(&self) -> Option< &Atom> { + self.0.as_ref().map(|n| n.as_atom()) + } + } #[inline] pub fn get_initial_value() -> computed_value::T { @@ -813,44 +820,32 @@ ${helpers.single_keyword("overflow-x", "visible hidden scroll auto", #[inline] pub fn get_initial_specified_value() -> SpecifiedValue { - SpecifiedValue(CustomIdent(atom!(""))) + SpecifiedValue(None) } impl fmt::Display for SpecifiedValue { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - self.0 .0.fmt(f) + self.to_css(f) } } impl ToCss for SpecifiedValue { fn to_css(&self, dest: &mut W) -> fmt::Result where W: fmt::Write { - if self.0 .0 == atom!("") { - dest.write_str("none") + if let Some(ref name) = self.0 { + name.to_css(dest) } else { - self.0.to_css(dest) + dest.write_str("none") } } } impl Parse for SpecifiedValue { - fn parse(_context: &ParserContext, input: &mut ::cssparser::Parser) -> Result { - use cssparser::Token; - use std::ascii::AsciiExt; - use values::CustomIdent; - - let atom = match input.next() { - Ok(Token::Ident(value)) => { - if value.eq_ignore_ascii_case("none") { - // FIXME We may want to support `@keyframes ""` at some point. - CustomIdent(atom!("")) - } else { - CustomIdent::from_ident(value, &[])? - } - } - Ok(Token::QuotedString(value)) => CustomIdent(Atom::from(value)), - _ => return Err(()), - }; - Ok(SpecifiedValue(atom)) + fn parse(context: &ParserContext, input: &mut ::cssparser::Parser) -> Result { + if let Ok(name) = input.try(|input| KeyframesName::parse(context, input)) { + Ok(SpecifiedValue(Some(name))) + } else { + input.expect_ident_matching("none").map(|()| SpecifiedValue(None)) + } } } no_viewport_percentage!(SpecifiedValue); diff --git a/components/style/properties/properties.mako.rs b/components/style/properties/properties.mako.rs index 403399acfa9..69932549fe2 100644 --- a/components/style/properties/properties.mako.rs +++ b/components/style/properties/properties.mako.rs @@ -1617,7 +1617,7 @@ pub mod style_structs { /// Returns whether there is any animation specified with /// animation-name other than `none`. pub fn specifies_animations(&self) -> bool { - self.animation_name_iter().any(|name| name.0 .0 != atom!("")) + self.animation_name_iter().any(|name| name.0.is_some()) } /// Returns whether there are any transitions specified. diff --git a/components/style/stylesheets.rs b/components/style/stylesheets.rs index bf7c10178f0..5460dba620b 100644 --- a/components/style/stylesheets.rs +++ b/components/style/stylesheets.rs @@ -6,10 +6,10 @@ #![deny(missing_docs)] -use {Atom, Prefix, Namespace}; +use {Prefix, Namespace}; use counter_style::{CounterStyleRule, parse_counter_style_name, parse_counter_style_body}; use cssparser::{AtRuleParser, Parser, QualifiedRuleParser}; -use cssparser::{AtRuleType, RuleListParser, Token, parse_one_rule}; +use cssparser::{AtRuleType, RuleListParser, SourcePosition, parse_one_rule}; use cssparser::ToCss as ParserToCss; use error_reporting::{ParseErrorReporter, NullReporter}; #[cfg(feature = "servo")] @@ -41,7 +41,7 @@ use str::starts_with_ignore_ascii_case; use style_traits::ToCss; use stylist::FnvHashMap; use supports::SupportsCondition; -use values::CustomIdent; +use values::{CustomIdent, KeyframesName}; use values::specified::url::SpecifiedUrl; use viewport::ViewportRule; @@ -519,7 +519,7 @@ impl ToCssWithGuard for ImportRule { #[derive(Debug)] pub struct KeyframesRule { /// The name of the current animation. - pub name: CustomIdent, + pub name: KeyframesName, /// The keyframes specified for this CSS rule. pub keyframes: Vec>>, /// Vendor prefix type the @keyframes has. @@ -931,7 +931,7 @@ enum AtRulePrelude { /// A @viewport rule prelude. Viewport, /// A @keyframes rule, with its animation name and vendor prefix if exists. - Keyframes(CustomIdent, Option), + Keyframes(KeyframesName, Option), /// A @page rule prelude. Page, } @@ -1139,11 +1139,7 @@ impl<'a, 'b> AtRuleParser for NestedRuleParser<'a, 'b> { // Servo should not support @-moz-keyframes. return Err(()) } - let name = match input.next() { - Ok(Token::Ident(value)) => CustomIdent::from_ident(value, &["none"])?, - Ok(Token::QuotedString(value)) => CustomIdent(Atom::from(value)), - _ => return Err(()) - }; + let name = KeyframesName::parse(self.context, input)?; Ok(AtRuleType::WithBlock(AtRulePrelude::Keyframes(name, prefix))) }, diff --git a/components/style/stylist.rs b/components/style/stylist.rs index 4f5489f0d8c..8ae9998c4cb 100644 --- a/components/style/stylist.rs +++ b/components/style/stylist.rs @@ -349,13 +349,13 @@ impl Stylist { // Don't let a prefixed keyframes animation override a non-prefixed one. let needs_insertion = keyframes_rule.vendor_prefix.is_none() || - self.animations.get(&keyframes_rule.name.0).map_or(true, |rule| + self.animations.get(keyframes_rule.name.as_atom()).map_or(true, |rule| rule.vendor_prefix.is_some()); if needs_insertion { let animation = KeyframesAnimation::from_keyframes( &keyframes_rule.keyframes, keyframes_rule.vendor_prefix.clone(), guard); debug!("Found valid keyframe animation: {:?}", animation); - self.animations.insert(keyframes_rule.name.0.clone(), animation); + self.animations.insert(keyframes_rule.name.as_atom().clone(), animation); } } CssRule::FontFace(ref rule) => { diff --git a/components/style/values/mod.rs b/components/style/values/mod.rs index 7379cbad846..5caf64fc0ea 100644 --- a/components/style/values/mod.rs +++ b/components/style/values/mod.rs @@ -9,7 +9,7 @@ #![deny(missing_docs)] use Atom; -pub use cssparser::{RGBA, Parser, serialize_identifier}; +pub use cssparser::{RGBA, Token, Parser, serialize_identifier, serialize_string}; use parser::{Parse, ParserContext}; use std::ascii::AsciiExt; use std::borrow::Cow; @@ -240,6 +240,52 @@ impl ToCss for CustomIdent { } } +/// https://drafts.csswg.org/css-animations/#typedef-keyframes-name +#[derive(Debug, Clone, PartialEq, Eq, Hash)] +#[cfg_attr(feature = "servo", derive(HeapSizeOf))] +pub enum KeyframesName { + /// + Ident(CustomIdent), + /// + QuotedString(Atom), +} + +impl KeyframesName { + /// https://drafts.csswg.org/css-animations/#dom-csskeyframesrule-name + pub fn from_ident(value: String) -> Self { + match CustomIdent::from_ident((&*value).into(), &["none"]) { + Ok(ident) => KeyframesName::Ident(ident), + Err(()) => KeyframesName::QuotedString(value.into()), + } + } + + /// The name as an Atom + pub fn as_atom(&self) -> &Atom { + match *self { + KeyframesName::Ident(ref ident) => &ident.0, + KeyframesName::QuotedString(ref atom) => atom, + } + } +} + +impl Parse for KeyframesName { + fn parse(_context: &ParserContext, input: &mut Parser) -> Result { + match input.next() { + Ok(Token::Ident(s)) => Ok(KeyframesName::Ident(CustomIdent::from_ident(s, &["none"])?)), + Ok(Token::QuotedString(s)) => Ok(KeyframesName::QuotedString(s.into())), + _ => Err(()) + } + } +} + +impl ToCss for KeyframesName { + fn to_css(&self, dest: &mut W) -> fmt::Result where W: fmt::Write { + match *self { + KeyframesName::Ident(ref ident) => ident.to_css(dest), + KeyframesName::QuotedString(ref atom) => serialize_string(&atom.to_string(), dest), + } + } +} // A type for possible values for min- and max- flavors of width, // height, block-size, and inline-size. diff --git a/tests/unit/style/lib.rs b/tests/unit/style/lib.rs index f6a39d1a656..baddd4da618 100644 --- a/tests/unit/style/lib.rs +++ b/tests/unit/style/lib.rs @@ -13,7 +13,7 @@ extern crate parking_lot; extern crate rayon; extern crate rustc_serialize; extern crate selectors; -#[macro_use] extern crate servo_atoms; +extern crate servo_atoms; extern crate servo_config; extern crate servo_url; extern crate style; diff --git a/tests/unit/style/parsing/animation.rs b/tests/unit/style/parsing/animation.rs index 936d6c644f7..37cb3c5037b 100644 --- a/tests/unit/style/parsing/animation.rs +++ b/tests/unit/style/parsing/animation.rs @@ -7,7 +7,7 @@ use servo_atoms::Atom; use style::parser::Parse; use style::properties::longhands::animation_iteration_count::single_value::computed_value::T as AnimationIterationCount; use style::properties::longhands::animation_name; -use style::values::CustomIdent; +use style::values::{KeyframesName, CustomIdent}; use style_traits::ToCss; #[test] @@ -15,13 +15,13 @@ fn test_animation_name() { use self::animation_name::single_value::SpecifiedValue as SingleValue; let other_name = Atom::from("other-name"); assert_eq!(parse_longhand!(animation_name, "none"), - animation_name::SpecifiedValue(vec![SingleValue(CustomIdent(atom!("")))])); + animation_name::SpecifiedValue(vec![SingleValue(None)])); assert_eq!(parse_longhand!(animation_name, "other-name, none, 'other-name', \"other-name\""), animation_name::SpecifiedValue( - vec![SingleValue(CustomIdent(other_name.clone())), - SingleValue(CustomIdent(atom!(""))), - SingleValue(CustomIdent(other_name.clone())), - SingleValue(CustomIdent(other_name.clone()))])); + vec![SingleValue(Some(KeyframesName::Ident(CustomIdent(other_name.clone())))), + SingleValue(None), + SingleValue(Some(KeyframesName::QuotedString(other_name.clone()))), + SingleValue(Some(KeyframesName::QuotedString(other_name.clone())))])); } #[test] diff --git a/tests/unit/style/stylesheets.rs b/tests/unit/style/stylesheets.rs index 9a56ee80d53..dd1762ee686 100644 --- a/tests/unit/style/stylesheets.rs +++ b/tests/unit/style/stylesheets.rs @@ -23,7 +23,7 @@ use style::properties::longhands::animation_play_state; use style::shared_lock::SharedRwLock; use style::stylesheets::{Origin, Namespaces}; use style::stylesheets::{Stylesheet, NamespaceRule, CssRule, CssRules, StyleRule, KeyframesRule}; -use style::values::CustomIdent; +use style::values::{KeyframesName, CustomIdent}; use style::values::specified::{LengthOrPercentageOrAuto, Percentage}; pub fn block_from(iterable: I) -> PropertyDeclarationBlock @@ -222,7 +222,7 @@ fn test_parse_stylesheet() { ]))), }))), CssRule::Keyframes(Arc::new(stylesheet.shared_lock.wrap(KeyframesRule { - name: CustomIdent("foo".into()), + name: KeyframesName::Ident(CustomIdent("foo".into())), keyframes: vec![ Arc::new(stylesheet.shared_lock.wrap(Keyframe { selector: KeyframeSelector::new_for_unit_testing(