Closed Bug 1371493 Opened 4 years ago Closed 4 years ago

stylo: Servo_GetComputedKeyframeValues does not respect property ordering


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




Tracking Status
firefox56 --- fixed


(Reporter: birtles, Assigned: birtles)




(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

    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

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.

Priority: -- → P2
Assignee: nobody → bbirtles
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).
Comment on attachment 8889252 [details]
Bug 1371493 - Compare AnimationValues when producing property-based keyframes;
Attachment #8889252 - Flags: review?(hikezoe) → review+
Comment on attachment 8889251 [details]
Bug 1371493 - Iterate through properties in priority order when computing keyframes;

Looks good, thanks!

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

Nit: "{" on previous line for Rust.

::: servo/ports/geckolib/
(Diff revision 1)
> +      if self.curr >= self.sorted_property_indices.len() {
> +          return None
> +      }
> +      self.curr += 1;
> +      Some(&[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
Compare AnimationValues when producing property-based keyframes; r=hiro
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.