Closed Bug 1264865 Opened 4 years ago Closed 4 years ago

steps(3, end) should serialize using the shorter steps(3) syntax

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox48 --- affected
firefox50 --- fixed

People

(Reporter: birtles, Assigned: daisuke)

Details

Attachments

(3 files)

See thread starting: https://lists.w3.org/Archives/Public/www-style/2016Apr/0107.html

In Chrome/Edge:

  elem.style.animationTimingFunction = 'steps(3)';
  getComputedStyle(elem).animationTimingFunction; // steps(3, end)
  elem.style.animationTimingFunction = 'steps(3, end)';
  getComputedStyle(elem).animationTimingFunction; // steps(3, end)

In Firefox:

  elem.style.animationTimingFunction = 'steps(3)';
  getComputedStyle(elem).animationTimingFunction; // steps(3)
  elem.style.animationTimingFunction = 'steps(3, end)';
  getComputedStyle(elem).animationTimingFunction; // steps(3, end)

But apparently, "The standard serialization strategy is to serialize with the minimum amount necessary to accurately represent the value. If omitting some
component is the same as specifying a particular keyword, then all values with that keyword serialize with it omitted, etc."[1]

[1] https://lists.w3.org/Archives/Public/www-style/2016Apr/0118.html

i.e.

  elem.style.animationTimingFunction = 'steps(3)';
  getComputedStyle(elem).animationTimingFunction; // steps(3)
  elem.style.animationTimingFunction = 'steps(3, end)';
  getComputedStyle(elem).animationTimingFunction; // steps(3)

As for steps(1), should that be 'step-end' or vice versa? Presumably not, but I'll follow up to check.
Oh, I notice that Chrome serializes 'steps(1)' as 'step-end' but Edge serializes 'steps(1)' as 'steps(1, end)'. Followed up about this: https://lists.w3.org/Archives/Public/www-style/2016Apr/0261.html
Assignee: nobody → daisuke
Comment on attachment 8773623 [details]
Bug 1264865 - Part 2: Remove trailing whitespaces.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66364/diff/1-2/
Comment on attachment 8773624 [details]
Bug 1264865 - Part 3: Remove codes that are no longer in use.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66366/diff/1-2/
Submitted PR to specify this in CSS transitions: https://github.com/w3c/csswg-drafts/pull/340/files
Attachment #8773623 - Flags: review?(bbirtles) → review+
Comment on attachment 8773623 [details]
Bug 1264865 - Part 2: Remove trailing whitespaces.

https://reviewboard.mozilla.org/r/66364/#review63438

r=me with the changes made but I guess we're going to merge this into part 1 so I'll check it again there anyway

::: testing/web-platform/tests/web-animations/interfaces/AnimationEffectTiming/easing.html:31
(Diff revision 2)
>      var target = createDiv(t);
>      var anim = target.animate([ { opacity: 0 }, { opacity: 1 } ],
>                                { duration: 1000 * MS_PER_SEC,
>                                  fill: 'forwards' });
>      anim.effect.timing.easing = options.easing;
> -    assert_equals(anim.effect.timing.easing, options.easing);
> +    assert_equals(anim.effect.timing.easing, options.expectedSerializedEasing);

Just call this 'serialization'? And make it optional to specify (i.e. you only need to specify it when the value serialized differently).

e.g. assert_equals(anim.effect.timing.easing, options.serialization || options.easing)

::: testing/web-platform/tests/web-animations/resources/effect-easing-tests.js:12
(Diff revision 2)
> +  },
> +  {
> +    desc: 'steps(1, start) function',
> +    easing: 'steps(1, start)',
> +    easingFunction: stepStart(1),
> +    expectedSerializedEasing: 'steps(1, start)'

With the suggestion above, we could omit the serialization here (and likewise below).
Comment on attachment 8773624 [details]
Bug 1264865 - Part 3: Remove codes that are no longer in use.

https://reviewboard.mozilla.org/r/66366/#review63440

r=me with changes made

This patch should be merged into part 1 (or possibly landed ahead of part 1 if that works).

::: dom/animation/test/css-animations/file_keyframeeffect-getkeyframes.html:164
(Diff revision 2)
>  // animation-timing-function values to test with, where the value
>  // is exactly the same as its serialization, sorted by the order
>  // getKeyframes() will group frames with the same easing function
>  // together (by nsTimingFunction::Compare).
>  const kTimingFunctionValues = [
> -  "ease",
> -  "linear",
> -  "ease-in",
> -  "ease-out",
> -  "ease-in-out",
> -  "step-start",
> -  "steps(1, start)",
> -  "steps(2, start)",
> -  "step-end",
> -  "steps(1)",
> -  "steps(1, end)",
> -  "steps(2)",
> -  "steps(2, end)",
> -  "cubic-bezier(0, 0, 1, 1)",
> -  "cubic-bezier(0, 0.25, 0.75, 1)",
> +  { easing: "ease", expectedSerializedEasing: "ease" },
> +  { easing: "linear", expectedSerializedEasing: "linear" },
> +  { easing: "ease-in", expectedSerializedEasing: "ease-in" },
> +  { easing: "ease-out", expectedSerializedEasing: "ease-out" },
> +  { easing: "ease-in-out", expectedSerializedEasing: "ease-in-out" },
> +  { easing: "step-start", expectedSerializedEasing: "steps(1, start)" },
> +  { easing: "steps(1, start)", expectedSerializedEasing: "steps(1, start)" },
> +  { easing: "steps(2, start)", expectedSerializedEasing: "steps(2, start)" },
> +  { easing: "step-end", expectedSerializedEasing: "steps(1)" },
> +  { easing: "steps(1)", expectedSerializedEasing: "steps(1)" },
> +  { easing: "steps(1, end)", expectedSerializedEasing: "steps(1)" },
> +  { easing: "steps(2)", expectedSerializedEasing: "steps(2)" },
> +  { easing: "steps(2, end)", expectedSerializedEasing: "steps(2)" },
> +  { easing: "cubic-bezier(0, 0, 1, 1)",
> +    expectedSerializedEasing: "cubic-bezier(0, 0, 1, 1)" },
> +  { easing: "cubic-bezier(0, 0.25, 0.75, 1)",
> +    expectedSerializedEasing: "cubic-bezier(0, 0.25, 0.75, 1)" },
>  ];

According to the comment, this is only supposed to test where the value exactly matches its serialization. Perhaps we can just drop the ones that serialize differently since I'm suggesting that we simplify nsTimingFunction::Compare and hence those test cases won't be needed.
Attachment #8773624 - Flags: review?(bbirtles) → review+
Attachment #8773622 - Flags: review?(bbirtles)
Comment on attachment 8773622 [details]
Bug 1264865 - Part 1:  steps(3, end) should serialize using the shorter steps(3) syntax.

https://reviewboard.mozilla.org/r/66362/#review63436

(Oops, it looks like this comment never got submitted, sorry!)

We should merge parts 2 and 3 into this patch since each changeset should, ideally, be atomic at least in the cumulative sense. That is it should be possible to land part 1 without tests failing.

Also, I think we can now drop nsTimingFunction::StepSyntax and a lot of the complexity involved in storing the particular syntax used to specify the timing function since I think that's no longer needed (assuming the PR in comment 9 is accepted). The code simplification, however, can happen in a separate patch and it might be better to wait on that PR being accepted before going ahead with that.

Clearing review request for now since parts 2 and 3 should be merged in.
Comment on attachment 8773622 [details]
Bug 1264865 - Part 1:  steps(3, end) should serialize using the shorter steps(3) syntax.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66362/diff/1-2/
Attachment #8773623 - Attachment description: Bug 1264865 - Part 2: Modify web-platform tests. → Bug 1264865 - Part 2: Remove trailing whitespaces.
Attachment #8773624 - Attachment description: Bug 1264865 - Part 3: Modify dom/animation tests. → Bug 1264865 - Part 3: Remove codes that are no longer in use.
Attachment #8773622 - Flags: review?(bbirtles)
Comment on attachment 8773623 [details]
Bug 1264865 - Part 2: Remove trailing whitespaces.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66364/diff/2-3/
Comment on attachment 8773624 [details]
Bug 1264865 - Part 3: Remove codes that are no longer in use.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66366/diff/2-3/
Comment on attachment 8773622 [details]
Bug 1264865 - Part 1:  steps(3, end) should serialize using the shorter steps(3) syntax.

https://reviewboard.mozilla.org/r/66362/#review63900

r=birtles with comments addressed

::: testing/web-platform/tests/web-animations/interfaces/AnimationEffectTiming/easing.html:31
(Diff revision 2)
>      var target = createDiv(t);
>      var anim = target.animate([ { opacity: 0 }, { opacity: 1 } ],
>                                { duration: 1000 * MS_PER_SEC,
>                                  fill: 'forwards' });
>      anim.effect.timing.easing = options.easing;
> -    assert_equals(anim.effect.timing.easing, options.easing);
> +    assert_equals(anim.effect.timing.easing, options.serialization || options.easing);

Can we wrap this to < 80 chars per line?
Attachment #8773622 - Flags: review?(bbirtles) → review+
Comment on attachment 8773622 [details]
Bug 1264865 - Part 1:  steps(3, end) should serialize using the shorter steps(3) syntax.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66362/diff/2-3/
Comment on attachment 8773623 [details]
Bug 1264865 - Part 2: Remove trailing whitespaces.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66364/diff/3-4/
Comment on attachment 8773624 [details]
Bug 1264865 - Part 3: Remove codes that are no longer in use.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66366/diff/3-4/
Pushed by bbirtles@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0b37174053f7
Part 1:  steps(3, end) should serialize using the shorter steps(3) syntax. r=birtles
https://hg.mozilla.org/integration/autoland/rev/a4d5431e38f0
Part 2: Remove trailing whitespaces. r=birtles
https://hg.mozilla.org/integration/autoland/rev/b1832f8aecd0
Part 3: Remove codes that are no longer in use. r=birtles
You need to log in before you can comment on or make changes to this bug.