From ea1c5485bd384769417921b12cdd6a2d2d12f0ed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= 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