From ea1c5485bd384769417921b12cdd6a2d2d12f0ed 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] Bug 1597273 - Handle logical shorthand animations with
 variable references correctly. r=hiro

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
---
 servo/components/style/properties/data.py     | 27 ++++++++++-------
 .../style/properties/properties.mako.rs       | 30 ++++++++++++-------
 .../tests/css/css-logical/animation-001.html  | 12 ++++++++
 .../tests/css/css-logical/animation-002.html  | 13 ++++++++
 4 files changed, 61 insertions(+), 21 deletions(-)

diff --git a/servo/components/style/properties/data.py b/servo/components/style/properties/data.py
index 12bd3ce38aec..0842c9158f89 100644
--- a/servo/components/style/properties/data.py
+++ b/servo/components/style/properties/data.py
@@ -165,6 +165,10 @@ def parse_property_aliases(alias_list):
     return result
 
 
+def to_phys(name, logical, physical):
+    return name.replace(logical, physical).replace("inset-", "")
+
+
 class Longhand(object):
     def __init__(self, style_struct, name, spec=None, animation_value_type=None, keyword=None,
                  predefined_type=None, servo_pref=None, gecko_pref=None,
@@ -239,20 +243,21 @@ class Longhand(object):
     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
-        logical_side = None
-        for s in LOGICAL_SIDES + LOGICAL_SIZES + LOGICAL_CORNERS:
-            if s in self.name:
-                assert not logical_side
-                logical_side = s
-        assert logical_side
+    # 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]
+        assert(len(candidates) == 1)
+        logical_side = candidates[0]
+
         physical = PHYSICAL_SIDES if logical_side in LOGICAL_SIDES \
             else PHYSICAL_SIZES if logical_side in LOGICAL_SIZES \
             else LOGICAL_CORNERS
-        return [self.name.replace(logical_side, physical_side).replace("inset-", "")
+        return [data.longhands_by_name[to_phys(self.name, logical_side, physical_side)]
                 for physical_side in physical]
 
     def experimental(self, product):
diff --git a/servo/components/style/properties/properties.mako.rs b/servo/components/style/properties/properties.mako.rs
index 1e92066c7656..a9ffad0eed96 100644
--- a/servo/components/style/properties/properties.mako.rs
+++ b/servo/components/style/properties/properties.mako.rs
@@ -1599,13 +1599,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!()
                         }
                     })
@@ -2051,14 +2063,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 {
diff --git a/testing/web-platform/tests/css/css-logical/animation-001.html b/testing/web-platform/tests/css/css-logical/animation-001.html
index 8135f43275bb..71542022c979 100644
--- a/testing/web-platform/tests/css/css-logical/animation-001.html
+++ b/testing/web-platform/tests/css/css-logical/animation-001.html
@@ -299,6 +299,18 @@ test(t => {
   assert_equals(getComputedStyle(div).marginRight, '50px');
 }, 'Animations update when the direction is changed');
 
+test(t => {
+  const div = addDiv(t);
+  const anim = div.animate(
+    {
+      insetBlock: ['var(--200px)', 'var(--300px)'],
+    },
+    1000
+  );
+  anim.currentTime = 500;
+  assert_equals(getComputedStyle(div).insetBlockStart, '250px');
+}, 'Logical shorthand with variable references animates correctly');
+
 test(t => {
   const div = addDiv(t);
   const anim = div.animate(
diff --git a/testing/web-platform/tests/css/css-logical/animation-002.html b/testing/web-platform/tests/css/css-logical/animation-002.html
index d4f199d50e48..9213a05d83da 100644
--- a/testing/web-platform/tests/css/css-logical/animation-002.html
+++ b/testing/web-platform/tests/css/css-logical/animation-002.html
@@ -196,6 +196,19 @@ test(t => {
   assert_equals(getComputedStyle(div).height, '0px');
 }, 'Animations update when the writing-mode is changed through a CSS variable');
 
+test(t => {
+  addStyle(t, { ':root': '--200px: 200px; --300px: 300px' });
+  addStyle(t, {
+    '@keyframes anim': 'from { inset-block: var(--200px) } to { inset-block: var(--300px) }',
+  });
+  const div = addDiv(t, {
+    style:
+      'animation: anim 10s -5s paused linear;'
+      + ' width: 0px; height: 0px;'
+  });
+  assert_equals(getComputedStyle(div).insetBlockStart, '250px');
+}, 'Logical shorthand with variable references animates correctly');
+
 test(t => {
   addStyle(t, {
     '@keyframes anim':
-- 
2.24.0

