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

WebKit export of https://bugs.webkit.org/show_bug.cgi?id=170784 #11266

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@birtles

This comment has been minimized.

Contributor

birtles commented Jun 1, 2018

We can't write this as a scripted test that just compares the values returned by getComputedStyle across animation frames?

(I'm concerned that this test will become flaky, or simply useless because on our CI we often can have tens of seconds between animation frames when the test machine is under load whereas this test seems to be dealing with changes in hundreds of milliseconds.)

@wpt-pr-bot wpt-pr-bot requested review from dbaron, grorg, plinss and tabatkins Jun 1, 2018

@fred-wang

This comment has been minimized.

Contributor

fred-wang commented Jun 1, 2018

We can't write this as a scripted test that just compares the values returned by getComputedStyle across animation frames?
(I'm concerned that this test will become flaky, or simply useless because on our CI we often can have tens of seconds between animation frames when the test machine is under load whereas this test seems to be dealing with changes in hundreds of milliseconds.)

I agree flakiness is a potential issue, that's why the test is not too strict: the animation not too fast and the covering rect is wider than the animated squares.

I initially tried doing getComputedStyle() but the issue is that the visual rendering does not necessary match the value returned from the DOM. In this particular WebKit iOS bug, the UI Process continues to animate the squares while the Web process claims they are paused.

@birtles

This comment has been minimized.

Contributor

birtles commented Jun 1, 2018

@fred-wang

This comment has been minimized.

Contributor

fred-wang commented Jun 1, 2018

OK, it worked for me locally but probably this is because the test is flaky. I think I can verify getComputedStyle() to make the test a bit more robust and also fire the pause in a small interval around the target time instead of only after it.

Make the test more robust:
- Increase the height of covering rect to avoid anti-aliasing issue with the red squares.
- Instead of waiting a delay, use the shift returned by getComputedStyle() to know when animated squares are supposed to be hidden by the green rect.
- Instead of waiting an additional amount of time to ensure red squares will show up again if they were not paused, just use the animation duration to decide when the screenshot should be taken.
@fred-wang

This comment has been minimized.

Contributor

fred-wang commented Jun 1, 2018

@birtles What do you think of this new version?

The parsing of window.getComputedStyle(...).transform is not really nice and the minimum duration is now always 2s but at least the whole thjing should be more accurate than relying on timestamp.

@fred-wang

This comment has been minimized.

Contributor

fred-wang commented Jun 1, 2018

It's failing again under Firefox Nightly, but I'm not able to reproduce the issue locally. Is there a way to get the screenshots from travis? Otherwise, I guess I should send the test to the Mozilla try sever...

@fred-wang

This comment has been minimized.

Contributor

fred-wang commented Jun 7, 2018

@birtles css/css-animations/set-animation-play-state-to-paused-001.html passes in Try server, so probably it's an issue with the Travis WPT bot:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=07483dcde214241025fc1ad7f0622bdcd3aeea9f

@birtles

This comment has been minimized.

Contributor

birtles commented Jun 7, 2018

@birtles css/css-animations/set-animation-play-state-to-paused-001.html passes in Try server, so probably it's an issue with the Travis WPT bot:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=07483dcde214241025fc1ad7f0622bdcd3aeea9f

@fred-wang That appears to be failing pretty consistently in the try run you linked to.

@fred-wang

This comment has been minimized.

Contributor

fred-wang commented Jun 8, 2018

@birtles /set-animation-play-state-to-paused-002.html is failing, not /set-animation-play-state-to-paused-001.html

@fred-wang

This comment has been minimized.

Contributor

fred-wang commented Jun 8, 2018

.

@fred-wang fred-wang closed this Jun 8, 2018

@fred-wang fred-wang deleted the fred-wang:wpt-export-for-webkit-170784 branch Jun 8, 2018

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