Bug 1908819 - Add the tentative serialization for animation shorthand…

…. r=firefox-style-system-reviewers,layout-reviewers,emilio In order to avoid this unexpected fail, we add this tentative serialization for animation shorthand. 1. animation-timeline is reset-only in this shorthand, so we don't parse it and we reset it to `vec![auto]`. 2. we don't serialize the shorthand if animation-timeline is not the initial value. This is a tentative solution and we should update it with the new syntax once the spec gets update (w3c/csswg-drafts#6946). Differential Revision: https://phabricator.services.mozilla.com/D217164
i3roly · Jul 22, 2024 · 3ef00ff · 3ef00ff
1 parent 170fc9a
commit 3ef00ff
Show file tree

Hide file tree

Showing 8 changed files with 78 additions and 64 deletions.
diff --git a/dom/animation/test/mozilla/test_get_animations_on_scroll_animations.html b/dom/animation/test/mozilla/test_get_animations_on_scroll_animations.html
@@ -27,7 +27,8 @@
 test(function(t) {
   const div = addDiv(t,
     { style: "width: 10px; height: 100px; " +
-             "animation: animWidth 100s scroll(), animTop 200s;" });
+             "animation: animWidth 100s, animTop 200s; " +
+             "animation-timeline: scroll(), auto;"});
 
   // Sanity check to make sure the scroll animation is there.
   addDiv(t, { class: "fill-vh" });
@@ -50,7 +51,8 @@
 test(function(t) {
   const div = addDiv(t,
     { style: "width: 10px; height: 100px; " +
-             "animation: animWidth 100s scroll(), animTop 100s;" });
+             "animation: animWidth 100s, animTop 100s; " +
+             "animation-timeline: scroll(), auto;"});
 
   // Sanity check to make sure the scroll animation is there.
   addDiv(t, { class: "fill-vh" });

diff --git a/layout/style/test/file_animations_omta_scroll.html b/layout/style/test/file_animations_omta_scroll.html
@@ -129,7 +129,8 @@
 // with document-timeline.
 addAsyncAnimTest(async function() {
   new_scroller();
-  new_div("animation: geometry 10s --scroll_timeline, always_fifty 1s infinite;");
+  new_div("animation: geometry 10s, always_fifty 1s infinite; " +
+          "animation-timeline: --scroll_timeline, auto");
   await waitForPaintsFlushed();
 
   // Note: width is not a OMTA property, so it must be running on the main
@@ -144,7 +145,8 @@
 // transform property with scroll-driven animations.
 addAsyncAnimTest(async function() {
   let scroller = new_scroller();
-  new_div("animation: transform_anim 1s linear --scroll_timeline;");
+  new_div("animation: transform_anim 1s linear; " +
+          "animation-timeline: --scroll_timeline;");
   await waitForPaintsFlushed();
 
   scroller.scrollTop = 50;
@@ -163,7 +165,8 @@
 // active phase to the before phase.
 addAsyncAnimTest(async function() {
   let scroller = new_scroller();
-  new_div("animation: always_fifty 5s linear 5s --scroll_timeline; " +
+  new_div("animation: always_fifty 5s linear 5s; " +
+          "animation-timeline: --scroll_timeline; " +
           "transform: translate(25px);");
   await waitForPaintsFlushed();
 
@@ -212,7 +215,8 @@
 // the active phase to the before phase.
 addAsyncAnimTest(async function() {
   let scroller = new_scroller();
-  new_div("animation: always_fifty 5s linear 5s --scroll_timeline; ");
+  new_div("animation: always_fifty 5s linear 5s; " +
+          "animation-timeline: --scroll_timeline;");
   await waitForPaintsFlushed();
 
   // NOTE: getOMTAStyle() can't detect the animation is running on the
@@ -252,7 +256,8 @@
 addAsyncAnimTest(async function() {
   let scroller = new_scroller();
   new_div("animation: transform_anim 10s linear -5s paused, " +
-          "           always_fifty 5s linear 5s --scroll_timeline;");
+          "           always_fifty 5s linear 5s; " +
+          "animation-timeline: auto, --scroll_timeline;");
   await waitForPaintsFlushed();
 
   omta_is_approx("transform", { tx: 100 }, 0.1, RunningOn.Compositor,
@@ -294,8 +299,9 @@
     "@keyframes anim { from, to { transform: translate(50px) } }" +
     "</style>" +
     "<div id='target_in_iframe' " +
-    "     style='width:50px; height:50px; background:green;" +
-    "            animation: anim 10s linear scroll(root);'>" +
+    "     style='width:50px; height:50px; background:green; " +
+    "            animation: anim 10s linear; " +
+    "            animation-timeline: scroll(root);'>" +
     "</div>" +
     "</html>";
 

diff --git a/layout/style/test/file_animations_omta_scroll_rtl.html b/layout/style/test/file_animations_omta_scroll_rtl.html
@@ -92,7 +92,8 @@
 // transform property with scroll-driven animations. The writing mode is from
 // right to left, so we have to use the negative scroll offset.
 addAsyncAnimTest(async function() {
-  new_div("animation: transform_anim 1s linear scroll(x root);");
+  new_div("animation: transform_anim 1s linear; " +
+          "animation-timeline: scroll(x root);");
   await waitForPaintsFlushed();
 
   const root = document.scrollingElement;

diff --git a/layout/style/test/property_database.js b/layout/style/test/property_database.js
@@ -13965,20 +13965,6 @@ if (IsCSSPropertyPrefEnabled("layout.css.scroll-driven-animations.enabled")) {
   // Basically, web-platform-tests should cover most cases, so here we only
   // put some basic test cases.
   gCSSProperties["animation"].subproperties.push("animation-timeline");
-  gCSSProperties["animation"].initial_values.push(
-    "none none 0s 0s ease normal running 1.0 auto",
-    "none none auto"
-  );
-  gCSSProperties["animation"].other_values.push(
-    "none none 0s 0s cubic-bezier(0.25, 0.1, 0.25, 1.0) normal running 1.0 auto",
-    "bounce 1s linear 2s --timeline",
-    "bounce 1s 2s linear none",
-    "bounce --timeline",
-    "2s, 1s bounce --timeline",
-    "1s bounce --timeline, 2s",
-    "1s bounce none, 2s none auto"
-  );
-
   gCSSProperties["-moz-animation"].subproperties.push("animation-timeline");
   gCSSProperties["-webkit-animation"].subproperties.push("animation-timeline");
 

diff --git a/layout/style/test/test_shorthand_property_getters.html b/layout/style/test/test_shorthand_property_getters.html
@@ -200,8 +200,10 @@
 e.setAttribute("style", "transition: color, width; transition-delay: 0s");
 is(e.style.transition, "", "should not have transition shorthand (lists different lengths)");
 
+// FIXME: Bug 1824261. The 1st case should be non-empty if animation-timeline is
+// not reset-only.
 e.setAttribute("style", "animation-name: bounce, roll; animation-duration: 1s, 200ms; animation-timing-function: ease-in, linear; animation-delay: 0s, 1s; animation-direction: normal, reverse; animation-fill-mode: forwards, backwards; animation-iteration-count: infinite, 2; animation-play-state: paused, running; animation-timeline: auto, auto;");
-isnot(e.style.animation, "", "should have animation shorthand (lists same length)");
+is(e.style.animation, "", "should have animation shorthand (lists same length, Bug 1824261)");
 e.setAttribute("style", "animation-name: bounce, roll, left; animation-duration: 1s, 200ms; animation-timing-function: ease-in, linear; animation-delay: 0s, 1s; animation-direction: normal, reverse; animation-fill-mode: forwards, backwards; animation-iteration-count: infinite, 2; animation-play-state: paused, running; animation-timeline: auto, auto;");
 is(e.style.animation, "", "should not have animation shorthand (lists different lengths)");
 e.setAttribute("style", "animation-name: bounce; animation-duration: 1s, 200ms; animation-timing-function: ease-in, linear; animation-delay: 0s, 1s; animation-direction: normal, reverse; animation-fill-mode: forwards, backwards; animation-iteration-count: infinite, 2; animation-play-state: paused, running; animation-timeline: auto, auto;");

diff --git a/servo/components/style/properties/shorthands/ui.mako.rs b/servo/components/style/properties/shorthands/ui.mako.rs
@@ -213,12 +213,13 @@ macro_rules! try_parse_one {
                     rule_types_allowed="Style"
                     spec="https://drafts.csswg.org/css-animations/#propdef-animation">
     <%
-        props = "name timeline duration timing_function delay iteration_count \
+        props = "name duration timing_function delay iteration_count \
                  direction fill_mode play_state".split()
     %>
     % for prop in props:
     use crate::properties::longhands::animation_${prop};
     % endfor
+    use crate::properties::longhands::animation_timeline;
 
     pub fn parse_value<'i, 't>(
         context: &ParserContext,
@@ -254,9 +255,14 @@ macro_rules! try_parse_one {
                 try_parse_one!(context, input, fill_mode, animation_fill_mode);
                 try_parse_one!(context, input, play_state, animation_play_state);
                 try_parse_one!(context, input, name, animation_name);
-                if static_prefs::pref!("layout.css.scroll-driven-animations.enabled") {
-                    try_parse_one!(context, input, timeline, animation_timeline);
-                }
+
+                // Note: per spec issue discussion, all animation longhands not defined in
+                // Animations 1 are defined as reset-only sub-properties for now.
+                // https://github.com/w3c/csswg-drafts/issues/6946#issuecomment-1233190360
+                //
+                // FIXME: Bug 1824261. We should revisit this when the spec gets updated with the
+                // new syntax.
+                // https://github.com/w3c/csswg-drafts/issues/6946
 
                 parsed -= 1;
                 break
@@ -290,6 +296,10 @@ macro_rules! try_parse_one {
             % for prop in props:
             animation_${prop}: animation_${prop}::SpecifiedValue(${prop}s.into()),
             % endfor
+            // FIXME: Bug 1824261. animation-timeline is reset-only for now.
+            animation_timeline: animation_timeline::SpecifiedValue(
+                vec![animation_timeline::single_value::get_initial_specified_value()].into()
+            ),
         })
     }
 
@@ -310,15 +320,15 @@ macro_rules! try_parse_one {
 
             // If any value list length is differs then we don't do a shorthand serialization
             // either.
-            % for name in props[2:]:
+            % for name in props[1:]:
                 if len != self.animation_${name}.0.len() {
                     return Ok(())
                 }
             % endfor
 
-            // If the preference of animation-timeline is disabled, `self.animation_timeline` is
-            // None.
-            if self.animation_timeline.map_or(false, |v| len != v.0.len()) {
+            // FIXME: Bug 1824261. We don't serialize this shorthand if the animation-timeline is
+            // speficied, per the wpt update: https://github.com/web-platform-tests/wpt/pull/38848.
+            if self.animation_timeline.map_or(false, |v| v.0.len() != 1 || !v.0[0].is_auto()) {
                 return Ok(());
             }
 
@@ -355,10 +365,6 @@ macro_rules! try_parse_one {
                     !matches!(self.animation_play_state.0[i], AnimationPlayState::Running);
                 let animation_name = &self.animation_name.0[i];
                 let has_name = !animation_name.is_none();
-                let has_timeline = match self.animation_timeline {
-                    Some(timeline) => !timeline.0[i].is_auto(),
-                    _ => false,
-                };
 
                 let mut writer = SequenceWriter::new(dest, " ");
 
@@ -373,7 +379,7 @@ macro_rules! try_parse_one {
                 }
 
                 // For animation-delay and animation-iteration-count.
-                % for name in props[4:6]:
+                % for name in props[3:5]:
                 if has_${name} {
                     writer.item(&self.animation_${name}.0[i])?;
                 }
@@ -393,18 +399,14 @@ macro_rules! try_parse_one {
 
                 // If all values are initial, we must serialize animation-name.
                 let has_any = {
-                    has_timeline
+                    has_duration
                 % for name in props[2:]:
                         || has_${name}
                 % endfor
                 };
                 if has_name || !has_any {
                     writer.item(animation_name)?;
                 }
-
-                if has_timeline {
-                    writer.item(&self.animation_timeline.unwrap().0[i])?;
-                }
             }
             Ok(())
         }

diff --git a/testing/web-platform/meta/scroll-animations/css/animation-shorthand.html.ini b/testing/web-platform/meta/scroll-animations/css/animation-shorthand.html.ini
@@ -1,28 +1,10 @@
 [animation-shorthand.html]
-  [e.style['animation'\] = "1s linear 1s 2 reverse forwards paused anim scroll()" should not set the property value]
-    bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1824261
-    expected: FAIL
-
-  [e.style['animation'\] = "1s linear 1s 2 reverse forwards paused anim view()" should not set the property value]
-    bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1824261
-    expected: FAIL
-
-  [e.style['animation'\] = "1s linear 1s 2 reverse forwards paused anim --timeline" should not set the property value]
-    bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1824261
-    expected: FAIL
-
   [e.style['animation'\] = "1s linear 1s 2 reverse forwards paused anim1,\\n   1s linear 1s 2 reverse forwards paused anim2,\\n   1s linear 1s 2 reverse forwards paused anim3" should set animation-range-end]
     expected: FAIL
 
   [e.style['animation'\] = "1s linear 1s 2 reverse forwards paused anim1,\\n   1s linear 1s 2 reverse forwards paused anim2,\\n   1s linear 1s 2 reverse forwards paused anim3" should set animation-range-start]
     expected: FAIL
 
-  [Animation shorthand can not represent non-initial timelines (specified)]
-    expected: FAIL
-
-  [Animation shorthand can not represent non-initial timelines (computed)]
-    expected: FAIL
-
   [Animation shorthand can not represent non-initial animation-range-start (specified)]
     expected: FAIL
 
@@ -34,6 +16,3 @@
 
   [Animation shorthand can not represent non-initial animation-range-end (computed)]
     expected: FAIL
-
-  [e.style['animation'\] = "1s linear 1s 2 reverse forwards paused anim1,\\n   1s linear 1s 2 reverse forwards paused anim2,\\n   1s linear 1s 2 reverse forwards paused anim3" should set animation-timeline]
-    expected: FAIL
diff --git a/testing/web-platform/tests/scroll-animations/css/animation-shorthand.html b/testing/web-platform/tests/scroll-animations/css/animation-shorthand.html
@@ -133,4 +133,40 @@
   assert_equals(getComputedStyle(target).animation, '');
 }, 'Animation shorthand can not represent non-initial animation-range-end (computed)');
 
+test((t) => {
+  t.add_cleanup(() => {
+    target.style = '';
+  });
+
+  target.style.animationName = "bounce, roll";
+  target.style.animationDuration = "1s, 0.2s";
+  target.style.animationTimingFunction = "ease-in, linear";
+  target.style.animationDelay = "0s, 1s";
+  target.style.animationDirection = "normal, reverse";
+  target.style.animationFillMode = "forwards, backwards";
+  target.style.animationIterationCount = "infinite, 2";
+  target.style.animationPlayState = "paused, running";
+  // animation-timeline and animation-range-{start|end} are initial values.
+  assert_equals(target.style.animation, '');
+}, 'Animation shorthand can not be represented with same list length (specified)');
+
+test((t) => {
+  t.add_cleanup(() => {
+    target.style = '';
+  });
+
+  target.style.animationName = "bounce, roll";
+  target.style.animationDuration = "1s, 0.2s";
+  target.style.animationTimingFunction = "ease-in, linear";
+  target.style.animationDelay = "0s, 1s";
+  target.style.animationDirection = "normal, reverse";
+  target.style.animationFillMode = "forwards, backwards";
+  target.style.animationIterationCount = "infinite, 2";
+  target.style.animationPlayState = "paused, running";
+  // animation-timeline and animation-range-{start|end} are initial values.
+  assert_equals(getComputedStyle(target).animation,
+      '1s ease-in infinite forwards paused bounce, ' +
+      '0.2s linear 1s 2 reverse backwards roll');
+}, 'Animation shorthand can be represented with same list length (computed)');
+
 </script>