Bug 1309752: Animate logical properties. r=birtles
authorEmilio Cobos Álvarez <emilio@crisal.io>
Fri, 06 Jul 2018 05:19:10 +0200
changeset 427068 851648d9e9710c7587a28e103565c5f09ed19ac8
parent 427067 a5fa7f73f54968fcd672c32ed8fba6e01264f281
child 427069 c4bd0fbb78654005886cb3d02c4d836b010763a1
push id105397
push useremilio@crisal.io
push dateWed, 18 Jul 2018 10:10:47 +0000
treeherdermozilla-inbound@1467f56b0eee [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbirtles
bugs1309752
milestone63.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 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
@@ -1971,17 +1971,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
@@ -355,22 +355,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
@@ -642,23 +642,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
@@ -539,25 +539,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;
@@ -651,23 +654,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
@@ -218,22 +218,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
@@ -126,45 +126,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,33 @@ 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:
     /// `${prop.name}`
+    % if prop.animatable and not prop.logical:
     ${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 +370,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 +444,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 +476,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 +565,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 +674,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 +2942,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
@@ -4670,16 +4670,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();
@@ -4876,16 +4877,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();
@@ -4901,16 +4903,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;
         }
@@ -4932,17 +4936,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 {
@@ -4951,33 +4962,36 @@ 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() {
+                //
+                // Also, iterate in reverse to respect the source order in case
+                // there are logical and physical longhands in the same block.
+                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,
                                 DeclarationPushMode::Append,
                             );
                             continue;
@@ -4991,17 +5005,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 {
@@ -5019,29 +5033,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
 }