Bug 1472917 - Implement the even more forgiving interpolation rules for transform lists; r=hiro
authorBrian Birtles <birtles@gmail.com>
Fri, 19 Oct 2018 04:41:12 +0000
changeset 500534 660b4ee6c9133fb2fb9fee9d70cf0e8d9b04051a
parent 500533 e7aff1bbcd20a54914a046ead912bcf21cf86e76
child 500535 3e62b3523fb5f8af9996e88ff5e15d5edd4742fd
push id1864
push userffxbld-merge
push dateMon, 03 Dec 2018 15:51:40 +0000
treeherdermozilla-release@f040763d99ad [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewershiro
bugs1472917
milestone64.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 1472917 - Implement the even more forgiving interpolation rules for transform lists; r=hiro As discussed in: https://github.com/w3c/csswg-drafts/issues/927 with tentative spec text: https://github.com/w3c/csswg-drafts/pull/3215 Depends on D9184 Differential Revision: https://phabricator.services.mozilla.com/D9185
servo/components/style/properties/helpers/animated_properties.mako.rs
testing/web-platform/meta/css/css-transforms/animation/list-interpolation.html.ini
--- a/servo/components/style/properties/helpers/animated_properties.mako.rs
+++ b/servo/components/style/properties/helpers/animated_properties.mako.rs
@@ -1346,21 +1346,21 @@ fn is_matched_operation(first: &Computed
         (&TransformOperation::RotateX(..),
          &TransformOperation::RotateX(..)) |
         (&TransformOperation::RotateY(..),
          &TransformOperation::RotateY(..)) |
         (&TransformOperation::RotateZ(..),
          &TransformOperation::RotateZ(..)) |
         (&TransformOperation::Perspective(..),
          &TransformOperation::Perspective(..)) => true,
-        // we animate scale and translate operations against each other
+        // Match functions that have the same primitive transform function
         (a, b) if a.is_translate() && b.is_translate() => true,
         (a, b) if a.is_scale() && b.is_scale() => true,
         (a, b) if a.is_rotate() && b.is_rotate() => true,
-        // InterpolateMatrix and AccumulateMatrix are for mismatched transform.
+        // InterpolateMatrix and AccumulateMatrix are for mismatched transforms
         _ => false
     }
 }
 
 /// A 2d matrix for interpolation.
 #[derive(Clone, ComputeSquaredDistance, Copy, Debug)]
 #[cfg_attr(feature = "servo", derive(MallocSizeOf))]
 #[allow(missing_docs)]
@@ -2458,89 +2458,122 @@ impl Animate for ComputedTransform {
     fn animate(&self, other: &Self, procedure: Procedure) -> Result<Self, ()> {
         use std::borrow::Cow;
 
         if procedure == Procedure::Add {
             let result = self.0.iter().chain(&other.0).cloned().collect::<Vec<_>>();
             return Ok(Transform(result));
         }
 
-        // https://drafts.csswg.org/css-transforms-1/#transform-transform-neutral-extend-animation
-        fn match_operations_if_possible<'a>(
-            this: &mut Cow<'a, Vec<ComputedTransformOperation>>,
-            other: &mut Cow<'a, Vec<ComputedTransformOperation>>,
-        ) -> bool {
-            if !this.iter().zip(other.iter()).all(|(this, other)| is_matched_operation(this, other)) {
-                return false;
-            }
+        let this = Cow::Borrowed(&self.0);
+        let other = Cow::Borrowed(&other.0);
+
+        // Interpolate the common prefix
+        let mut result = this
+            .iter()
+            .zip(other.iter())
+            .take_while(|(this, other)| is_matched_operation(this, other))
+            .map(|(this, other)| this.animate(other, procedure))
+            .collect::<Result<Vec<_>, _>>()?;
 
-            if this.len() == other.len() {
-                return true;
-            }
-
-            let (shorter, longer) =
-                if this.len() < other.len() {
-                    (this.to_mut(), other)
-                } else {
-                    (other.to_mut(), this)
-                };
+        // Deal with the remainders
+        let this_remainder = if this.len() > result.len() {
+            Some(&this[result.len()..])
+        } else {
+            None
+        };
+        let other_remainder = if other.len() > result.len() {
+            Some(&other[result.len()..])
+        } else {
+            None
+        };
 
-            shorter.reserve(longer.len());
-            for op in longer.iter().skip(shorter.len()) {
-                shorter.push(op.to_animated_zero().unwrap());
-            }
+        match (this_remainder, other_remainder) {
+            // If there is a remainder from *both* lists we must have had mismatched functions.
+            // => Add the remainders to a suitable ___Matrix function.
+            (Some(this_remainder), Some(other_remainder)) => match procedure {
+                Procedure::Add => {
+                    debug_assert!(false, "Should have already dealt with add by the point");
+                    return Err(());
+                }
+                Procedure::Interpolate { progress } => {
+                    result.push(TransformOperation::InterpolateMatrix {
+                        from_list: Transform(this_remainder.to_vec()),
+                        to_list: Transform(other_remainder.to_vec()),
+                        progress: Percentage(progress as f32),
+                    });
+                }
+                Procedure::Accumulate { count } => {
+                    result.push(TransformOperation::AccumulateMatrix {
+                        from_list: Transform(this_remainder.to_vec()),
+                        to_list: Transform(other_remainder.to_vec()),
+                        count: cmp::min(count, i32::max_value() as u64) as i32,
+                    });
+                }
+            },
+            // If there is a remainder from just one list, then one list must be shorter but
+            // completely match the type of the corresponding functions in the longer list.
+            // => Interpolate the remainder with identity transforms.
+            (Some(remainder), None) | (None, Some(remainder)) => {
+                let fill_right = this_remainder.is_some();
+                result.append(
+                    &mut remainder
+                        .iter()
+                        .map(|transform| {
+                            let identity = transform.to_animated_zero().unwrap();
 
-            // The resulting operations won't be matched regardless if the
-            // extended component is already InterpolateMatrix /
-            // AccumulateMatrix.
-            //
-            // Otherwise they should be matching operations all the time.
-            let already_mismatched = matches!(
-                longer[0],
-                TransformOperation::InterpolateMatrix { .. } |
-                TransformOperation::AccumulateMatrix { .. }
-            );
+                            match transform {
+                                // We can't interpolate/accumulate ___Matrix types directly with a
+                                // matrix. Instead we need to wrap it in another ___Matrix type.
+                                TransformOperation::AccumulateMatrix { .. }
+                                | TransformOperation::InterpolateMatrix { .. } => {
+                                    let transform_list = Transform(vec![transform.clone()]);
+                                    let identity_list = Transform(vec![identity]);
+                                    let (from_list, to_list) = if fill_right {
+                                        (transform_list, identity_list)
+                                    } else {
+                                        (identity_list, transform_list)
+                                    };
 
-            debug_assert_eq!(
-                !already_mismatched,
-                longer.iter().zip(shorter.iter()).all(|(this, other)| is_matched_operation(this, other)),
-                "ToAnimatedZero should generate matched operations"
-            );
-
-            !already_mismatched
+                                    match procedure {
+                                        Procedure::Add => Err(()),
+                                        Procedure::Interpolate { progress } => {
+                                            Ok(TransformOperation::InterpolateMatrix {
+                                                from_list,
+                                                to_list,
+                                                progress: Percentage(progress as f32),
+                                            })
+                                        }
+                                        Procedure::Accumulate { count } => {
+                                            Ok(TransformOperation::AccumulateMatrix {
+                                                from_list,
+                                                to_list,
+                                                count: cmp::min(count, i32::max_value() as u64)
+                                                    as i32,
+                                            })
+                                        }
+                                    }
+                                }
+                                _ => {
+                                    let (lhs, rhs) = if fill_right {
+                                        (transform, &identity)
+                                    } else {
+                                        (&identity, transform)
+                                    };
+                                    lhs.animate(rhs, procedure)
+                                }
+                            }
+                        })
+                        .collect::<Result<Vec<_>, _>>()?,
+                );
+            }
+            (None, None) => {}
         }
 
-        let mut this = Cow::Borrowed(&self.0);
-        let mut other = Cow::Borrowed(&other.0);
-
-        if match_operations_if_possible(&mut this, &mut other) {
-            return Ok(Transform(
-                this.iter().zip(other.iter())
-                    .map(|(this, other)| this.animate(other, procedure))
-                    .collect::<Result<Vec<_>, _>>()?
-            ));
-        }
-
-        match procedure {
-            Procedure::Add => Err(()),
-            Procedure::Interpolate { progress } => {
-                Ok(Transform(vec![TransformOperation::InterpolateMatrix {
-                    from_list: Transform(this.into_owned()),
-                    to_list: Transform(other.into_owned()),
-                    progress: Percentage(progress as f32),
-                }]))
-            },
-            Procedure::Accumulate { count } => {
-                Ok(Transform(vec![TransformOperation::AccumulateMatrix {
-                    from_list: Transform(this.into_owned()),
-                    to_list: Transform(other.into_owned()),
-                    count: cmp::min(count, i32::max_value() as u64) as i32,
-                }]))
-            },
-        }
+        Ok(Transform(result))
     }
 }
 
 // This might not be the most useful definition of distance. It might be better, for example,
 // to trace the distance travelled by a point as its transform is interpolated between the two
 // lists. That, however, proves to be quite complicated so we take a simple approach for now.
 // See https://bugzilla.mozilla.org/show_bug.cgi?id=1318591#c0.
 impl ComputeSquaredDistance for ComputedTransformOperation {
deleted file mode 100644
--- a/testing/web-platform/meta/css/css-transforms/animation/list-interpolation.html.ini
+++ /dev/null
@@ -1,3 +0,0 @@
-[list-interpolation.html]
-  [Animation between "rotate(0deg) translate(100px)" and "rotate(720deg) scale(2) translate(200px)" at progress 0.25]
-    expected: FAIL