Closed Bug 1371493 Opened 2 years ago Closed 2 years ago

stylo: Servo_GetComputedKeyframeValues does not respect property ordering

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: birtles, Assigned: birtles)

References

Details

Attachments

(2 files, 2 obsolete files)

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
Priority: -- → P2
Assignee: nobody → bbirtles
Status: NEW → ASSIGNED
Attached patch WIP patch (obsolete) — Splinter Review
This seems to mostly work although the corresponding tests won't pass without 1381389 (which should hit central soon I expect).
Attachment #8888683 - Attachment is obsolete: true
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 on attachment 8889252 [details]
Bug 1371493 - Compare AnimationValues when producing property-based keyframes;

https://reviewboard.mozilla.org/r/160308/#review165584
Attachment #8889252 - Flags: review?(hikezoe) → review+
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.
Attachment #8889251 - Flags: review?(cam) → review+
Attached file Servo PR #17831
Attachment #8889251 - Attachment is obsolete: true
Pushed by bbirtles@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e09385879b6e
Compare AnimationValues when producing property-based keyframes; r=hiro
https://hg.mozilla.org/mozilla-central/rev/e09385879b6e
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.