Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Serialize animation in the right order #14480

Merged
merged 1 commit into from
Dec 14, 2018

Conversation

chromium-wpt-export-bot
Copy link
Collaborator

@chromium-wpt-export-bot chromium-wpt-export-bot commented Dec 12, 2018

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}

Copy link
Collaborator

@wpt-pr-bot wpt-pr-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already reviewed downstream.

@chromium-wpt-export-bot chromium-wpt-export-bot force-pushed the chromium-export-cl-1367935 branch 3 times, most recently from edf6f91 to 31b765b Compare December 13, 2018 18:43
@birtles
Copy link
Contributor

birtles commented Dec 14, 2018

Unfortunately these tests rely on w3c/csswg-drafts#2529 so they will fail in Gecko and EdgeHTML. It would be nice if we can avoid making the tests fail in other UAs for reasons not related to the feature under test where possible.
/cc @xidachen @stephenmcgruer

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}
@stephenmcgruer
Copy link
Contributor

The copying of the animation is done via style not getComputedStyle, so it should be simple to split each test into two separate 'check style' and 'check getComputedStyle' version; the latter of which will fail in Gecko and EdgeHTML until they fix w3c/csswg-drafts#2529

WDYT @birtles ?

@stephenmcgruer
Copy link
Contributor

Brian - we believe we have addressed this in #14525 . There are now two test files; Gecko and EdgeHTML should pass one until they fix w3c/csswg-drafts#2529, after which they will pass both.

@birtles
Copy link
Contributor

birtles commented Dec 16, 2018

Thanks for doing that. Ideally we shouldn't rely on w3c/csswg-drafts#2529 since that resolution hasn't been properly specced yet. I was really just hoping we'd drop the following line:

style2.animation = computedStyle1.animation;

It should be possible to test this without that line, right?

@stephenmcgruer
Copy link
Contributor

It seems I was mistaken on how to treat w3c/csswg-drafts#2529 - I had taken w3c/csswg-drafts#2529 (comment) as a sign that vendors would implement the behavior. It seems a very worrying trend if a resolution was reached July 4th and six months later the spec hasn't changed :(.

Without relying on w3c/csswg-drafts#2529, we can test serialization of the animation property, which is css/css-animations/style-animation-parsing.html (which Firefox should pass - https://wpt.fyi/results/css/css-animations/style-animation-parsing.html). We cannot test serialization of the computed style of the animation property (because Firefox won't be serializing it).

I'm happy for you to send out a PR to remove the computed style version if you feel that we shouldn't rely on it.

@birtles
Copy link
Contributor

birtles commented Dec 16, 2018

Without relying on w3c/csswg-drafts#2529, we can test serialization of the animation property, which is css/css-animations/style-animation-parsing.html (which Firefox should pass - https://wpt.fyi/results/css/css-animations/style-animation-parsing.html). We cannot test serialization of the computed style of the animation property (because Firefox won't be serializing it).

I'm happy for you to send out a PR to remove the computed style version if you feel that we shouldn't rely on it.

Oh, actually never mind. I think what you have is fine for now. While I suspect EdgeHTML may not be updated to match that resolution, we at least have a bug for doing it in Gecko.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants