From a7c50b57a182bc58266e20a85dbdbed1aecbe2e8 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= <emilio@crisal.io>
Date: Tue, 19 Nov 2019 00:43:34 +0000
Subject: [PATCH] style: Handle logical shorthand animations with variable
 references correctly.

When we physicalize the declarations for @keyframes, we end up having a physical
declaration with an unparsed value with `from_shorthand` being the logical
shorthand.

Account for this case properly when substituting custom properties, to avoid
panicking.

Differential Revision: https://phabricator.services.mozilla.com/D53663
---
 components/style/properties/data.py           | 12 ++++----
 .../style/properties/properties.mako.rs       | 30 ++++++++++++-------
 2 files changed, 27 insertions(+), 15 deletions(-)

diff --git a/components/style/properties/data.py b/components/style/properties/data.py
index d1876a5ea027..87ec87a7fa5e 100644
--- a/components/style/properties/data.py
+++ b/components/style/properties/data.py
@@ -257,10 +257,12 @@ def __init__(self, style_struct, name, spec=None, animation_value_type=None, key
     def type():
         return "longhand"
 
-    # For a given logical property return all the physical
-    # property names corresponding to it.
-    def all_physical_mapped_properties(self):
-        assert self.logical
+    # For a given logical property return all the physical property names
+    # corresponding to it.
+    def all_physical_mapped_properties(self, data):
+        if not self.logical:
+            return []
+
         candidates = [s for s in LOGICAL_SIDES + LOGICAL_SIZES + LOGICAL_CORNERS
                       if s in self.name] + [s for s in LOGICAL_AXES if self.name.endswith(s)]
         assert(len(candidates) == 1)
@@ -270,7 +272,7 @@ def all_physical_mapped_properties(self):
             else PHYSICAL_SIZES if logical_side in LOGICAL_SIZES \
             else PHYSICAL_AXES if logical_side in LOGICAL_AXES \
             else LOGICAL_CORNERS
-        return [to_phys(self.name, logical_side, physical_side)
+        return [data.longhands_by_name[to_phys(self.name, logical_side, physical_side)]
                 for physical_side in physical]
 
     def experimental(self, engine):
diff --git a/components/style/properties/properties.mako.rs b/components/style/properties/properties.mako.rs
index 37b2319dfd6f..484b52e8c870 100644
--- a/components/style/properties/properties.mako.rs
+++ b/components/style/properties/properties.mako.rs
@@ -1651,13 +1651,25 @@ impl UnparsedValue {
                     shorthands::${shorthand.ident}::parse_value(&context, input)
                     .map(|longhands| {
                         match longhand_id {
+                            <% seen = set() %>
                             % for property in shorthand.sub_properties:
-                                LonghandId::${property.camel_case} => {
-                                    PropertyDeclaration::${property.camel_case}(
-                                        longhands.${property.ident}
-                                    )
-                                }
+                            // When animating logical properties, we end up
+                            // physicalizing the value during the animation, but
+                            // the value still comes from the logical shorthand.
+                            //
+                            // So we need to handle the physical properties too.
+                            % for prop in [property] + property.all_physical_mapped_properties(data):
+                            % if prop.camel_case not in seen:
+                            LonghandId::${prop.camel_case} => {
+                                PropertyDeclaration::${prop.camel_case}(
+                                    longhands.${property.ident}
+                                )
+                            }
+                            <% seen.add(prop.camel_case) %>
+                            % endif
                             % endfor
+                            % endfor
+                            <% del seen %>
                             _ => unreachable!()
                         }
                     })
@@ -2176,14 +2188,12 @@ impl PropertyDeclaration {
         let mut ret = self.clone();
 
         % for prop in data.longhands:
-        % if prop.logical:
-        % for physical_property in prop.all_physical_mapped_properties():
-        % if data.longhands_by_name[physical_property].specified_type() != prop.specified_type():
+        % for physical_property in prop.all_physical_mapped_properties(data):
+        % if physical_property.specified_type() != prop.specified_type():
             <% raise "Logical property %s should share specified value with physical property %s" % \
-                     (prop.name, physical_property) %>
+                     (prop.name, physical_property.name) %>
         % endif
         % endfor
-        % endif
         % endfor
 
         unsafe {