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

[web-animations] Update timing interfaces #10047

Merged
merged 16 commits into from Mar 16, 2018

Conversation

Projects
None yet
4 participants
@birtles
Copy link
Contributor

birtles commented Mar 15, 2018

These are the tests for w3c/csswg-drafts#2432

birtles added some commits Mar 14, 2018

[web-animations] Simplify various tests in interfaces
Later in this patch series we will move these tests around and rework
them so in this patch we:

* Replace unnecessary local 'div' variables.
* Replace unnecessary 'opacity' keyframes where 'null' is sufficient.
[web-animations] Move tests for default timing set by Animatable.anim…
…ate()

Later in this patch series we want to update the tests for the
AnimationEffectReadOnly timing interface (since it is going away) but
some of those tests are currently in the wrong place---that is, they are
not actually testing that interface.

This patch moves a subset of those tests to the location corresponding
to the part of the spec they are actually testing.
[web-animations] Tidy up tests for delay property
I deliberated a lot about where these tests should live. In a sense they
are testing the timing model and specifically the calculation of the
active time. However, we already have tests for those timing
calculations.

What these tests are really covering is that we can dynamically change
properties using the API and have the timing model update. So it makes
sense to keep them in the 'interfaces' folder but just make clear
exactly what they are testing.
[web-animations] Tidy up the tests for easing property
I dropped the roundtrip tests since in this case they are a strict
subset of the other tests based on gEasingTests.
[web-animations] Remove a number of references to *ReadOnly interfaces
This patch also removes the test that checks that the copy constructor
for KeyframeEffect produces a mutable copy of a KeyframeEffectReadOnly
since the mutable/immutable distinction no longer exists.

@birtles birtles self-assigned this Mar 15, 2018

@wpt-pr-bot

This comment has been minimized.

Copy link
Collaborator

wpt-pr-bot commented Mar 15, 2018

There are no owners for this pull request besides its author. Please reach out on W3C's irc server (irc.w3.org, port 6665) on channel #testing (web client) to get help with this. Thank you!

@birtles

This comment has been minimized.

Copy link
Contributor Author

birtles commented Mar 15, 2018

Reached out on IRC to try and get @flackr @graouts and @stephenmcgruer to be able to review PRs to web-animations but am still awaiting a response.

@w3c-bots

This comment has been minimized.

Copy link

w3c-bots commented Mar 15, 2018

Build PASSED

Started: 2018-03-16 01:04:25
Finished: 2018-03-16 01:27:58

View more information about this build on:

@stephenmcgruer
Copy link
Contributor

stephenmcgruer left a comment

Generally LGTM, a bit difficult to review such a large change. One comment that I don't quite follow.

I notice from this that the new API also makes doing 'double the duration' awkward which is a pity :(.

@@ -91,13 +93,15 @@
const div = createDiv(t);
const animation = div.animate(null, 100 * MS_PER_SEC);
animation.effect.timing.endDelay = -200 * MS_PER_SEC;
animation.effect.updateTiming({
endDelay: animation.effect.getTiming().endDelay - 200 * MS_PER_SEC,

This comment has been minimized.

Copy link
@stephenmcgruer

stephenmcgruer Mar 15, 2018

Contributor

I don't follow this change. The previous version just set it to -200 * MS_PER_SEC, but this version decrements the existing value by 200 * MS_PER_SEC - why ?

This comment has been minimized.

Copy link
@birtles

birtles Mar 15, 2018

Author Contributor

That's a bug in the patch. Thanks!

[web-animations] Update various references to an effect's `timing` me…
…mber

This patch also drops the test that an AnimationEffectTiming object is
created in the appropriate realm since it is no longer the case that
a separate timing object is created.

@birtles birtles force-pushed the birtles:update-timing-interfaces branch from eb9f8a9 to 509341b Mar 16, 2018

@birtles

This comment has been minimized.

Copy link
Contributor Author

birtles commented Mar 16, 2018

I notice from this that the new API also makes doing 'double the duration' awkward which is a pity :(.

Yeah, that operation is definitely harder. I wonder in practice how often you do that, however, and how big a deal it is at the time anyway (React programmers anyway are used to this sort of pattern with setState() etc.)

@birtles birtles merged commit b73f249 into web-platform-tests:master Mar 16, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.