Bug 1597273 - Handle logical shorthand animations with variable references correctly. r=hiro
authorEmilio Cobos Álvarez <emilio@crisal.io>
Tue, 19 Nov 2019 00:43:34 +0000
changeset 502520 6a9a26055f9c47058638838305867c87e86eddec
parent 502519 59d5dcf22862ff998d5f08fcf42309bdb0fc066d
child 502521 53ca0eff415ed67f2d735f8f81588db69dd7d8c6
push id114172
push userdluca@mozilla.com
push dateTue, 19 Nov 2019 11:31:10 +0000
treeherdermozilla-inbound@b5c5ba07d3db [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewershiro
bugs1597273
milestone72.0a1
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
servo/components/style/properties/properties.mako.rs
testing/web-platform/tests/css/css-logical/animation-001.html
testing/web-platform/tests/css/css-logical/animation-002.html
--- a/servo/components/style/properties/data.py
+++ b/servo/components/style/properties/data.py
@@ -252,30 +252,32 @@ class Longhand(object):
 
         # See compute_damage for the various values this can take
         self.servo_restyle_damage = servo_restyle_damage
 
     @staticmethod
     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)
         logical_side = candidates[0]
 
         physical = PHYSICAL_SIDES if logical_side in LOGICAL_SIDES \
             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):
         if engine == "gecko":
             return bool(self.gecko_pref)
         elif engine == "servo-2013":
             return bool(self.servo_2013_pref)
         elif engine == "servo-2020":
--- a/servo/components/style/properties/properties.mako.rs
+++ b/servo/components/style/properties/properties.mako.rs
@@ -1648,23 +1648,35 @@ impl UnparsedValue {
                     // than a CSS-wide keyword, after variable substitution.
                     Err(input.new_custom_error(SelectorParseErrorKind::UnexpectedIdent("all".into())))
                 }
                 % for shorthand in data.shorthands_except_all():
                 Some(ShorthandId::${shorthand.camel_case}) => {
                     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!()
                         }
                     })
                 }
                 % endfor
             }
         });
 
@@ -2173,24 +2185,22 @@ impl PropertyDeclaration {
             % for prop in data.longhands:
             PropertyDeclaration::${prop.camel_case}(..) => {},
             % endfor
         }
 
         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 {
             let longhand_id = *(&mut ret as *mut _ as *mut LonghandId);
 
             debug_assert_eq!(
                 PropertyDeclarationId::Longhand(longhand_id),
                 ret.id()
--- a/testing/web-platform/tests/css/css-logical/animation-001.html
+++ b/testing/web-platform/tests/css/css-logical/animation-001.html
@@ -297,16 +297,28 @@ test(t => {
   div.style.direction = 'rtl';
   assert_equals(getComputedStyle(div).marginLeft, '0px');
   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(
     { writingMode: 'vertical-rl' },
     { duration: 1, easing: 'step-start' }
   );
   assert_equals(getComputedStyle(div).writingMode, 'horizontal-tb');
   assert_equals(anim.effect.getKeyframes().length, 0);
 }, 'writing-mode is not animatable');
 
 test(t => {
--- a/testing/web-platform/tests/css/css-logical/animation-002.html
+++ b/testing/web-platform/tests/css/css-logical/animation-002.html
@@ -192,16 +192,29 @@ test(t => {
   assert_equals(getComputedStyle(div).height, '50px');
 
   div.style.setProperty('--writingMode', 'vertical-rl');
   assert_equals(getComputedStyle(div).width, '50px');
   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':
       'from { margin-inline-start: 0px } to { margin-inline-start: 100px }',
   });
   const div = addDiv(t, { style: 'animation: anim 10s -5s paused linear' });
   assert_equals(getComputedStyle(div).marginLeft, '50px');
   assert_equals(getComputedStyle(div).marginRight, '0px');