Conversation
ydaniv
left a comment
There was a problem hiding this comment.
So far reviewed animation-group, effects, and pointer-animations.
Let's fix those and continue
| "@babel/code-frame@npm:^7.10.4, @babel/code-frame@npm:^7.28.6": | ||
| version: 7.28.6 | ||
| resolution: "@babel/code-frame@npm:7.28.6" | ||
| "@babel/code-frame@npm:^7.10.4, @babel/code-frame@npm:^7.28.6, @babel/code-frame@npm:^7.29.0": |
There was a problem hiding this comment.
Looks like we have misaligned versions in the different package.json files.
I can see one is @vitejs/plugin-react.
Please see if you can dedupe those.
| ) as HTMLElement; | ||
|
|
||
| // X-axis animation: translateX driven by horizontal mouse position | ||
| const xAxisGroup = getWebAnimation(xAxisTarget, { |
There was a problem hiding this comment.
All of the pointer fixtures use getWebAnimation(), none actually use getScrubScene() which is what we actually use for all pointer-move animations.
So none actually check the correct code path for pointer animations.
Let's correct that, so it's actually passing the correct trigger data, with different axes, using all the type of effects, etc.
| return Math.max(0, Math.min(1, progress)); | ||
| } | ||
|
|
||
| const animationGroup = getWebAnimation(target, { |
There was a problem hiding this comment.
Looks like none of the examples in here actually uses native scroll-driven animations.
Try asking cursor to test both native SDAs and using getScrubScene which uses a polyfill.
| await scrollPage.goto(); | ||
| }); | ||
|
|
||
| test.describe('View Progress Trigger', () => { |
There was a problem hiding this comment.
This suite should attempt testing native SDAs if supported, and also using getScrubScene
| }); | ||
| }); | ||
|
|
||
| test.describe('Scrub Scene', () => { |
There was a problem hiding this comment.
Motion allows calling getScrubScene both when ViewTimeline is supported natively and when not.
Then native use-case is for customEffects. Could be nice to test that.
The non-native support is also for the simple polyfill flow of simple named/keyframe effects, should be tested as well.
| const progressAfterScroll = await scrollPage.getScrollProgress(); | ||
| expect(progressAfterScroll).toBeGreaterThan(0); |
There was a problem hiding this comment.
This is not testing the animation in any way. It's just checking scroll progress which it gets from measuring stuff from the page.
| (window as ScrollFixtureWindow).getScrubSceneMode = () => | ||
| Array.isArray(rangeSceneResult) ? 'polyfill' : 'native'; |
There was a problem hiding this comment.
This could easily break if we change implementation. Let's change it to check having scrub scenes and supporting ViewTimeline or not. Should be enough
| expect(sceneStart).toBeNull(); | ||
| expect(sceneEnd).toBeNull(); |
There was a problem hiding this comment.
This is also silly, probably because the scene is an array
| test.skip(!supportsNative, 'Native ViewTimeline is required for this customEffect flow.'); | ||
|
|
||
| await scrollPage.scrollTo(0); | ||
| await scrollPage.wait(60); |
There was a problem hiding this comment.
See if we can generally avoid calls to wait and ask Cursor to scroll and check ASAP. All those waits add up to slow tests
There was a problem hiding this comment.
Ok, it suggested to use playwright's toPass(), which is a retrying assertion. Seems to do the trick!
| expect(after.shift).toBeGreaterThan(before.shift); | ||
| }); | ||
|
|
||
| test('should expose getScrubScene range offsets in fallback metadata', async () => { |
There was a problem hiding this comment.
I'm not sure what this test checks... any idea?
There was a problem hiding this comment.
Not really e2e material, I removed it
|
|
||
| test('should handle destroy cleanup properly', async ({ page }) => { | ||
| await scrollPage.cancelScrubScene(); | ||
| await waitForWindowPlayState(page, 'scrubScene', ['idle', 'paused'], 5000); |
There was a problem hiding this comment.
I think we want to only allow for idle there. Please check
There was a problem hiding this comment.
paused was allowed because there was a race condition between setting idle state via animationGroup.cancel() and the scroll listener callback that was still setting progress. I added a flag scrubSceneDestroyed to avoid this and only enable idle.
Description
Related Issue
Checklist
Screenshots / Demos
Additional Notes