New issue
Advanced search Search tips
Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Dec 14
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac , Fuchsia
Pri: 3
Type: Bug



Sign in to add a comment
link

Issue 772852: CSS animation: shorthand is serialized with wrong order

Reported by ericwilligers@chromium.org, Oct 9 2017 Project Member

Issue description

See
https://bugs.chromium.org/p/chromium/issues/detail?id=273092#c5
and
https://jsfiddle.net/ericwilligers/5a21nsqq/

The specification
https://drafts.csswg.org/css-animations/#animation
has animation-name last.

<single-animation> = <time> || <single-timing-function> || <time> || <single-animation-iteration-count> || <single-animation-direction> || <single-animation-fill-mode> || <single-animation-play-state> || [ none | <keyframes-name> ]

"Note that order is also important within each animation definition for distinguishing <keyframes-name> values from other keywords. When parsing, keywords that are valid for properties other than animation-name whose values were not found earlier in the shorthand must be accepted for those properties rather than for animation-name. Furthermore, when serializing, default values of other properties must be output in at least the cases necessary to distinguish an animation-name that could be a value of another property, and may be output in additional cases."
 

Comment 1 by ericwilligers@chromium.org, Oct 16 2017

All current browsers fail to follow the spec.

The jsfiddle results for Edge/Firefox/Safari are:
none
backwards


none
none


forwards
backwards


none
none


The jsfiddle results for Chrome are:
none
backwards
backwards 3s ease 0s 1 normal none running

backwards
none
none 3s ease 0s 1 normal backwards running

forwards
backwards
backwards 1s ease 0s 1 normal forwards running

backwards
forwards
forwards 1s ease 0s 1 normal backwards running

Comment 2 by smcgruer@chromium.org, Sep 7

Cc: smcgruer@chromium.org
The results from #1 are slightly misleading. Firefox/Edge/Safari do not support shorthands from getComputedStyle (they should in the future, the CSSWG ruled in favour of that in https://github.com/w3c/csswg-drafts/issues/2529). Therefore the copying of the style that way in the test doesn't work, and we can't compare across browsers.

Concentrating just on Chrome though, we do get the serialization wrong, as the test shows. I also prettied it up a little: http://output.jsbin.com/ravineb

There is some compat risk to fixing this, but I suspect not much given that no other browser even implements the shorthand yet. Plus given that it's so buggy currently I doubt anyone is using it :D

Comment 3 by xidac...@chromium.org, Dec 4

Owner: xidac...@chromium.org
Status: Assigned (was: Available)

Comment 4 by ericwilligers@chromium.org, Dec 4

> Firefox/Edge/Safari do not support shorthands from getComputedStyle

If we adapt the test to read the shorthand from the specified value,
then Firefox succeeds and Blink/Edge/Safari do not:-
https://jsfiddle.net/ericwilligers/kzmj70ya/

Firefox

first
- none
- backwards

second (copied from first)
- none
- backwards

third
- forwards
- backwards

fourth (copied from third)
- forwards
- backwards


Blink/Edge/Safari

first
- none
- backwards

second (copied from first)
- backwards
- none

third
- forwards
- backwards

fourth (copied from third)
- backwards
- forwards

Comment 5 by bugdroid1@chromium.org, Dec 14

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/66eb6c38f820a88d507d891208a164ae9bff516c

commit 66eb6c38f820a88d507d891208a164ae9bff516c
Author: Xida Chen <xidachen@chromium.org>
Date: Fri Dec 14 11:45:49 2018

Serialize animation in the right order

Per spec:
https://drafts.csswg.org/css-animations/#animation
The name of the animation should be the end of the value list, to
avoid ambiguity when serializing/deserializing the animation.

This CL fixes the problem for both element.style and the
window.getComputedStyle. A wpt test is added.

Since this CL changes the output of an existing web API, an Intent to
Ship has been sent to blink-dev and 3 LGTMs has been granted.
https://groups.google.com/a/chromium.org/forum/?utm_medium=email&utm_source=footer#!msg/blink-dev/KUelGRZP73Y/F4YxTGBOBwAJ

Bug:  772852 
Change-Id: I88b06b57c68013d99c8c4fd4bd39bdfcf9b807fc
Reviewed-on: https://chromium-review.googlesource.com/c/1367935
Reviewed-by: Stephen McGruer <smcgruer@chromium.org>
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Commit-Queue: Xida Chen <xidachen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#616639}
[modify] https://crrev.com/66eb6c38f820a88d507d891208a164ae9bff516c/third_party/blink/renderer/core/css/css_properties.json5
[modify] https://crrev.com/66eb6c38f820a88d507d891208a164ae9bff516c/third_party/blink/renderer/core/css/properties/shorthands/animation_custom.cc
[modify] https://crrev.com/66eb6c38f820a88d507d891208a164ae9bff516c/third_party/blink/web_tests/animations/animations-csstext.html
[modify] https://crrev.com/66eb6c38f820a88d507d891208a164ae9bff516c/third_party/blink/web_tests/animations/animations-parsing-005.html
[add] https://crrev.com/66eb6c38f820a88d507d891208a164ae9bff516c/third_party/blink/web_tests/external/wpt/css/css-animations/animations-parsing.html
[modify] https://crrev.com/66eb6c38f820a88d507d891208a164ae9bff516c/third_party/blink/web_tests/fast/css/transform-inline-style-remove-expected.txt
[modify] https://crrev.com/66eb6c38f820a88d507d891208a164ae9bff516c/third_party/blink/web_tests/http/tests/devtools/elements/styles-4/styles-keyframes-expected.txt

Comment 6 by bugdroid1@chromium.org, Dec 14

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f48839c17381e751306a8a87571a1f060c609ea3

commit f48839c17381e751306a8a87571a1f060c609ea3
Author: Xida Chen <xidachen@chromium.org>
Date: Fri Dec 14 15:19:15 2018

[LayoutTest] Split animations-parsing.html into two tests

Currently it tests both style.animation and getComputedStyle.animation.
This CL splits the test into two tests, one for style.animation, and
the other one for getComputedStyle.animation such that each test is
minimal.

Bug:  772852 
Change-Id: I3c3f43305497ffe9d91c4e3806efe2e65ff17806
Reviewed-on: https://chromium-review.googlesource.com/c/1378230
Reviewed-by: Stephen McGruer <smcgruer@chromium.org>
Commit-Queue: Xida Chen <xidachen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#616677}
[rename] https://crrev.com/f48839c17381e751306a8a87571a1f060c609ea3/third_party/blink/web_tests/external/wpt/css/css-animations/computed-style-animation-parsing.html
[add] https://crrev.com/f48839c17381e751306a8a87571a1f060c609ea3/third_party/blink/web_tests/external/wpt/css/css-animations/style-animation-parsing.html

Comment 7 by xidac...@chromium.org, Dec 14

Status: Fixed (was: Assigned)

Sign in to add a comment