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

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Brian Birtles (:birtles)]

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.

Comment 1

[Brian Birtles (:birtles)]

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

Comment 2

[Daisuke Akatsuka (:daisuke)]

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

Comment 3

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1264865 - Part 1: steps(3, end) should serialize using the shorter steps(3) syntax.

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

Comment 4

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1264865 - Part 2: Remove trailing whitespaces.

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

Comment 5

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1264865 - Part 3: Remove codes that are no longer in use.

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

Comment 6

[Daisuke Akatsuka (: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 7

[Daisuke Akatsuka (:daisuke)]

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/

Comment 8

[Daisuke Akatsuka (:daisuke)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ac06405c632

Comment 9

[Brian Birtles (:birtles)]

Submitted PR to specify this in CSS transitions: https://github.com/w3c/csswg-drafts/pull/340/files

Comment 10

[Brian Birtles (:birtles)]

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 11

[Brian Birtles (:birtles)]

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.

Comment 12

[Brian Birtles (:birtles)]

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 13

[Daisuke Akatsuka (:daisuke)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9e3f65e1e33d

Comment 14

[Daisuke Akatsuka (:daisuke)]

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/

Comment 15

[Daisuke Akatsuka (: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/2-3/

Comment 16

[Daisuke Akatsuka (:daisuke)]

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 17

[Brian Birtles (:birtles)]

Spec changes have now landed: https://drafts.csswg.org/css-transitions/#serializing-a-timing-function

Comment 18

[Brian Birtles (:birtles)]

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?

Comment 19

[Daisuke Akatsuka (:daisuke)]

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 20

[Daisuke Akatsuka (: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/3-4/

Comment 21

[Daisuke Akatsuka (:daisuke)]

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/

Comment 22

[Pulsebot]

Pushed by [e-mail]:
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

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0b37174053f7
https://hg.mozilla.org/mozilla-central/rev/a4d5431e38f0
https://hg.mozilla.org/mozilla-central/rev/b1832f8aecd0