Bug 1348519 - Part 2: Implement Animate for track lists on grid-template-{columns|rows}. r=emilio
authorBoris Chiou <boris.chiou@gmail.com>
Sat, 12 Jan 2019 02:22:39 +0000
changeset 510701 bd4efc854419ae1643bfaeeedc67403703575ea0
parent 510700 37838b3650fd5b4c66cb1b63b366a40931076fcb
child 510702 80dc7df104e63383c2e80fbf7644bd348634c8c4
push id10547
push userffxbld-merge
push dateMon, 21 Jan 2019 13:03:58 +0000
treeherdermozilla-beta@24ec1916bffe [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersemilio
bugs1348519
milestone66.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1348519 - Part 2: Implement Animate for track lists on grid-template-{columns|rows}. r=emilio Based on https://bugzilla.mozilla.org/show_bug.cgi?id=1348519#c6 and https://github.com/w3c/csswg-drafts/issues/3201: Currently grid-template-rows/columns interpolate “per computed value”, which means that if the number of tracks differs, or any track changes to/from a particular keyword value to any other value, or if a line name is added/removed at any position, the entire track listing is interpolated as “discrete”. But we "agree" with two more granular options: 1. Check interpolation type per track, rather than for the entire list, before falling back to discrete. I.e. a length-percentage track can animate between two values while an adjacent auto track flips discretely to min-content. 2. Allow discrete interpolation of line name changes independently of track sizes. Besides, for the repeat() function, it's complicated to support interpolation between different repeat types (i.e. auto-fill, auto-fit) and different repeat counts, so we always fall-back to discrete if the first parameter of repeat() is different. Depends on D16339 Differential Revision: https://phabricator.services.mozilla.com/D16129
layout/style/test/test_transitions_per_property.html
servo/components/style/properties/longhands/position.mako.rs
servo/components/style/values/animated/grid.rs
servo/components/style/values/animated/mod.rs
servo/components/style/values/generics/grid.rs
testing/web-platform/tests/web-animations/animation-model/animation-types/property-list.js
--- a/layout/style/test/test_transitions_per_property.html
+++ b/layout/style/test/test_transitions_per_property.html
@@ -322,16 +322,23 @@ if (IsCSSPropertyPrefEnabled("layout.css
 }
 
 if (IsCSSPropertyPrefEnabled("layout.css.scrollbar-color.enabled")) {
   supported_properties["scrollbar-color"] = [
     test_scrollbar_color_transition,
   ];
 }
 
+// For properties which are well-tested by web-platform-tests, we don't need to
+// test animations/transitions again on them.
+var skipped_transitionable_properties = [
+  "grid-template-columns",
+  "grid-template-rows",
+]
+
 // Logical properties.
 for (const logical_side of ["inline-start", "inline-end", "block-start", "block-end"]) {
   supported_properties["border-" + logical_side + "-color"] = supported_properties["border-top-color"];
   supported_properties["border-" + logical_side + "-width"] = supported_properties["border-top-width"];
   supported_properties["margin-" + logical_side] = supported_properties["margin-top"];
   supported_properties["padding-" + logical_side] = supported_properties["padding-top"];
   supported_properties["inset-" + logical_side] = supported_properties["top"];
 }
@@ -1086,16 +1093,17 @@ function sample_array(array, count) {
 }
 
 // Test that transitions don't do anything (i.e., aren't supported) on
 // the properties not in our test list above (and not transition
 // properties themselves).
 for (prop in gCSSProperties) {
   var info = gCSSProperties[prop];
   if (!(prop in supported_properties) &&
+      !skipped_transitionable_properties.includes(prop) &&
       info.type != CSS_TYPE_TRUE_SHORTHAND &&
       !("alias_for" in info) &&
       !prop.match(/^transition-/) &&
       // FIXME (Bug 119078): THIS SHOULD REALLY NOT BE NEEDED!
       prop != "-moz-binding" &&
       prop != "mask") {
 
     if ("prerequisites" in info) {
--- a/servo/components/style/properties/longhands/position.mako.rs
+++ b/servo/components/style/properties/longhands/position.mako.rs
@@ -378,17 +378,17 @@ macro_rules! impl_align_conversions {
     ${helpers.predefined_type(
         "grid-template-%ss" % kind,
         "GridTemplateComponent",
         "specified::GenericGridTemplateComponent::None",
         products="gecko",
         spec="https://drafts.csswg.org/css-grid/#propdef-grid-template-%ss" % kind,
         boxed=True,
         flags="GETCS_NEEDS_LAYOUT_FLUSH",
-        animation_value_type="discrete",
+        animation_value_type="ComputedValue",
     )}
 
 % endfor
 
 ${helpers.predefined_type(
     "grid-auto-flow",
     "GridAutoFlow",
     "computed::GridAutoFlow::row()",
new file mode 100644
--- /dev/null
+++ b/servo/components/style/values/animated/grid.rs
@@ -0,0 +1,153 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at https://mozilla.org/MPL/2.0/. */
+
+//! Animation implementation for various grid-related types.
+
+// Note: we can implement Animate on their generic types directly, but in this case we need to
+// make sure two trait bounds, L: Clone and I: PartialEq, are satisfied on almost all the
+// grid-related types and their other trait implementations because Animate needs them. So in
+// order to avoid adding these two trait bounds (or maybe more..) everywhere, we implement
+// Animate for the computed types, instead of the generic types.
+
+use super::{Animate, Procedure, ToAnimatedZero};
+use crate::values::computed::{GridTemplateComponent, TrackList, TrackSize};
+use crate::values::computed::Integer;
+use crate::values::computed::LengthPercentage;
+use crate::values::distance::{ComputeSquaredDistance, SquaredDistance};
+use crate::values::generics::grid as generics;
+
+fn discrete<T: Clone>(from: &T, to: &T, procedure: Procedure) -> Result<T, ()> {
+    if let Procedure::Interpolate { progress } = procedure {
+        Ok(if progress < 0.5 { from.clone() } else { to.clone() })
+    } else {
+        Err(())
+    }
+}
+
+fn animate_with_discrete_fallback<T: Animate + Clone>(
+    from: &T,
+    to: &T,
+    procedure: Procedure
+) -> Result<T, ()> {
+    from.animate(to, procedure).or_else(|_| discrete(from, to, procedure))
+}
+
+impl Animate for TrackSize {
+    fn animate(&self, other: &Self, procedure: Procedure) -> Result<Self, ()> {
+        match (self, other) {
+            (&generics::TrackSize::Breadth(ref from),
+             &generics::TrackSize::Breadth(ref to)) => {
+                animate_with_discrete_fallback(from, to, procedure)
+                    .map(generics::TrackSize::Breadth)
+            },
+            (&generics::TrackSize::Minmax(ref from_min, ref from_max),
+             &generics::TrackSize::Minmax(ref to_min, ref to_max)) => {
+                Ok(generics::TrackSize::Minmax(
+                    animate_with_discrete_fallback(from_min, to_min, procedure)?,
+                    animate_with_discrete_fallback(from_max, to_max, procedure)?,
+                ))
+            },
+            (&generics::TrackSize::FitContent(ref from),
+             &generics::TrackSize::FitContent(ref to)) => {
+                animate_with_discrete_fallback(from, to, procedure)
+                    .map(generics::TrackSize::FitContent)
+            },
+            (_, _) => discrete(self, other, procedure),
+        }
+    }
+}
+
+impl Animate for generics::TrackRepeat<LengthPercentage, Integer>
+where
+    generics::RepeatCount<Integer>: PartialEq,
+{
+    fn animate(&self, other: &Self, procedure: Procedure) -> Result<Self, ()> {
+        // If the keyword, auto-fit/fill, is the same it can result in different
+        // number of tracks. For both auto-fit/fill, the number of columns isn't
+        // known until you do layout since it depends on the container size, item
+        // placement and other factors, so we cannot do the correct interpolation
+        // by computed values. Therefore, return Err(()) if it's keywords. If it
+        // is Number, we support animation only if the count is the same and the
+        // length of track_sizes is the same.
+        // https://github.com/w3c/csswg-drafts/issues/3503
+        match (&self.count, &other.count) {
+            (&generics::RepeatCount::Number(from),
+             &generics::RepeatCount::Number(to)) if from == to => (),
+            (_, _) => return Err(()),
+        }
+
+        // The length of track_sizes should be matched.
+        if self.track_sizes.len() != other.track_sizes.len() {
+            return Err(());
+        }
+
+        let count = self.count;
+        let track_sizes = self.track_sizes
+            .iter()
+            .zip(other.track_sizes.iter())
+            .map(|(a, b)| a.animate(b, procedure))
+            .collect::<Result<Vec<_>, _>>()?;
+
+        // The length of |line_names| is always 0 or N+1, where N is the length
+        // of |track_sizes|. Besides, <line-names> is always discrete.
+        let line_names = discrete(&self.line_names, &other.line_names, procedure)?;
+
+        Ok(generics::TrackRepeat { count, line_names, track_sizes })
+    }
+}
+
+impl Animate for TrackList {
+    // Based on https://github.com/w3c/csswg-drafts/issues/3201:
+    // 1. Check interpolation type per track, so we need to handle discrete animations
+    //    in TrackSize, so any Err(()) returned from TrackSize doesn't make all TrackSize
+    //    fallback to discrete animation.
+    // 2. line-names is always discrete.
+    fn animate(&self, other: &Self, procedure: Procedure) -> Result<Self, ()> {
+        if self.values.len() != other.values.len() {
+            return Err(());
+        }
+
+        if self.list_type != other.list_type {
+            return Err(());
+        }
+
+        // For now, repeat(auto-fill/auto-fit, ...) is not animatable. TrackRepeat will
+        // return Err(()) if we use keywords. Therefore, we can early return here to avoid
+        // traversing |values| in <auto-track-list>. This may be updated in the future.
+        // https://github.com/w3c/csswg-drafts/issues/3503
+        if let generics::TrackListType::Auto(_) = self.list_type {
+            return Err(());
+        }
+
+        let list_type = self.list_type;
+        let auto_repeat = self.auto_repeat.animate(&other.auto_repeat, procedure)?;
+        let values = self.values
+            .iter()
+            .zip(other.values.iter())
+            .map(|(a, b)| a.animate(b, procedure))
+            .collect::<Result<Vec<_>, _>>()?;
+        // The length of |line_names| is always 0 or N+1, where N is the length
+        // of |track_sizes|. Besides, <line-names> is always discrete.
+        let line_names = discrete(&self.line_names, &other.line_names, procedure)?;
+
+        Ok(TrackList { list_type, values, line_names, auto_repeat })
+    }
+}
+
+impl ComputeSquaredDistance for GridTemplateComponent {
+    #[inline]
+    fn compute_squared_distance(&self, _other: &Self) -> Result<SquaredDistance, ()> {
+        // TODO: Bug 1518585, we should implement ComputeSquaredDistance.
+        Err(())
+    }
+}
+
+impl ToAnimatedZero for GridTemplateComponent {
+    #[inline]
+    fn to_animated_zero(&self) -> Result<Self, ()> {
+        // It's not clear to get a zero grid track list based on the current definition
+        // of spec, so we return Err(()) directly.
+        Err(())
+    }
+}
--- a/servo/components/style/values/animated/mod.rs
+++ b/servo/components/style/values/animated/mod.rs
@@ -18,16 +18,17 @@ use crate::values::CSSFloat;
 use app_units::Au;
 use euclid::{Point2D, Size2D};
 use smallvec::SmallVec;
 use std::cmp;
 
 pub mod color;
 pub mod effects;
 mod font;
+mod grid;
 mod length;
 mod svg;
 pub mod transform;
 
 /// The category a property falls into for ordering purposes.
 ///
 /// https://drafts.csswg.org/web-animations/#calculating-computed-keyframes
 #[derive(Clone, Copy, Eq, Ord, PartialEq, PartialOrd)]
--- a/servo/components/style/values/generics/grid.rs
+++ b/servo/components/style/values/generics/grid.rs
@@ -146,16 +146,17 @@ impl Parse for GridLine<specified::Integ
 
         Ok(grid_line)
     }
 }
 
 #[allow(missing_docs)]
 #[cfg_attr(feature = "servo", derive(Deserialize, Serialize))]
 #[derive(
+    Animate,
     Clone,
     Copy,
     Debug,
     Eq,
     MallocSizeOf,
     Parse,
     PartialEq,
     SpecifiedValueInfo,
@@ -167,17 +168,26 @@ pub enum TrackKeyword {
     MaxContent,
     MinContent,
 }
 
 /// A track breadth for explicit grid track sizing. It's generic solely to
 /// avoid re-implementing it for the computed type.
 ///
 /// <https://drafts.csswg.org/css-grid/#typedef-track-breadth>
-#[derive(Clone, Debug, MallocSizeOf, PartialEq, SpecifiedValueInfo, ToComputedValue, ToCss)]
+#[derive(
+    Animate,
+    Clone,
+    Debug,
+    MallocSizeOf,
+    PartialEq,
+    SpecifiedValueInfo,
+    ToComputedValue,
+    ToCss,
+)]
 pub enum TrackBreadth<L> {
     /// The generic type is almost always a non-negative `<length-percentage>`
     Breadth(L),
     /// A flex fraction specified in `fr` units.
     #[css(dimension)]
     Fr(CSSFloat),
     /// One of the track-sizing keywords (`auto`, `min-content`, `max-content`)
     Keyword(TrackKeyword),
@@ -476,22 +486,31 @@ impl<L: Clone> TrackRepeat<L, specified:
                 track_sizes: self.track_sizes.clone(),
                 line_names: self.line_names.clone(),
             }
         }
     }
 }
 
 /// Track list values. Can be <track-size> or <track-repeat>
-#[derive(Clone, Debug, MallocSizeOf, PartialEq, SpecifiedValueInfo, ToComputedValue, ToCss)]
+#[derive(
+    Animate,
+    Clone,
+    Debug,
+    MallocSizeOf,
+    PartialEq,
+    SpecifiedValueInfo,
+    ToComputedValue,
+    ToCss
+)]
 pub enum TrackListValue<LengthPercentage, Integer> {
     /// A <track-size> value.
-    TrackSize(TrackSize<LengthPercentage>),
+    TrackSize(#[animation(field_bound)] TrackSize<LengthPercentage>),
     /// A <track-repeat> value.
-    TrackRepeat(TrackRepeat<LengthPercentage, Integer>),
+    TrackRepeat(#[animation(field_bound)] TrackRepeat<LengthPercentage, Integer>),
 }
 
 /// The type of a `<track-list>` as determined during parsing.
 ///
 /// <https://drafts.csswg.org/css-grid/#typedef-track-list>
 #[derive(Clone, Copy, Debug, MallocSizeOf, PartialEq, ToComputedValue)]
 pub enum TrackListType {
     /// [`<auto-track-list>`](https://drafts.csswg.org/css-grid/#typedef-auto-track-list)
@@ -687,23 +706,34 @@ impl ToCss for LineNameList {
 
         Ok(())
     }
 }
 
 /// Variants for `<grid-template-rows> | <grid-template-columns>`
 /// Subgrid deferred to Level 2 spec due to lack of implementation.
 /// But it's implemented in gecko, so we have to as well.
-#[derive(Clone, Debug, MallocSizeOf, PartialEq, SpecifiedValueInfo, ToComputedValue, ToCss)]
+#[derive(
+    Animate,
+    Clone,
+    Debug,
+    MallocSizeOf,
+    PartialEq,
+    SpecifiedValueInfo,
+    ToComputedValue,
+    ToCss
+)]
 pub enum GridTemplateComponent<L, I> {
     /// `none` value.
     None,
     /// The grid `<track-list>`
-    TrackList(#[compute(field_bound)] TrackList<L, I>),
+    TrackList(#[animation(field_bound)] #[compute(field_bound)] TrackList<L, I>),
     /// A `subgrid <line-name-list>?`
+    /// TODO: Support animations for this after subgrid is addressed in [grid-2] spec.
+    #[animation(error)]
     Subgrid(LineNameList),
 }
 
 impl<L, I> GridTemplateComponent<L, I> {
     /// Returns length of the <track-list>s <track-size>
     pub fn track_list_len(&self) -> usize {
         match *self {
             GridTemplateComponent::TrackList(ref tracklist) => tracklist.values.len(),
--- a/testing/web-platform/tests/web-animations/animation-model/animation-types/property-list.js
+++ b/testing/web-platform/tests/web-animations/animation-model/animation-types/property-list.js
@@ -674,28 +674,16 @@ const gCSSProperties = {
     ]
   },
   'grid-template-areas': {
     // https://drafts.csswg.org/css-template/#grid-template-areas
     types: [
       { type: 'discrete', options: [ [ '". . a b" ". .a b"', 'none' ] ] }
     ]
   },
-  'grid-template-columns': {
-    // https://drafts.csswg.org/css-template/#grid-template-columns
-    types: [
-      { type: 'discrete', options: [ [ '1px', '5px' ] ] }
-    ]
-  },
-  'grid-template-rows': {
-    // https://drafts.csswg.org/css-template/#grid-template-rows
-    types: [
-      { type: 'discrete', options: [ [ '1px', '5px' ] ] }
-    ]
-  },
   'height': {
     // https://drafts.csswg.org/css21/visudet.html#propdef-height
     types: [
     ]
   },
   'hyphens': {
     // https://drafts.csswg.org/css-text-3/#propdef-hyphens
     types: [