Bug 1766046 - cleanup animation-name. r=boris

Make the representation the same between Gecko and Servo code. This will
enable further clean-ups in the future.

Make serialization be correct, serializing as identifier unless it's an
invalid one (in which case we serialize as a string).

This changes our stringification behavior in the specified style, but
now it will match the computed style and be more correct over-all.

Differential Revision: https://phabricator.services.mozilla.com/D144473

layout/style/ServoStyleConstsInlines.h

@@ -1054,10 +1054,6 @@ inline AspectRatio StyleAspectRatio::ToLayoutRatio() const {
                     : AspectRatio();
 }
 
-inline nsAtom* StyleTimelineOrKeyframesName::AsAtom() const {
-  return IsIdent() ? AsIdent().AsAtom() : AsQuotedString().AsAtom();
-}
-
 }  // namespace mozilla
 
 #endif

layout/style/nsAnimationManager.cpp

@@ -208,39 +208,36 @@ static void UpdateOldAnimationPropertiesWithNew(
 static already_AddRefed<dom::AnimationTimeline> GetTimeline(
     const StyleAnimationTimeline& aStyleTimeline, nsPresContext* aPresContext,
     const NonOwningAnimationTarget& aTarget) {
-  RefPtr<dom::AnimationTimeline> timeline;
   switch (aStyleTimeline.tag) {
     case StyleAnimationTimeline::Tag::Timeline: {
       nsAtom* name = aStyleTimeline.AsTimeline().AsAtom();
+      if (name == nsGkAtoms::_empty) {
+        // That's how we represent `none`.
+        return nullptr;
+      }
       const auto* rule =
           aPresContext->StyleSet()->ScrollTimelineRuleForName(name);
-      if (rule) {
-        // We do intentionally use the pres context's document for the owner of
-        // ScrollTimeline since it's consistent with what we do for
-        // KeyframeEffect instance.
-        RefPtr<ScrollTimeline> scrollTimeline =
-            ScrollTimeline::FromRule(*rule, aPresContext->Document(), aTarget);
-        timeline = scrollTimeline;
-      } else {
-        // Unknown timeline, so treat is as no timeline.
-        // Keep nullptr.
+      if (!rule) {
+        // Unknown timeline, so treat is as no timeline. Keep nullptr.
+        return nullptr;
       }
-      break;
+      // We do intentionally use the pres context's document for the owner of
+      // ScrollTimeline since it's consistent with what we do for
+      // KeyframeEffect instance.
+      return ScrollTimeline::FromRule(*rule, aPresContext->Document(),
+                                      aTarget);
     }
     case StyleAnimationTimeline::Tag::Scroll: {
       const auto& scroll = aStyleTimeline.AsScroll();
-      timeline = ScrollTimeline::FromAnonymousScroll(
+      return ScrollTimeline::FromAnonymousScroll(
           aPresContext->Document(), aTarget, scroll._0, scroll._1);
       break;
     }
-    case StyleAnimationTimeline::Tag::None:
-      // Keep nullptr.
-      break;
     case StyleAnimationTimeline::Tag::Auto:
-      timeline = aTarget.mElement->OwnerDoc()->Timeline();
-      break;
+      return do_AddRef(aTarget.mElement->OwnerDoc()->Timeline());
   }
-  return timeline.forget();
+  MOZ_ASSERT_UNREACHABLE("Unknown animation-timeline value?");
+  return nullptr;
 }
 
 // Returns a new animation set up with given StyleAnimation.

servo/components/style/properties/gecko.mako.rs

@@ -1924,10 +1924,7 @@ mask-mode mask-repeat mask-clip mask-origin mask-composite mask-position-x mask-
 
         self.gecko.mAnimationNameCount = v.len() as u32;
         for (servo, gecko) in v.zip(self.gecko.mAnimations.iter_mut()) {
-            let atom = match servo.0 {
-                None => atom!(""),
-                Some(ref name) => name.as_atom().clone(),
-            };
+            let atom = servo.0.as_atom().clone();
             unsafe { bindings::Gecko_SetAnimationName(gecko, atom.into_addrefed()); }
         }
     }
@@ -1936,10 +1933,7 @@ mask-mode mask-repeat mask-clip mask-origin mask-composite mask-position-x mask-
         use crate::properties::longhands::animation_name::single_value::SpecifiedValue as AnimationName;
 
         let atom = self.gecko.mAnimations[index].mName.mRawPtr;
-        if atom == atom!("").as_ptr() {
-            return AnimationName(None)
-        }
-        AnimationName(Some(KeyframesName::from_atom(unsafe { Atom::from_raw(atom) })))
+        AnimationName(KeyframesName::from_atom(unsafe { Atom::from_raw(atom) }))
     }
     pub fn copy_animation_name_from(&mut self, other: &Self) {
         self.gecko.mAnimationNameCount = other.gecko.mAnimationNameCount;

servo/components/style/properties/properties.mako.rs

@@ -2928,7 +2928,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.is_some())
+                self.animation_name_iter().any(|name| !name.is_none())
             }
 
             /// Returns whether there are any transitions specified.

servo/components/style/values/mod.rs

@@ -16,7 +16,6 @@ pub use cssparser::{SourceLocation, Token, RGBA};
 use precomputed_hash::PrecomputedHash;
 use selectors::parser::SelectorParseErrorKind;
 use std::fmt::{self, Debug, Write};
-use std::hash;
 use style_traits::{CssWriter, ParseError, StyleParseErrorKind, ToCss};
 use to_shmem::impl_trivial_to_shmem;
 
@@ -448,28 +447,33 @@ impl CustomIdent {
         ident: &CowRcStr<'i>,
         excluding: &[&str],
     ) -> Result<Self, ParseError<'i>> {
-        use crate::properties::CSSWideKeyword;
-        // https://drafts.csswg.org/css-values-4/#custom-idents:
-        //
-        //     The CSS-wide keywords are not valid <custom-ident>s. The default
-        //     keyword is reserved and is also not a valid <custom-ident>.
-        //
-        if CSSWideKeyword::from_ident(ident).is_ok() || ident.eq_ignore_ascii_case("default") {
+        if !Self::is_valid(ident, excluding) {
             return Err(
                 location.new_custom_error(SelectorParseErrorKind::UnexpectedIdent(ident.clone()))
             );
         }
-
-        // https://drafts.csswg.org/css-values-4/#custom-idents:
-        //
-        //     Excluded keywords are excluded in all ASCII case permutations.
-        //
         if excluding.iter().any(|s| ident.eq_ignore_ascii_case(s)) {
             Err(location.new_custom_error(StyleParseErrorKind::UnspecifiedError))
         } else {
             Ok(CustomIdent(Atom::from(ident.as_ref())))
         }
     }
+
+    fn is_valid(ident: &str, excluding: &[&str]) -> bool {
+        use crate::properties::CSSWideKeyword;
+        // https://drafts.csswg.org/css-values-4/#custom-idents:
+        //
+        //     The CSS-wide keywords are not valid <custom-ident>s. The default
+        //     keyword is reserved and is also not a valid <custom-ident>.
+        if CSSWideKeyword::from_ident(ident).is_ok() || ident.eq_ignore_ascii_case("default") {
+            return false;
+        }
+
+        // https://drafts.csswg.org/css-values-4/#custom-idents:
+        //
+        //     Excluded keywords are excluded in all ASCII case permutations.
+        !excluding.iter().any(|s| ident.eq_ignore_ascii_case(s))
+    }
 }
 
 impl ToCss for CustomIdent {
@@ -486,45 +490,39 @@ impl ToCss for CustomIdent {
 ///
 /// <https://drafts.csswg.org/css-animations-2/#typedef-timeline-name>
 /// <https://drafts.csswg.org/css-animations/#typedef-keyframes-name>
+///
+/// We use a single atom for these. Empty atom represents no animation.
+#[repr(transparent)]
 #[derive(
-    Clone, Debug, MallocSizeOf, SpecifiedValueInfo, ToComputedValue, ToResolvedValue, ToShmem,
+    Clone, Debug, Hash, PartialEq, MallocSizeOf, SpecifiedValueInfo, ToComputedValue, ToResolvedValue, ToShmem,
 )]
-#[repr(C, u8)]
-pub enum TimelineOrKeyframesName {
-    /// <custom-ident>
-    Ident(CustomIdent),
-    /// <string>
-    QuotedString(Atom),
-}
+pub struct TimelineOrKeyframesName(Atom);
 
 impl TimelineOrKeyframesName {
     /// <https://drafts.csswg.org/css-animations/#dom-csskeyframesrule-name>
     pub fn from_ident(value: &str) -> Self {
-        let location = SourceLocation { line: 0, column: 0 };
-        let custom_ident = CustomIdent::from_ident(location, &value.into(), &["none"]).ok();
-        match custom_ident {
-            Some(ident) => Self::Ident(ident),
-            None => Self::QuotedString(value.into()),
-        }
+        Self(Atom::from(value))
+    }
+
+    /// Returns the `none` value.
+    pub fn none() -> Self {
+        Self(atom!(""))
+    }
+
+    /// Returns whether this is the special `none` value.
+    pub fn is_none(&self) -> bool {
+        self.0 == atom!("")
     }
 
     /// Create a new TimelineOrKeyframesName from Atom.
     #[cfg(feature = "gecko")]
     pub fn from_atom(atom: Atom) -> Self {
-        debug_assert_ne!(atom, atom!(""));
-
-        // FIXME: We might want to preserve <string>, but currently Gecko
-        // stores both of <custom-ident> and <string> into nsAtom, so
-        // we can't tell it.
-        Self::Ident(CustomIdent(atom))
+        Self(atom)
     }
 
     /// The name as an Atom
     pub fn as_atom(&self) -> &Atom {
-        match *self {
-            Self::Ident(ref ident) => &ident.0,
-            Self::QuotedString(ref atom) => atom,
-        }
+        &self.0
     }
 }
 
@@ -537,36 +535,17 @@ pub trait IsAuto {
     fn is_auto(&self) -> bool;
 }
 
-impl PartialEq for TimelineOrKeyframesName {
-    fn eq(&self, other: &Self) -> bool {
-        self.as_atom() == other.as_atom()
-    }
-}
-
-impl hash::Hash for TimelineOrKeyframesName {
-    fn hash<H>(&self, state: &mut H)
-    where
-        H: hash::Hasher,
-    {
-        self.as_atom().hash(state)
-    }
-}
-
 impl Parse for TimelineOrKeyframesName {
     fn parse<'i, 't>(
         _context: &ParserContext,
         input: &mut Parser<'i, 't>,
     ) -> Result<Self, ParseError<'i>> {
         let location = input.current_source_location();
-        match *input.next()? {
-            Token::Ident(ref s) => Ok(Self::Ident(CustomIdent::from_ident(
-                location,
-                s,
-                &["none"],
-            )?)),
-            Token::QuotedString(ref s) => Ok(Self::QuotedString(Atom::from(s.as_ref()))),
-            ref t => Err(location.new_unexpected_token_error(t.clone())),
-        }
+        Ok(match *input.next()? {
+            Token::Ident(ref s) => Self(CustomIdent::from_ident(location, s, &["none"])?.0),
+            Token::QuotedString(ref s) => Self(Atom::from(s.as_ref())),
+            ref t => return Err(location.new_unexpected_token_error(t.clone())),
+        })
     }
 }
 
@@ -575,10 +554,17 @@ impl ToCss for TimelineOrKeyframesName {
     where
         W: Write,
     {
-        match *self {
-            Self::Ident(ref ident) => ident.to_css(dest),
-            Self::QuotedString(ref atom) => atom.to_string().to_css(dest),
+        if self.0 == atom!("") {
+            return dest.write_str("none")
         }
+
+        self.0.with_str(|s| {
+            if CustomIdent::is_valid(s, &["none"]) {
+                serialize_identifier(s, dest)
+            } else {
+                s.to_css(dest)
+            }
+        })
     }
 }
 

servo/components/style/values/specified/box.rs

@@ -708,33 +708,30 @@ impl AnimationIterationCount {
     PartialEq,
     SpecifiedValueInfo,
     ToComputedValue,
+    ToCss,
     ToResolvedValue,
     ToShmem,
 )]
 #[value_info(other_values = "none")]
-pub struct AnimationName(pub Option<KeyframesName>);
+pub struct AnimationName(pub KeyframesName);
 
 impl AnimationName {
     /// Get the name of the animation as an `Atom`.
     pub fn as_atom(&self) -> Option<&Atom> {
-        self.0.as_ref().map(|n| n.as_atom())
+        if self.is_none() {
+            return None;
+        }
+        Some(self.0.as_atom())
     }
 
     /// Returns the `none` value.
     pub fn none() -> Self {
-        AnimationName(None)
+        AnimationName(KeyframesName::none())
     }
-}
 
-impl ToCss for AnimationName {
-    fn to_css<W>(&self, dest: &mut CssWriter<W>) -> fmt::Result
-    where
-        W: Write,
-    {
-        match self.0 {
-            Some(ref name) => name.to_css(dest),
-            None => dest.write_str("none"),
-        }
+    /// Returns whether this is the none value.
+    pub fn is_none(&self) -> bool {
+        self.0.is_none()
     }
 }
 
@@ -744,11 +741,11 @@ impl Parse for AnimationName {
         input: &mut Parser<'i, 't>,
     ) -> Result<Self, ParseError<'i>> {
         if let Ok(name) = input.try_parse(|input| KeyframesName::parse(context, input)) {
-            return Ok(AnimationName(Some(name)));
+            return Ok(AnimationName(name));
         }
 
         input.expect_ident_matching("none")?;
-        Ok(AnimationName(None))
+        Ok(AnimationName(KeyframesName::none()))
     }
 }
 
@@ -847,8 +844,6 @@ fn is_default<T: Default + PartialEq>(value: &T) -> bool {
 pub enum AnimationTimeline {
     /// Use default timeline. The animation’s timeline is a DocumentTimeline.
     Auto,
-    /// The animation is not associated with a timeline.
-    None,
     /// The scroll-timeline name
     Timeline(TimelineName),
     /// The scroll() notation
@@ -878,16 +873,20 @@ impl Parse for AnimationTimeline {
         input: &mut Parser<'i, 't>,
     ) -> Result<Self, ParseError<'i>> {
         // We are using the same parser for TimelineName and KeyframesName, but animation-timeline
-        // accepts "auto", so need to manually parse this. (We can not derive Parse because
-        // TimelineName excludes only "none" keyword.)
+        // accepts "auto", so need to manually parse this. (We can not derive
+        // Parse because TimelineName excludes only the "none" keyword).
+        //
         // FIXME: Bug 1733260: we may drop None based on the spec issue:
-        // Note: https://github.com/w3c/csswg-drafts/issues/6674.
+        // https://github.com/w3c/csswg-drafts/issues/6674
+        //
+        // If `none` is removed, then we could potentially shrink this the same
+        // way we deal with animation-name.
         if input.try_parse(|i| i.expect_ident_matching("auto")).is_ok() {
             return Ok(Self::Auto);
         }
 
         if input.try_parse(|i| i.expect_ident_matching("none")).is_ok() {
-            return Ok(Self::None);
+            return Ok(AnimationTimeline::Timeline(TimelineName::none()));
         }
 
         // https://drafts.csswg.org/scroll-animations-1/rewrite#scroll-notation

servo/ports/geckolib/glue.rs

@@ -2711,7 +2711,7 @@ pub unsafe extern "C" fn Servo_KeyframesRule_SetName(
     name: *mut nsAtom,
 ) {
     write_locked_arc(rule, |rule: &mut KeyframesRule| {
-        rule.name = KeyframesName::Ident(CustomIdent(Atom::from_addrefed(name)));
+        rule.name = KeyframesName::from_atom(Atom::from_addrefed(name));
     })
 }
 

servo/tests/unit/style/parsing/animation.rs

@@ -10,20 +10,6 @@ use style::values::{KeyframesName, CustomIdent};
 use style::values::specified::AnimationIterationCount;
 use style_traits::ToCss;
 
-#[test]
-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(None)]));
-    assert_eq!(parse_longhand!(animation_name, "other-name, none, 'other-name', \"other-name\""),
-               animation_name::SpecifiedValue(
-                   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]
 fn test_animation_iteration() {
     assert_roundtrip_with_context!(AnimationIterationCount::parse, "0", "0");

testing/web-platform/tests/css/css-animations/parsing/animation-name-computed.html

@@ -20,8 +20,9 @@ test_computed_value("animation-name", 'ease-in');
 test_computed_value("animation-name", 'infinite');
 test_computed_value("animation-name", 'paused');
 test_computed_value("animation-name", 'first, second, third');
+test_computed_value("animation-name", '"something"', ["something", '"something"']);
 
-// TODO: Test strings, after https://github.com/w3c/csswg-drafts/issues/2435
+// TODO: Test more strings, after https://github.com/w3c/csswg-drafts/issues/2435
 // is resolved.
 // Examples that need testing either here or in animation-name-invalid.html :
 // '"Initial"', '"initial"', '"None"', '"Default"', '" x "', "1", '" "', '""',

testing/web-platform/tests/css/css-animations/parsing/animation-name-valid.html

@@ -20,10 +20,10 @@ test_valid_value("animation-name", 'infinite');
 test_valid_value("animation-name", 'paused');
 test_valid_value("animation-name", 'first, second, third');
 
-test_valid_value("animation-name", '"string"');
-test_valid_value("animation-name", '"multi word string"');
+test_valid_value("animation-name", '"string"', ['"string"', "string"]);
+test_valid_value("animation-name", '"multi word string"', ['"multi word string"', "multi\\ word\\ string"]);
 test_valid_value("animation-name", '"initial"');
-test_valid_value("animation-name", '"---\\22---"', '\"---\\\"---\"');
+test_valid_value("animation-name", '"---\\22---"', ['\"---\\\"---\"', '---\\\"---']);
 </script>
 </body>
 </html>