1371493 - stylo: Servo_GetComputedKeyframeValues does not respect property ordering

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P2)

Description

[Brian Birtles (:birtles, Feb 26~ parental leave)]

When calculating the animation properties from keyframes there are specific rules for the order in which properties apply when shorthands and longhands overlap:

  "If conflicts arise when expanding shorthand properties due to shorthand
  properties overlapping with existing longhand properties or other with other
  shorthand properties, apply the following rules in order until the conflict is
  resolved:

    1. Longhand properties override shorthand properties (e.g. border-top-color
       overrides border-top).

    2. Shorthand properties with fewer longhand components override those with
       more longhand components (e.g. border-top overrides border-color).

    3. For shorthand properties with an equal number of longhand components,
       properties whose IDL name (see the CSS property to IDL attribute
       algorithm [CSSOM]) appears earlier when sorted in ascending order by the
       Unicode codepoints that make up each IDL name, override those who appear
       later."[1]

Gecko implements this via the PropertyPriorityIterator. Servo's Servo_GetComputedKeyframeValues does not appear to do this however, hence several tests in dom/animation/test/chrome/test_animation_properties.html fail.

[1] https://w3c.github.io/web-animations/#calculating-computed-keyframes
[2] http://searchfox.org/mozilla-central/rev/a798ee4fc323f9387b7576dbed177859d29d09b7/dom/animation/KeyframeUtils.cpp#154

Comment 1

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: WIP patch

This seems to mostly work although the corresponding tests won't pass without 1381389 (which should hit central soon I expect).

Comment 2

[Brian Birtles (:birtles, Feb 26~ parental leave)]

After rebasing and fixing a minor bug in KeyframeEffectReadOnly::GetProperties, dom/animation/test/chrome/test_animation_properties.html passes for me (although since we don't run this test on Stylo builds yet there are no test expectations to update).

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1035e929c56455c46e01b958fe3abbf2d24f07f8

Comment 3

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Bug 1371493 - Iterate through properties in priority order when computing keyframes;

This is largely just a translation of PropertyPriorityIterator[1] into rust with
the exception that IDL sort order is only defined for shorthands since that's
all we currently require.

[1]
http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/dom/animation/KeyframeUtils.cpp#151

Review commit: https://reviewboard.mozilla.org/r/160306/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/160306/

Comment 4

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Bug 1371493 - Compare AnimationValues when producing property-based keyframes;

The KeyframeEffectReadOnly::GetProperties compares AnimationValue's mGecko
member which means it sometimes produces the wrong results when using the Servo
backend. Now that AnimationValue has a suitable operator!= method we can simply
compare the AnimationValues directly.

Review commit: https://reviewboard.mozilla.org/r/160308/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/160308/

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8889252 [details]
Bug 1371493 - Compare AnimationValues when producing property-based keyframes;

https://reviewboard.mozilla.org/r/160308/#review165584

Comment 6

[Cameron McCormack (:heycam)]

Comment on attachment 8889251 [details]
Bug 1371493 - Iterate through properties in priority order when computing keyframes;

https://reviewboard.mozilla.org/r/160306/#review165588

Looks good, thanks!

::: servo/components/style/properties/helpers/animated_properties.mako.rs:3226
(Diff revision 1)
> +pub fn compare_property_priority(a: &PropertyId, b: &PropertyId) -> cmp::Ordering
> +{

Nit: "{" on previous line for Rust.

::: servo/ports/geckolib/glue.rs:2952
(Diff revision 1)
> +      if self.curr >= self.sorted_property_indices.len() {
> +          return None
> +      }
> +      self.curr += 1;
> +      Some(&self.properties[self.sorted_property_indices[self.curr - 1].index])

Nit: use a 4 space indent here.

Comment 7

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Servo PR #17831

Comment 8

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8889252 [details]
Bug 1371493 - Compare AnimationValues when producing property-based keyframes;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/160308/diff/1-2/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e09385879b6e
Compare AnimationValues when producing property-based keyframes; r=hiro

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e09385879b6e