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: Add test for Document::getAnimations across iframes #10291

Merged
merged 2 commits into from May 6, 2018

Conversation

Projects
None yet
5 participants
@stephenmcgruer
Contributor

stephenmcgruer commented Apr 3, 2018

No description provided.

@w3c-bots

This comment has been minimized.

w3c-bots commented Apr 3, 2018

Build PASSED

Started: 2018-04-04 11:56:33
Finished: 2018-04-04 12:07:49

View more information about this build on:

@birtles

This comment has been minimized.

Contributor

birtles commented Apr 4, 2018

Looks good, but I'm curious if we actually need the separate iframe document? Elsewhere we just create an empty <iframe>:

  • web-animations/timing-model/timelines/timelines.html
  • web-animations/interfaces/Animation/constructor.html

Or just use a locally defined empty <iframe>:

  • web-animations/interfaces/Animatable/animate.html
  • web-animations/interfaces/Document/timeline.html
const effect = new KeyframeEffect(
iframeTarget,
[ { backgroundColor: 'green' }, { backgroundColor: 'red' } ],
10000);

This comment has been minimized.

@birtles

birtles Apr 4, 2018

Contributor

10s is pretty short -- we should probably use 100 * MS_PER_SEC.

This comment has been minimized.

@stephenmcgruer

stephenmcgruer Apr 4, 2018

Contributor

Done

const iframeTarget = iframe.contentDocument.getElementById("target");
const effect = new KeyframeEffect(
iframeTarget,
[ { backgroundColor: 'green' }, { backgroundColor: 'red' } ],

This comment has been minimized.

@birtles

birtles Apr 4, 2018

Contributor

(Nit: I'm not sure we actually need any keyframes here. In the interest of keeping this test minimal perhaps we should drop them?)

This comment has been minimized.

@stephenmcgruer

stephenmcgruer Apr 4, 2018

Contributor

Done

@birtles

We might decide to drop the extra iframe document if it's not necessary, but at very least we should fix the 10s duration.

@stephenmcgruer

Removed the iframe, addressed the other comments.

const iframeTarget = iframe.contentDocument.getElementById("target");
const effect = new KeyframeEffect(
iframeTarget,
[ { backgroundColor: 'green' }, { backgroundColor: 'red' } ],

This comment has been minimized.

@stephenmcgruer

stephenmcgruer Apr 4, 2018

Contributor

Done

const effect = new KeyframeEffect(
iframeTarget,
[ { backgroundColor: 'green' }, { backgroundColor: 'red' } ],
10000);

This comment has been minimized.

@stephenmcgruer

stephenmcgruer Apr 4, 2018

Contributor

Done

@birtles

birtles approved these changes Apr 5, 2018

Thanks!

@birtles birtles merged commit b8639ae into web-platform-tests:master May 6, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@birtles

This comment has been minimized.

Contributor

birtles commented May 6, 2018

Looks like this never got merged. Sorry for the delay there.

@hiikezoe

This comment has been minimized.

Contributor

hiikezoe commented May 9, 2018

I am wondering we don't normally squash this kind of modifications into the first commit?

@birtles

This comment has been minimized.

Contributor

birtles commented May 10, 2018

I probably just hit the wrong button when I merged it.

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