|
|
||||
Issue descriptionSee 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." , Sep 7
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 , Dec 4
, 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 , Dec 14Project MemberThe 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 , Dec 14Project MemberThe 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 , Dec 14
|
||||
►
Sign in to add a comment |
||||
Comment 1 by ericwilligers@chromium.org, Oct 16 2017