search

flackr / scroll-timeline

A polyfill of ScrollTimeline.
Apache License 2.0
951 stars 94 forks source link

Broken recently #183

Closed marcfilleul closed 9 months ago

marcfilleul commented 9 months ago

Hi,

I just checked websites where it was working correctly before and saw it wasn't working anymore.

Here are the console issues on Safari:

CleanShot 2023-12-23 at 14 00 35@2x

And Firefox:

CleanShot 2023-12-23 at 14 04 14@2x

Looks like it could be cause by recent updates around proxy-animations.

Best regards

bramus commented 9 months ago

Thanks for reporting. Things seems to have gone wrong indeed …

The first error about proxyAnimations is an error that had gone unnoticed as it only triggers in strict mode. This happens when you load the script as via type="module". I’ve got PR #184 up to fix this.

The second error is a bug introduced in #178, which broke the parsing of animation-range for ViewTimeline. I guess this went unnoticed as none of the demos actually uses an animation-range. I’ve got PR #185 up to fix that bug.

/cc @johannesodland @flackr

bramus commented 9 months ago

@marcfilleul The PRs I mentioned got merged. I believe these resolve the issues you reported. Can you confirm?

flackr commented 9 months ago

Thanks so much for the fixes @bramus

The first error about proxyAnimations is an error that had gone unnoticed as it only triggers in strict mode. This happens when you load the script as via type="module". I’ve got PR #184 up to fix this.

I filed #188 to ensure we test running the code in strict mode so this sort of thing should be caught.

The second error is a bug introduced in #178, which broke the parsing of animation-range for ViewTimeline. I guess this went unnoticed as none of the demos actually uses an animation-range. I’ve got PR #185 up to fix that bug.

We're running all of the scroll-animation wpt tests too, I wonder if it's one that we fail for some other reason already which resulted in it not being caught.

johannesodland commented 9 months ago

We're running all of the scroll-animation wpt tests too, I wonder if it's one that we fail for some other reason already which resulted in it not being caught.

I’ve got some time over the weekend. I’ll try to figure out which tests should have covered it, and see if we can get them to pass.

marcfilleul commented 9 months ago

@marcfilleul The PRs I mentioned got merged. I believe these resolve the issues you reported. Can you confirm?

Indeed, it works now! Thanks a lot 🥳