Backed out changeset dfec425c53fe (bug 1766046) for causing wpt failures at animation-timeline-computed.html. CLOSED TREE

layout/style/ServoStyleConstsInlines.h

@@ -1054,6 +1054,10 @@ 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,36 +208,39 @@ 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) {
-        // Unknown timeline, so treat is as no timeline. Keep nullptr.
-        return nullptr;
-      }
+      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.
-      return ScrollTimeline::FromRule(*rule, aPresContext->Document(),
-                                      aTarget);
+        RefPtr<ScrollTimeline> scrollTimeline =
+            ScrollTimeline::FromRule(*rule, aPresContext->Document(), aTarget);
+        timeline = scrollTimeline;
+      } else {
+        // Unknown timeline, so treat is as no timeline.
+        // Keep nullptr.
+      }
+      break;
     }
     case StyleAnimationTimeline::Tag::Scroll: {
       const auto& scroll = aStyleTimeline.AsScroll();
-      return ScrollTimeline::FromAnonymousScroll(
+      timeline = ScrollTimeline::FromAnonymousScroll(
           aPresContext->Document(), aTarget, scroll._0, scroll._1);
       break;
     }
+    case StyleAnimationTimeline::Tag::None:
+      // Keep nullptr.
+      break;
     case StyleAnimationTimeline::Tag::Auto:
-      return do_AddRef(aTarget.mElement->OwnerDoc()->Timeline());
+      timeline = aTarget.mElement->OwnerDoc()->Timeline();
+      break;
   }
-  MOZ_ASSERT_UNREACHABLE("Unknown animation-timeline value?");
-  return nullptr;
+  return timeline.forget();
 }
 
 // Returns a new animation set up with given StyleAnimation.

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

@@ -1924,7 +1924,10 @@ 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 = servo.0.as_atom().clone();
+            let atom = match servo.0 {
+                None => atom!(""),
+                Some(ref name) => name.as_atom().clone(),
+            };
             unsafe { bindings::Gecko_SetAnimationName(gecko, atom.into_addrefed()); }
         }
     }
@@ -1933,7 +1936,10 @@ 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;
-        AnimationName(KeyframesName::from_atom(unsafe { Atom::from_raw(atom) }))
+        if atom == atom!("").as_ptr() {
+            return AnimationName(None)
+        }
+        AnimationName(Some(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.is_none())
+                self.animation_name_iter().any(|name| name.0.is_some())
             }
 
             /// Returns whether there are any transitions specified.

servo/components/style/values/mod.rs

@@ -16,6 +16,7 @@ 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;
 
@@ -447,32 +448,27 @@ impl CustomIdent {
         ident: &CowRcStr<'i>,
         excluding: &[&str],
     ) -> Result<Self, ParseError<'i>> {
-        if !Self::is_valid(ident, excluding) {
-            return Err(
-                location.new_custom_error(SelectorParseErrorKind::UnexpectedIdent(ident.clone()))
-            );
-        }
-        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;
+            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.
-        !excluding.iter().any(|s| ident.eq_ignore_ascii_case(s))
+        //
+        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())))
+        }
     }
 }
 
@@ -490,39 +486,45 @@ 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, Hash, PartialEq, MallocSizeOf, SpecifiedValueInfo, ToComputedValue, ToResolvedValue, ToShmem,
+    Clone, Debug, MallocSizeOf, SpecifiedValueInfo, ToComputedValue, ToResolvedValue, ToShmem,
 )]
-pub struct TimelineOrKeyframesName(Atom);
+#[repr(C, u8)]
+pub enum TimelineOrKeyframesName {
+    /// <custom-ident>
+    Ident(CustomIdent),
+    /// <string>
+    QuotedString(Atom),
+}
 
 impl TimelineOrKeyframesName {
     /// <https://drafts.csswg.org/css-animations/#dom-csskeyframesrule-name>
     pub fn from_ident(value: &str) -> Self {
-        Self(Atom::from(value))
+        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()),
         }
-
-    /// 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 {
-        Self(atom)
+        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))
     }
 
     /// The name as an Atom
     pub fn as_atom(&self) -> &Atom {
-        &self.0
+        match *self {
+            Self::Ident(ref ident) => &ident.0,
+            Self::QuotedString(ref atom) => atom,
+        }
     }
 }
 
@@ -535,17 +537,36 @@ 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();
-        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())),
-        })
+        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())),
+        }
     }
 }
 
@@ -554,17 +575,10 @@ impl ToCss for TimelineOrKeyframesName {
     where
         W: Write,
     {
-        if self.0 == atom!("") {
-            return dest.write_str("none")
+        match *self {
+            Self::Ident(ref ident) => ident.to_css(dest),
+            Self::QuotedString(ref atom) => atom.to_string().to_css(dest),
         }
-
-        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,30 +708,33 @@ impl AnimationIterationCount {
     PartialEq,
     SpecifiedValueInfo,
     ToComputedValue,
-    ToCss,
     ToResolvedValue,
     ToShmem,
 )]
 #[value_info(other_values = "none")]
-pub struct AnimationName(pub KeyframesName);
+pub struct AnimationName(pub Option<KeyframesName>);
 
 impl AnimationName {
     /// Get the name of the animation as an `Atom`.
     pub fn as_atom(&self) -> Option<&Atom> {
-        if self.is_none() {
-            return None;
-        }
-        Some(self.0.as_atom())
+        self.0.as_ref().map(|n| n.as_atom())
     }
 
     /// Returns the `none` value.
     pub fn none() -> Self {
-        AnimationName(KeyframesName::none())
+        AnimationName(None)
     }
+}
 
-    /// Returns whether this is the none value.
-    pub fn is_none(&self) -> bool {
-        self.0.is_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"),
+        }
     }
 }
 
@@ -741,11 +744,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(name));
+            return Ok(AnimationName(Some(name)));
         }
 
         input.expect_ident_matching("none")?;
-        Ok(AnimationName(KeyframesName::none()))
+        Ok(AnimationName(None))
     }
 }
 
@@ -844,6 +847,8 @@ 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
@@ -873,20 +878,16 @@ 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 the "none" keyword).
-        //
+        // accepts "auto", so need to manually parse this. (We can not derive Parse because
+        // TimelineName excludes only "none" keyword.)
         // FIXME: Bug 1733260: we may drop None based on the spec issue:
-        // 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.
+        // Note: https://github.com/w3c/csswg-drafts/issues/6674.
         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(AnimationTimeline::Timeline(TimelineName::none()));
+            return Ok(Self::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::from_atom(Atom::from_addrefed(name));
+        rule.name = KeyframesName::Ident(CustomIdent(Atom::from_addrefed(name)));
     })
 }
 

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

@@ -10,6 +10,20 @@ 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,9 +20,8 @@ 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 more strings, after https://github.com/w3c/csswg-drafts/issues/2435
+// TODO: Test 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"', ['"string"', "string"]);
-test_valid_value("animation-name", '"multi word string"', ['"multi word string"', "multi\\ word\\ string"]);
+test_valid_value("animation-name", '"string"');
+test_valid_value("animation-name", '"multi word string"');
 test_valid_value("animation-name", '"initial"');
-test_valid_value("animation-name", '"---\\22---"', ['\"---\\\"---\"', '---\\\"---']);
+test_valid_value("animation-name", '"---\\22---"', '\"---\\\"---\"');
 </script>
 </body>
 </html>