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

Schedule tasks for timeline updates #192

Merged

Conversation

johannesodland
Copy link
Contributor

@johannesodland johannesodland commented Dec 28, 2023

Scheduling a task to update timelines, instead of waiting for the next animation frame.

Some tests only wait for one animation frame. If we wait for the next animation frame before ticking animations, it might result in assertions running right before the animations are ticked.

Tests that that use waitForNextFrame() from web-animations/testcommon.js such as the last test in the scroll-animation-inactive-timeline. are affected by this issue.

This test should now pass.

@johannesodland johannesodland force-pushed the schedule-tasks-for-timeline-updates branch 5 times, most recently from ac3648e to ebe0be5 Compare January 3, 2024 15:00
@johannesodland
Copy link
Contributor Author

Fixing this causes the first subtest in /scroll-animations/scroll-timelines/scroll-timeline-invalidation.html to pass every now and then, depending on timing.

This subtest sets the height of the first child element as part of the test. To pass this subtest property I think we need to observe the children of the source element.

@flackr
You were originally opposed to observing the children, but without it some tests will be flaky. Are you ok with observing the children, or should we add other measures such as polyfill options to activate/deactivate observers first?

@flackr
Copy link
Owner

flackr commented Jan 25, 2024

You were originally opposed to observing the children, but without it some tests will be flaky. Are you ok with observing the children, or should we add other measures such as polyfill options to activate/deactivate observers first?

I'm okay with observing all the direct children. It's a bit unfortunate that we have to do this but seems like an okay tradeoff.

Copy link
Owner

@flackr flackr left a comment

Choose a reason for hiding this comment

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

LGTM

@johannesodland johannesodland force-pushed the schedule-tasks-for-timeline-updates branch from 858b288 to 1fba765 Compare January 25, 2024 22:18
@johannesodland johannesodland marked this pull request as ready for review January 25, 2024 22:34
@johannesodland johannesodland force-pushed the schedule-tasks-for-timeline-updates branch from 1fba765 to b39aa50 Compare January 27, 2024 11:36
@johannesodland johannesodland force-pushed the schedule-tasks-for-timeline-updates branch from b39aa50 to 3d8815b Compare January 29, 2024 17:12
@bramus bramus merged commit 4229714 into flackr:master Jan 29, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants