Bug 1309752: Animate logical properties. r?birtles draft
authorEmilio Cobos Álvarez <emilio@crisal.io>
Fri, 06 Jul 2018 05:19:10 +0200
changeset 815643 f289db27f31ace8a0211611d7bf012e870a815d9
parent 815642 a8fcee647805fc363885125bb9e8e5be54c911e9
child 815644 7b4dd75f61b2d5884f105d4a9b6aa93ec022d00e
push id115591
push userbmo:emilio@crisal.io
push dateMon, 09 Jul 2018 16:15:39 +0000
reviewersbirtles
bugs1309752
milestone63.0a1
Bug 1309752: Animate logical properties. r?birtles The setup is that AnimationValue only contains physical properties, and we physicalize when building keyframes and transitions. MozReview-Commit-ID: 9dI20N0LFrk
layout/style/ServoBindingList.h
layout/style/ServoBindings.cpp
layout/style/ServoStyleSet.cpp
layout/style/ServoStyleSet.h
layout/style/nsAnimationManager.cpp
layout/style/nsTransitionManager.cpp
servo/components/style/gecko/wrapper.rs
servo/components/style/properties/data.py
servo/components/style/properties/declaration_block.rs
servo/components/style/properties/helpers/animated_properties.mako.rs
servo/ports/geckolib/glue.rs
--- a/layout/style/ServoBindingList.h
+++ b/layout/style/ServoBindingList.h
@@ -131,16 +131,17 @@ SERVO_BINDING_FUNC(Servo_StyleSet_SetAut
                    RawServoStyleSetBorrowed set,
                    bool author_style_disabled)
 SERVO_BINDING_FUNC(Servo_StyleSet_NoteStyleSheetsChanged, void,
                    RawServoStyleSetBorrowed set,
                    mozilla::OriginFlags changed_origins)
 SERVO_BINDING_FUNC(Servo_StyleSet_GetKeyframesForName, bool,
                    RawServoStyleSetBorrowed set,
                    RawGeckoElementBorrowed element,
+                   ComputedStyleBorrowed style,
                    nsAtom* name,
                    nsTimingFunctionBorrowed timing_function,
                    RawGeckoKeyframeListBorrowedMut keyframe_list)
 SERVO_BINDING_FUNC(Servo_StyleSet_GetFontFaceRules, void,
                    RawServoStyleSetBorrowed set,
                    RawGeckoFontFaceRuleListBorrowedMut list)
 SERVO_BINDING_FUNC(Servo_StyleSet_GetCounterStyleRule,
                    const RawServoCounterStyleRule*,
--- a/layout/style/ServoBindings.cpp
+++ b/layout/style/ServoBindings.cpp
@@ -1994,17 +1994,19 @@ Gecko_GetOrCreateFinalKeyframe(nsTArray<
                              KeyframeInsertPosition::LastForOffset);
 }
 
 PropertyValuePair*
 Gecko_AppendPropertyValuePair(nsTArray<PropertyValuePair>* aProperties,
                               nsCSSPropertyID aProperty)
 {
   MOZ_ASSERT(aProperties);
-  return aProperties->AppendElement(PropertyValuePair {aProperty});
+  MOZ_ASSERT(aProperty == eCSSPropertyExtra_variable ||
+             !nsCSSProps::PropHasFlags(aProperty, CSSPropFlags::IsLogical));
+  return aProperties->AppendElement(PropertyValuePair { aProperty });
 }
 
 void
 Gecko_ResetStyleCoord(nsStyleUnit* aUnit, nsStyleUnion* aValue)
 {
   nsStyleCoord::Reset(*aUnit, *aValue);
 }
 
--- a/layout/style/ServoStyleSet.cpp
+++ b/layout/style/ServoStyleSet.cpp
@@ -1154,23 +1154,25 @@ ServoStyleSet::AssertTreeIsClean()
   while (Element* root = iter.GetNextStyleRoot()) {
     Servo_AssertTreeIsClean(root);
   }
 }
 #endif
 
 bool
 ServoStyleSet::GetKeyframesForName(const Element& aElement,
+                                   const ComputedStyle& aStyle,
                                    nsAtom* aName,
                                    const nsTimingFunction& aTimingFunction,
                                    nsTArray<Keyframe>& aKeyframes)
 {
   MOZ_ASSERT(!StylistNeedsUpdate());
   return Servo_StyleSet_GetKeyframesForName(mRawSet.get(),
                                             &aElement,
+                                            &aStyle,
                                             aName,
                                             &aTimingFunction,
                                             &aKeyframes);
 }
 
 nsTArray<ComputedKeyframeValues>
 ServoStyleSet::GetComputedKeyframeValuesFor(
   const nsTArray<Keyframe>& aKeyframes,
--- a/layout/style/ServoStyleSet.h
+++ b/layout/style/ServoStyleSet.h
@@ -331,17 +331,18 @@ public:
    * Resolve style for the given element, and return it as a
    * ComputedStyle.
    *
    * FIXME(emilio): Is there a point in this after bug 1367904?
    */
   inline already_AddRefed<ComputedStyle>
     ResolveServoStyle(dom::Element* aElement);
 
-  bool GetKeyframesForName(const dom::Element& aElement,
+  bool GetKeyframesForName(const dom::Element&,
+                           const ComputedStyle&,
                            nsAtom* aName,
                            const nsTimingFunction& aTimingFunction,
                            nsTArray<Keyframe>& aKeyframes);
 
   nsTArray<ComputedKeyframeValues>
   GetComputedKeyframeValuesFor(const nsTArray<Keyframe>& aKeyframes,
                                dom::Element* aElement,
                                const mozilla::ComputedStyle* aStyle);
--- a/layout/style/nsAnimationManager.cpp
+++ b/layout/style/nsAnimationManager.cpp
@@ -354,22 +354,22 @@ public:
   }
 
   bool BuildKeyframes(const Element& aElement,
                       nsPresContext* aPresContext,
                       nsAtom* aName,
                       const nsTimingFunction& aTimingFunction,
                       nsTArray<Keyframe>& aKeyframes)
   {
-    ServoStyleSet* styleSet = aPresContext->StyleSet();
-    MOZ_ASSERT(styleSet);
-    return styleSet->GetKeyframesForName(aElement,
-                                         aName,
-                                         aTimingFunction,
-                                         aKeyframes);
+    return aPresContext->StyleSet()->GetKeyframesForName(
+        aElement,
+        *mComputedStyle,
+        aName,
+        aTimingFunction,
+        aKeyframes);
   }
   void SetKeyframes(KeyframeEffect& aEffect, nsTArray<Keyframe>&& aKeyframes)
   {
     aEffect.SetKeyframes(std::move(aKeyframes), mComputedStyle);
   }
 
   // Currently all the animation building code in this file is based on
   // assumption that creating and removing animations should *not* trigger
@@ -641,23 +641,23 @@ nsAnimationManager::DoUpdateAnimations(
       aStyleDisplay.mAnimations[0].GetName() == nsGkAtoms::_empty) {
     return;
   }
 
   nsAutoAnimationMutationBatch mb(aTarget.mElement->OwnerDoc());
 
   // Build the updated animations list, extracting matching animations from
   // the existing collection as we go.
-  OwningCSSAnimationPtrArray newAnimations;
-  newAnimations = BuildAnimations(mPresContext,
-                                  aTarget,
-                                  aStyleDisplay,
-                                  aBuilder,
-                                  collection,
-                                  mMaybeReferencedAnimations);
+  OwningCSSAnimationPtrArray newAnimations =
+    BuildAnimations(mPresContext,
+                    aTarget,
+                    aStyleDisplay,
+                    aBuilder,
+                    collection,
+                    mMaybeReferencedAnimations);
 
   if (newAnimations.IsEmpty()) {
     if (collection) {
       collection->Destroy();
     }
     return;
   }
 
--- a/layout/style/nsTransitionManager.cpp
+++ b/layout/style/nsTransitionManager.cpp
@@ -538,25 +538,28 @@ nsTransitionManager::DoUpdateTransitions
         if (property == eCSSPropertyExtra_no_properties ||
             property == eCSSPropertyExtra_variable ||
             property == eCSSProperty_UNKNOWN) {
           // Nothing to do, but need to exclude this from cases below.
         } else if (property == eCSSPropertyExtra_all_properties) {
           for (nsCSSPropertyID p = nsCSSPropertyID(0);
                p < eCSSProperty_COUNT_no_shorthands;
                p = nsCSSPropertyID(p + 1)) {
+            p = nsCSSProps::Physicalize(p, aNewStyle);
             allTransitionProperties.AddProperty(p);
           }
         } else if (nsCSSProps::IsShorthand(property)) {
           CSSPROPS_FOR_SHORTHAND_SUBPROPERTIES(
               subprop, property, CSSEnabledState::eForAllContent) {
-            allTransitionProperties.AddProperty(*subprop);
+            auto p = nsCSSProps::Physicalize(*subprop, aNewStyle);
+            allTransitionProperties.AddProperty(p);
           }
         } else {
-          allTransitionProperties.AddProperty(property);
+          allTransitionProperties.AddProperty(
+            nsCSSProps::Physicalize(property, aNewStyle));
         }
       }
     }
 
     OwningCSSTransitionPtrArray& animations = aElementTransitions->mAnimations;
     size_t i = animations.Length();
     MOZ_ASSERT(i != 0, "empty transitions list?");
     AnimationValue currentValue;
@@ -650,23 +653,25 @@ nsTransitionManager::ConsiderInitiatingT
   dom::Element* aElement,
   CSSPseudoElementType aPseudoType,
   CSSTransitionCollection*& aElementTransitions,
   const ComputedStyle& aOldStyle,
   const ComputedStyle& aNewStyle,
   nsCSSPropertyIDSet& aPropertiesChecked)
 {
   // IsShorthand itself will assert if aProperty is not a property.
-  MOZ_ASSERT(!nsCSSProps::IsShorthand(aProperty),
-             "property out of range");
+  MOZ_ASSERT(!nsCSSProps::IsShorthand(aProperty), "property out of range");
   NS_ASSERTION(!aElementTransitions ||
                aElementTransitions->mElement == aElement, "Element mismatch");
 
-  // A later item in transition-property already specified a transition for this
-  // property, so we ignore this one.
+  aProperty = nsCSSProps::Physicalize(aProperty, aNewStyle);
+
+  // A later item in transition-property already specified a transition for
+  // this property, so we ignore this one.
+  //
   // See http://lists.w3.org/Archives/Public/www-style/2009Aug/0109.html .
   if (aPropertiesChecked.HasProperty(aProperty)) {
     return false;
   }
 
   aPropertiesChecked.AddProperty(aProperty);
 
   if (!IsTransitionable(aProperty)) {
--- a/servo/components/style/gecko/wrapper.rs
+++ b/servo/components/style/gecko/wrapper.rs
@@ -877,30 +877,32 @@ impl<'le> GeckoElement<'le> {
 
         for i in 0..collection_length {
             let raw_end_value = unsafe { Gecko_ElementTransitions_EndValueAt(self.0, i) };
 
             let end_value = AnimationValue::arc_from_borrowed(&raw_end_value)
                 .expect("AnimationValue not found in ElementTransitions");
 
             let property = end_value.id();
+            debug_assert!(!property.is_logical());
             map.insert(property, end_value.clone_arc());
         }
         map
     }
 
     fn needs_transitions_update_per_property(
         &self,
         longhand_id: LonghandId,
         combined_duration: f32,
         before_change_style: &ComputedValues,
         after_change_style: &ComputedValues,
         existing_transitions: &FnvHashMap<LonghandId, Arc<AnimationValue>>,
     ) -> bool {
         use values::animated::{Animate, Procedure};
+        debug_assert!(!longhand_id.is_logical());
 
         // If there is an existing transition, update only if the end value
         // differs.
         //
         // If the end value has not changed, we should leave the currently
         // running transition as-is since we don't want to interrupt its timing
         // function.
         if let Some(ref existing) = existing_transitions.get(&longhand_id) {
@@ -1652,16 +1654,18 @@ impl<'le> TElement for GeckoElement<'le>
             // We don't need to update transition for none/custom properties.
             if is_none_or_custom_property(property) {
                 continue;
             }
 
             let transition_property: TransitionProperty = property.into();
 
             let mut property_check_helper = |property: LonghandId| -> bool {
+                let property =
+                    property.to_physical(after_change_style.writing_mode);
                 transitions_to_keep.insert(property);
                 self.needs_transitions_update_per_property(
                     property,
                     combined_duration,
                     before_change_style,
                     after_change_style,
                     &existing_transitions,
                 )
--- a/servo/components/style/properties/data.py
+++ b/servo/components/style/properties/data.py
@@ -215,22 +215,16 @@ class Longhand(object):
             raise TypeError("animation_value_type should be specified for (" + name + ")")
         self.animation_value_type = animation_value_type
 
         self.animatable = animation_value_type != "none"
         self.transitionable = animation_value_type != "none" \
             and animation_value_type != "discrete"
         self.is_animatable_with_computed_value = animation_value_type == "ComputedValue" \
             or animation_value_type == "discrete"
-        if self.logical:
-            # Logical properties will be animatable (i.e. the animation type is
-            # discrete). For now, it is still non-animatable.
-            self.animatable = False
-            self.transitionable = False
-            self.animation_value_type = None
 
         # See compute_damage for the various values this can take
         self.servo_restyle_damage = servo_restyle_damage
 
     @staticmethod
     def type():
         return "longhand"
 
--- a/servo/components/style/properties/declaration_block.rs
+++ b/servo/components/style/properties/declaration_block.rs
@@ -118,45 +118,62 @@ impl<'a> DoubleEndedIterator for Declara
     #[inline(always)]
     fn next_back(&mut self) -> Option<Self::Item> {
         self.iter.next_back().map(|(decl, important)|
             (decl, if important { Importance::Important } else { Importance::Normal }))
     }
 }
 
 /// Iterator over `PropertyDeclaration` for Importance::Normal.
+///
+/// TODO(emilio): This should be replaced by `impl Trait`, returning a
+/// filter()ed iterator when available instead, and all the boilerplate below
+/// should go.
 pub struct NormalDeclarationIterator<'a>(DeclarationImportanceIterator<'a>);
 
 impl<'a> NormalDeclarationIterator<'a> {
-    /// Constructor.
     #[inline]
-    pub fn new(declarations: &'a [PropertyDeclaration], important: &'a SmallBitVec) -> Self {
+    fn new(declarations: &'a [PropertyDeclaration], important: &'a SmallBitVec) -> Self {
         NormalDeclarationIterator(
             DeclarationImportanceIterator::new(declarations, important)
         )
     }
 }
 
 impl<'a> Iterator for NormalDeclarationIterator<'a> {
     type Item = &'a PropertyDeclaration;
 
+    #[inline]
     fn next(&mut self) -> Option<Self::Item> {
         loop {
             let (decl, importance) = self.0.iter.next()?;
             if !importance {
                 return Some(decl);
             }
         }
     }
 
+    #[inline]
     fn size_hint(&self) -> (usize, Option<usize>) {
         self.0.iter.size_hint()
     }
 }
 
+impl<'a> DoubleEndedIterator for NormalDeclarationIterator<'a> {
+    #[inline]
+    fn next_back(&mut self) -> Option<Self::Item> {
+        loop {
+            let (decl, importance) = self.0.iter.next_back()?;
+            if !importance {
+                return Some(decl);
+            }
+        }
+    }
+}
+
 /// Iterator for AnimationValue to be generated from PropertyDeclarationBlock.
 pub struct AnimationValueIterator<'a, 'cx, 'cx_a:'cx> {
     iter: NormalDeclarationIterator<'a>,
     context: &'cx mut Context<'cx_a>,
     default_values: &'a ComputedValues,
     /// Custom properties in a keyframe if exists.
     extra_custom_properties: Option<&'a Arc<::custom_properties::CustomPropertiesMap>>,
 }
--- a/servo/components/style/properties/helpers/animated_properties.mako.rs
+++ b/servo/components/style/properties/helpers/animated_properties.mako.rs
@@ -1,16 +1,16 @@
 /* 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 http://mozilla.org/MPL/2.0/. */
 
 <%namespace name="helpers" file="/helpers.mako.rs" />
 
 <%
-    from data import to_idl_name, SYSTEM_FONT_LONGHANDS
+    from data import to_idl_name, SYSTEM_FONT_LONGHANDS, to_camel_case
     from itertools import groupby
 %>
 
 #[cfg(feature = "gecko")] use gecko_bindings::bindings::RawServoAnimationValueMap;
 #[cfg(feature = "gecko")] use gecko_bindings::structs::RawGeckoGfxMatrix4x4;
 #[cfg(feature = "gecko")] use gecko_bindings::structs::nsCSSPropertyID;
 #[cfg(feature = "gecko")] use gecko_bindings::sugar::ownership::{HasFFI, HasSimpleFFI};
 use itertools::{EitherOrBoth, Itertools};
@@ -109,57 +109,57 @@ pub fn nscsspropertyid_is_transitionable
 }
 
 /// An animated property interpolation between two computed values for that
 /// property.
 #[derive(Clone, Debug, PartialEq)]
 #[cfg_attr(feature = "servo", derive(MallocSizeOf))]
 pub enum AnimatedProperty {
     % for prop in data.longhands:
-        % if prop.animatable:
+        % if prop.animatable and not prop.logical:
             <%
                 value_type = "longhands::{}::computed_value::T".format(prop.ident)
                 if not prop.is_animatable_with_computed_value:
                     value_type = "<{} as ToAnimatedValue>::AnimatedValue".format(value_type)
             %>
             /// ${prop.name}
             ${prop.camel_case}(${value_type}, ${value_type}),
         % endif
     % endfor
 }
 
 impl AnimatedProperty {
     /// Get the name of this property.
     pub fn name(&self) -> &'static str {
         match *self {
             % for prop in data.longhands:
-                % if prop.animatable:
+                % if prop.animatable and not prop.logical:
                     AnimatedProperty::${prop.camel_case}(..) => "${prop.name}",
                 % endif
             % endfor
         }
     }
 
     /// Whether this interpolation does animate, that is, whether the start and
     /// end values are different.
     pub fn does_animate(&self) -> bool {
         match *self {
             % for prop in data.longhands:
-                % if prop.animatable:
+                % if prop.animatable and not prop.logical:
                     AnimatedProperty::${prop.camel_case}(ref from, ref to) => from != to,
                 % endif
             % endfor
         }
     }
 
     /// Whether an animated property has the same end value as another.
     pub fn has_the_same_end_value_as(&self, other: &Self) -> bool {
         match (self, other) {
             % for prop in data.longhands:
-                % if prop.animatable:
+                % if prop.animatable and not prop.logical:
                     (&AnimatedProperty::${prop.camel_case}(_, ref this_end_value),
                      &AnimatedProperty::${prop.camel_case}(_, ref other_end_value)) => {
                         this_end_value == other_end_value
                     }
                 % endif
             % endfor
             _ => false,
         }
@@ -168,17 +168,17 @@ impl AnimatedProperty {
     /// Update `style` with the proper computed style corresponding to this
     /// animation at `progress`.
     #[cfg_attr(feature = "gecko", allow(unused))]
     pub fn update(&self, style: &mut ComputedValues, progress: f64) {
         #[cfg(feature = "servo")]
         {
             match *self {
                 % for prop in data.longhands:
-                % if prop.animatable:
+                % if prop.animatable and not prop.logical:
                     AnimatedProperty::${prop.camel_case}(ref from, ref to) => {
                         // https://drafts.csswg.org/web-animations/#discrete-animation-type
                         % if prop.animation_value_type == "discrete":
                             let value = if progress < 0.5 { from.clone() } else { to.clone() };
                         % else:
                             let value = match from.animate(to, Procedure::Interpolate { progress }) {
                                 Ok(value) => value,
                                 Err(()) => return,
@@ -198,19 +198,22 @@ impl AnimatedProperty {
 
     /// Get an animatable value from a transition-property, an old style, and a
     /// new style.
     pub fn from_longhand(
         property: LonghandId,
         old_style: &ComputedValues,
         new_style: &ComputedValues,
     ) -> Option<AnimatedProperty> {
+        // FIXME(emilio): Handle the case where old_style and new_style's
+        // writing mode differ.
+        let property = property.to_physical(new_style.writing_mode);
         Some(match property {
             % for prop in data.longhands:
-            % if prop.animatable:
+            % if prop.animatable and not prop.logical:
                 LonghandId::${prop.camel_case} => {
                     let old_computed = old_style.clone_${prop.ident}();
                     let new_computed = new_style.clone_${prop.ident}();
                     AnimatedProperty::${prop.camel_case}(
                     % if prop.is_animatable_with_computed_value:
                         old_computed,
                         new_computed,
                     % else:
@@ -249,31 +252,34 @@ unsafe impl HasSimpleFFI for AnimationVa
 ///
 /// FIXME: We need to add a path for custom properties, but that's trivial after
 /// this (is a similar path to that of PropertyDeclaration).
 #[cfg_attr(feature = "servo", derive(MallocSizeOf))]
 #[derive(Debug)]
 #[repr(u16)]
 pub enum AnimationValue {
     % for prop in data.longhands:
-    % if prop.animatable:
+    % if prop.animatable and not prop.logical:
     /// `${prop.name}`
     ${prop.camel_case}(${prop.animated_type()}),
     % else:
     /// `${prop.name}` (not animatable)
     ${prop.camel_case}(Void),
     % endif
     % endfor
 }
 
 <%
     animated = []
     unanimated = []
+    animated_with_logical = []
     for prop in data.longhands:
         if prop.animatable:
+            animated_with_logical.append(prop)
+        if prop.animatable and not prop.logical:
             animated.append(prop)
         else:
             unanimated.append(prop)
 %>
 
 #[repr(C)]
 struct AnimationValueVariantRepr<T> {
     tag: u16,
@@ -365,17 +371,17 @@ impl PartialEq for AnimationValue {
 
 impl AnimationValue {
     /// Returns the longhand id this animated value corresponds to.
     #[inline]
     pub fn id(&self) -> LonghandId {
         let id = unsafe { *(self as *const _ as *const LonghandId) };
         debug_assert_eq!(id, match *self {
             % for prop in data.longhands:
-            % if prop.animatable:
+            % if prop.animatable and not prop.logical:
             AnimationValue::${prop.camel_case}(..) => LonghandId::${prop.camel_case},
             % else:
             AnimationValue::${prop.camel_case}(void) => void::unreachable(void),
             % endif
             % endfor
         });
         id
     }
@@ -439,27 +445,28 @@ impl AnimationValue {
                 x.boxed,
                 not x.is_animatable_with_computed_value,
                 x.style_struct.inherited,
                 x.ident in SYSTEM_FONT_LONGHANDS and product == "gecko",
             )
         %>
 
         let animatable = match *decl {
-            % for (specified_ty, ty, boxed, to_animated, inherit, system), props in groupby(animated, key=keyfunc):
+            % for (specified_ty, ty, boxed, to_animated, inherit, system), props in groupby(animated_with_logical, key=keyfunc):
             ${" |\n".join("PropertyDeclaration::{}(ref value)".format(prop.camel_case) for prop in props)} => {
                 let decl_repr = unsafe {
                     &*(decl as *const _ as *const PropertyDeclarationVariantRepr<${specified_ty}>)
                 };
+                let longhand_id = unsafe {
+                    *(&decl_repr.tag as *const u16 as *const LonghandId)
+                };
                 % if inherit:
                 context.for_non_inherited_property = None;
                 % else:
-                context.for_non_inherited_property = unsafe {
-                    Some(*(&decl_repr.tag as *const u16 as *const LonghandId))
-                };
+                context.for_non_inherited_property = Some(longhand_id);
                 % endif
                 % if system:
                 if let Some(sf) = value.get_system() {
                     longhands::system_font::resolve_system_font(sf, context)
                 }
                 % endif
                 % if boxed:
                 let value = (**value).to_computed_value(context);
@@ -470,51 +477,67 @@ impl AnimationValue {
                 let value = value.to_animated_value();
                 % endif
 
                 unsafe {
                     let mut out = mem::uninitialized();
                     ptr::write(
                         &mut out as *mut _ as *mut AnimationValueVariantRepr<${ty}>,
                         AnimationValueVariantRepr {
-                            tag: decl_repr.tag,
+                            tag: longhand_id.to_physical(context.builder.writing_mode) as u16,
                             value,
                         },
                     );
                     out
                 }
             }
             % endfor
             PropertyDeclaration::CSSWideKeyword(ref declaration) => {
                 match declaration.id {
                     // We put all the animatable properties first in the hopes
                     // that it might increase match locality.
                     % for prop in data.longhands:
                     % if prop.animatable:
                     LonghandId::${prop.camel_case} => {
                         let style_struct = match declaration.keyword {
                             % if not prop.style_struct.inherited:
-                                CSSWideKeyword::Unset |
+                            CSSWideKeyword::Unset |
                             % endif
                             CSSWideKeyword::Initial => {
                                 initial.get_${prop.style_struct.name_lower}()
                             },
                             % if prop.style_struct.inherited:
-                                CSSWideKeyword::Unset |
+                            CSSWideKeyword::Unset |
                             % endif
                             CSSWideKeyword::Inherit => {
                                 context.builder
                                        .get_parent_${prop.style_struct.name_lower}()
                             },
                         };
-                        let computed = style_struct.clone_${prop.ident}();
+                        let computed = style_struct
+                        % if prop.logical:
+                            .clone_${prop.ident}(context.builder.writing_mode);
+                        % else:
+                            .clone_${prop.ident}();
+                        % endif
+
                         % if not prop.is_animatable_with_computed_value:
                         let computed = computed.to_animated_value();
                         % endif
-                        AnimationValue::${prop.camel_case}(computed)
+
+                        % if prop.logical:
+                        let wm = context.builder.writing_mode;
+                        <%helpers:logical_setter_helper name="${prop.name}">
+                        <%def name="inner(physical_ident)">
+                            AnimationValue::${to_camel_case(physical_ident)}(computed)
+                        </%def>
+                        </%helpers:logical_setter_helper>
+                        % else:
+                            AnimationValue::${prop.camel_case}(computed)
+                        % endif
                     },
                     % endif
                     % endfor
                     % for prop in data.longhands:
                     % if not prop.animatable:
                     LonghandId::${prop.camel_case} => return None,
                     % endif
                     % endfor
@@ -543,19 +566,20 @@ impl AnimationValue {
         Some(animatable)
     }
 
     /// Get an AnimationValue for an AnimatableLonghand from a given computed values.
     pub fn from_computed_values(
         property: LonghandId,
         style: &ComputedValues,
     ) -> Option<Self> {
+        let property = property.to_physical(style.writing_mode);
         Some(match property {
             % for prop in data.longhands:
-            % if prop.animatable:
+            % if prop.animatable and not prop.logical:
             LonghandId::${prop.camel_case} => {
                 let computed = style.clone_${prop.ident}();
                 AnimationValue::${prop.camel_case}(
                 % if prop.is_animatable_with_computed_value:
                     computed
                 % else:
                     computed.to_animated_value()
                 % endif
@@ -651,17 +675,17 @@ impl ComputeSquaredDistance for Animatio
     }
 }
 
 impl ToAnimatedZero for AnimationValue {
     #[inline]
     fn to_animated_zero(&self) -> Result<Self, ()> {
         match *self {
             % for prop in data.longhands:
-            % if prop.animatable and prop.animation_value_type != "discrete":
+            % if prop.animatable and not prop.logical and prop.animation_value_type != "discrete":
             AnimationValue::${prop.camel_case}(ref base) => {
                 Ok(AnimationValue::${prop.camel_case}(base.to_animated_zero()?))
             },
             % endif
             % endfor
             _ => Err(()),
         }
     }
@@ -2919,53 +2943,77 @@ impl ToAnimatedZero for AnimatedFilter {
             % if product == "gecko":
             Filter::DropShadow(ref this) => Ok(Filter::DropShadow(this.to_animated_zero()?)),
             % endif
             _ => Err(()),
         }
     }
 }
 
-/// A comparator to sort PropertyIds such that longhands are sorted before shorthands,
-/// shorthands with fewer components are sorted before shorthands with more components,
-/// and otherwise shorthands are sorted by IDL name as defined by [Web Animations][property-order].
+/// 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)]
+enum PropertyCategory {
+    Custom,
+    PhysicalLonghand,
+    LogicalLonghand,
+    Shorthand,
+}
+
+impl PropertyCategory {
+    fn of(id: &PropertyId) -> Self {
+        match *id {
+            PropertyId::Shorthand(..) |
+            PropertyId::ShorthandAlias(..) => PropertyCategory::Shorthand,
+            PropertyId::Longhand(id) |
+            PropertyId::LonghandAlias(id, ..) => {
+                if id.is_logical() {
+                    PropertyCategory::LogicalLonghand
+                } else {
+                    PropertyCategory::PhysicalLonghand
+                }
+            }
+            PropertyId::Custom(..) => PropertyCategory::Custom,
+        }
+    }
+}
+
+/// A comparator to sort PropertyIds such that physical longhands are sorted
+/// before logical longhands and shorthands, shorthands with fewer components
+/// are sorted before shorthands with more components, and otherwise shorthands
+/// are sorted by IDL name as defined by [Web Animations][property-order].
 ///
 /// Using this allows us to prioritize values specified by longhands (or smaller
-/// shorthand subsets) when longhands and shorthands are both specified on the one keyframe.
-///
-/// Example orderings that result from this:
-///
-///   margin-left, margin
-///
-/// and:
-///
-///   border-top-color, border-color, border-top, border
+/// shorthand subsets) when longhands and shorthands are both specified on the
+/// one keyframe.
 ///
 /// [property-order] https://drafts.csswg.org/web-animations/#calculating-computed-keyframes
 pub fn compare_property_priority(a: &PropertyId, b: &PropertyId) -> cmp::Ordering {
-    match (a.as_shorthand(), b.as_shorthand()) {
-        // Within shorthands, sort by the number of subproperties, then by IDL name.
-        (Ok(a), Ok(b)) => {
-            let subprop_count_a = a.longhands().count();
-            let subprop_count_b = b.longhands().count();
-            subprop_count_a
-                .cmp(&subprop_count_b)
-                .then_with(|| {
-                    get_idl_name_sort_order(a).cmp(&get_idl_name_sort_order(b))
-                })
-        },
+    let a_category = PropertyCategory::of(a);
+    let b_category = PropertyCategory::of(b);
+
+    if a_category != b_category {
+        return a_category.cmp(&b_category);
+    }
 
-        // Longhands go before shorthands.
-        (Ok(_), Err(_)) => cmp::Ordering::Greater,
-        (Err(_), Ok(_)) => cmp::Ordering::Less,
+    if a_category == PropertyCategory::Shorthand {
+        let a = a.as_shorthand().unwrap();
+        let b = b.as_shorthand().unwrap();
+        // Within shorthands, sort by the number of subproperties, then by IDL
+        // name.
+        let subprop_count_a = a.longhands().count();
+        let subprop_count_b = b.longhands().count();
+        return subprop_count_a.cmp(&subprop_count_b).then_with(|| {
+            get_idl_name_sort_order(a).cmp(&get_idl_name_sort_order(b))
+        });
+    }
 
-        // Both are longhands or custom properties in which case they don't overlap and should
-        // sort equally.
-        _ => cmp::Ordering::Equal,
-    }
+    cmp::Ordering::Equal
 }
 
 fn get_idl_name_sort_order(shorthand: ShorthandId) -> u32 {
 <%
 # Sort by IDL name.
 sorted_shorthands = sorted(data.shorthands, key=lambda p: to_idl_name(p.ident))
 
 # Annotate with sorted position
--- a/servo/ports/geckolib/glue.rs
+++ b/servo/ports/geckolib/glue.rs
@@ -4631,16 +4631,17 @@ pub extern "C" fn Servo_GetComputedKeyfr
 
         let mut property_index = 0;
         for property in PrioritizedPropertyIter::new(&keyframe.mPropertyValues) {
             if simulate_compute_values_failure(property) {
                 continue;
             }
 
             let mut maybe_append_animation_value = |property: LonghandId, value: Option<AnimationValue>| {
+                debug_assert!(!property.is_logical());
                 if seen.contains(property) {
                     return;
                 }
                 seen.insert(property);
 
                 // This is safe since we immediately write to the uninitialized values.
                 unsafe { animation_values.set_len((property_index + 1) as u32) };
                 animation_values[property_index].mProperty = property.to_nscsspropertyid();
@@ -4837,16 +4838,17 @@ fn fill_in_missing_keyframe_values(
         }
     }
 }
 
 #[no_mangle]
 pub unsafe extern "C" fn Servo_StyleSet_GetKeyframesForName(
     raw_data: RawServoStyleSetBorrowed,
     element: RawGeckoElementBorrowed,
+    style: ComputedStyleBorrowed,
     name: *mut nsAtom,
     inherited_timing_function: nsTimingFunctionBorrowed,
     keyframes: RawGeckoKeyframeListBorrowedMut,
 ) -> bool {
     debug_assert!(keyframes.len() == 0, "keyframes should be initially empty");
 
     let element = GeckoElement(element);
     let data = PerDocumentStyleData::from_ffi(raw_data).borrow();
@@ -4862,16 +4864,18 @@ pub unsafe extern "C" fn Servo_StyleSet_
 
     let mut properties_set_at_current_offset = LonghandIdSet::new();
     let mut properties_set_at_start = LonghandIdSet::new();
     let mut properties_set_at_end = LonghandIdSet::new();
     let mut has_complete_initial_keyframe = false;
     let mut has_complete_final_keyframe = false;
     let mut current_offset = -1.;
 
+    let writing_mode = style.writing_mode;
+
     // Iterate over the keyframe rules backwards so we can drop overridden
     // properties (since declarations in later rules override those in earlier
     // ones).
     for step in animation.steps.iter().rev() {
         if step.start_percentage.0 != current_offset {
             properties_set_at_current_offset.clear();
             current_offset = step.start_percentage.0;
         }
@@ -4893,17 +4897,24 @@ pub unsafe extern "C" fn Servo_StyleSet_
 
         match step.value {
             KeyframesStepValue::ComputedValues => {
                 // In KeyframesAnimation::from_keyframes if there is no 0% or
                 // 100% keyframe at all, we will create a 'ComputedValues' step
                 // to represent that all properties animated by the keyframes
                 // animation should be set to the underlying computed value for
                 // that keyframe.
+                let mut seen = LonghandIdSet::new();
                 for property in animation.properties_changed.iter() {
+                    let property = property.to_physical(writing_mode);
+                    if seen.contains(property) {
+                        continue;
+                    }
+                    seen.insert(property);
+
                     Gecko_AppendPropertyValuePair(
                         &mut (*keyframe).mPropertyValues,
                         property.to_nscsspropertyid(),
                     );
                 }
                 if current_offset == 0.0 {
                     has_complete_initial_keyframe = true;
                 } else if current_offset == 1.0 {
@@ -4912,33 +4923,33 @@ pub unsafe extern "C" fn Servo_StyleSet_
             },
             KeyframesStepValue::Declarations { ref block } => {
                 let guard = block.read_with(&guard);
 
                 let mut custom_properties = PropertyDeclarationBlock::new();
 
                 // Filter out non-animatable properties and properties with
                 // !important.
-                for declaration in guard.normal_declaration_iter() {
+                for declaration in guard.normal_declaration_iter().rev() {
                     let id = declaration.id();
 
                     let id = match id {
                         PropertyDeclarationId::Longhand(id) => {
                             // Skip the 'display' property because although it
                             // is animatable from SMIL, it should not be
                             // animatable from CSS Animations.
                             if id == LonghandId::Display {
                                 continue;
                             }
 
                             if !id.is_animatable() {
                                 continue;
                             }
 
-                            id
+                            id.to_physical(writing_mode)
                         }
                         PropertyDeclarationId::Custom(..) => {
                             custom_properties.push(
                                 declaration.clone(),
                                 Importance::Normal,
                                 DeclarationSource::CssOm,
                             );
                             continue;
@@ -4952,17 +4963,17 @@ pub unsafe extern "C" fn Servo_StyleSet_
                     let pair = Gecko_AppendPropertyValuePair(
                         &mut (*keyframe).mPropertyValues,
                         id.to_nscsspropertyid(),
                     );
 
                     (*pair).mServoDeclarationBlock.set_arc_leaky(
                         Arc::new(global_style_data.shared_lock.wrap(
                             PropertyDeclarationBlock::with_one(
-                                declaration.clone(),
+                                declaration.to_physical(writing_mode),
                                 Importance::Normal,
                             )
                         ))
                     );
 
                     if current_offset == 0.0 {
                         properties_set_at_start.insert(id);
                     } else if current_offset == 1.0 {
@@ -4980,29 +4991,34 @@ pub unsafe extern "C" fn Servo_StyleSet_
                     (*pair).mServoDeclarationBlock.set_arc_leaky(Arc::new(
                         global_style_data.shared_lock.wrap(custom_properties)
                     ));
                 }
             },
         }
     }
 
+    let mut properties_changed = LonghandIdSet::new();
+    for property in animation.properties_changed.iter() {
+        properties_changed.insert(property.to_physical(writing_mode));
+    }
+
     // Append property values that are missing in the initial or the final keyframes.
     if !has_complete_initial_keyframe {
         fill_in_missing_keyframe_values(
-            &animation.properties_changed,
+            &properties_changed,
             inherited_timing_function,
             &properties_set_at_start,
             Offset::Zero,
             keyframes,
         );
     }
     if !has_complete_final_keyframe {
         fill_in_missing_keyframe_values(
-            &animation.properties_changed,
+            &properties_changed,
             inherited_timing_function,
             &properties_set_at_end,
             Offset::One,
             keyframes,
         );
     }
     true
 }