stylo: Servo_GetComputedKeyframeValues does not respect property ordering

ASSIGNED
Assigned to

Status

()

Core
CSS Parsing and Computation
P2
normal
ASSIGNED
2 months ago
14 hours ago

People

(Reporter: birtles, Assigned: birtles)

Tracking

(Blocks: 1 bug)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

2 months ago
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)

Updated

6 days ago
Assignee: nobody → bbirtles
Status: NEW → ASSIGNED
(Assignee)

Comment 1

3 days ago
Created attachment 8888683 [details] [diff] [review]
WIP patch

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

Updated

17 hours ago
Attachment #8888683 - Attachment is obsolete: true
(Assignee)

Comment 2

17 hours ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 5

15 hours ago
mozreview-review
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 6

14 hours ago
mozreview-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+
(Assignee)

Comment 7

14 hours ago
Created attachment 8889264 [details] [review]
Servo PR #17831
Comment hidden (mozreview-request)
(Assignee)

Updated

14 hours ago
Attachment #8889251 - Attachment is obsolete: true

Comment 9

14 hours ago
Pushed by bbirtles@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e09385879b6e
Compare AnimationValues when producing property-based keyframes; r=hiro
You need to log in before you can comment on or make changes to this bug.