search

WordPress / gutenberg

The Block Editor project for WordPress and beyond. Plugin is available from the official repository.
https://wordpress.org/gutenberg/
Other
10.32k stars 4.12k forks source link

e2e tests: `framer-motion` animations are not disabled by plugin disabling CSS animations #43282

Open ciampo opened 2 years ago

ciampo commented 2 years ago

What

The disable-animation plugin runs during e2e tests in order to make them "more stable" (see original PR).

But this set of styles don't have an impact on animations happening via the framer-motion library (as noted by @talldan in https://github.com/WordPress/gutenberg/pull/43186#pullrequestreview-1072379034) — this is because framer-motion "manually" sets the animated values as inline styles for each frame (instead of relying on native CSS animations and transforms).

Proposed next steps

The best solution that comes to my mind is:

It should be noted that this is only a suboptimal solution:

An alternative approach could be to manually check for that global "e2e_disable_css_animations" variable in the React code, and conditionally render an alternative to framer-motion components that doesn't animate (i.e. a normal div instead of a motion.div) — although this approach would also mean that our e2e tests would not actually test the same exact components that are rendered in our user's browsers

talldan commented 2 years ago

Puppeteer does have a way to emulate reduced motion, but prior attempts to use it didn't work - https://github.com/WordPress/gutenberg/pull/18453.

There have been quite a few updates of puppeteer since, so things may be different.

ciampo commented 2 years ago

Should we be try to emulate reduced motion in addition to "force-disabling" all CSS animations, or as an alternative?

The original intention of the e2e tests plugin was to disable all animations in order to make e2e tests "more stable" — if that intention still holds true today, simulating reduced motion likely won't give the same results (some animations may still be happening) and we may need to find alternative options.

I also wonder if this plugin is necessary — as state before, by disabling all CSS animations we're making our e2e tests less realistic (compared to what the final user will interact with)

talldan commented 2 years ago

Should we be try to emulate reduced motion in addition to "force-disabling" all CSS animations, or as an alternative?

Possibly both? My main concern is test reliability, which is what makes me think both.

I also wonder if this plugin is necessary — as state before, by disabling all CSS animations we're making our e2e tests less realistic (compared to what the final user will interact with)

I think I've seen this discussion before somewhere (but I wasn't participating). For Puppeteer, the tests are already regularly flakey, and historically written in a way where they expect elements to be visible instantly. It may not be realistic to turn all animations on, a lot of tests would need to be updated to use waitForSelector.

For Playwright, the argument is a bit simpler. Animations being active does make tests run more slowly, but given Playwright does auto-waiting there should really be no difference between having animations on and off in terms of whether tests pass.

I think the best thing for the long term is to continue facilitating the migration to Playwright so that this is a simpler choice.

WunderBart commented 2 years ago

Unfortunately, some components still seem to fail with Playwright, even when the reduced motion is set and animations are disabled (natively). I recorded those components during the tests and noticed that they are animating into their default/initial state. I've checked if the transitionend event is fired for those components, and it is, just not instantly. This probably means that Playwright can't fast-forward those animations to completion as it claims in its documentation. I wouldn't say it's Playwright's fault, though – probably not even Framer-Motion's. At this point, I think it might have something to do with how we initialize those components that fail the screenshot test, as I don't think we want them to be animating into their initial states. Here's the list of those that I found to be problematic, along with the recordings of their initialization:

Hope it helps! 🙌

ciampo commented 2 years ago

Thank you @WunderBart !

We can look into removing initial animation from these components:

During your tests, were the rest of the framer-motion animations (i.e. the ones happening as the result of user interaction) detected correctly by Playwright ?

mirka commented 2 years ago

Sorry, I was under the impression that ToggleGroupControl was migrated to framer-motion but I guess it's not? It does its own transition handling.

ciampo commented 2 years ago

Sorry, I was under the impression that ToggleGroupControl was migrated to framer-motion but I guess it's not? It does its own transition handling.

I had started working on it — I opened a first attempt, which was then superseded by a second attempt using shared layout animations, which couldn't be merged because of a bug in framer-motion

ciampo commented 2 years ago

I also had another thought — if we wanted to disable framer-motion animations, what if we mocked framer-motion components?(motion.div mocked to render a div, ...)

WunderBart commented 2 years ago

During your tests, were the rest of the framer-motion animations (i.e. the ones happening as the result of user interaction) detected correctly by Playwright ?

@ciampo I haven't tested any user interactions yet - just the initial part. Which components do you think could be problematic? I'd be happy to do some more testing to learn more about FM behavior 👍

edit: I guess I can start from the FontSizePicker as it seems to be a good starting point 😄

ciampo commented 2 years ago

@ciampo I haven't tested any user interactions yet - just the initial part. Which components do you think could be problematic? I'd be happy to do some more testing to learn more about FM behavior 👍

edit: I guess I can start from the FontSizePicker as it seems to be a good starting point 😄

You could test FontSizePicker as it's an interesting one (even if it doesn't use framer-motion for animations)

Of the ones you listed, Popover would be a good candidate, since it features often in the editor and is often animated

WunderBart commented 2 years ago

You could test FontSizePicker as it's an interesting one (even if it doesn't use framer-motion for animations)

Huh, I should have checked myself, but I assumed that FontSizePicker does use the FM animation, hence problems with stability in Playwright as well. Why would Playwright fail handling those animations as well then? 🤔 This one's getting even more interesting. I'll keep digging. /cc @mirka

WunderBart commented 2 years ago

@ciampo I haven't tested any user interactions yet - just the initial part. Which components do you think could be problematic? I'd be happy to do some more testing to learn more about FM behavior 👍 edit: I guess I can start from the FontSizePicker as it seems to be a good starting point 😄

You could test FontSizePicker as it's an interesting one (even if it doesn't use framer-motion for animations)

Of the ones you listed, Popover would be a good candidate, since it features often in the editor and is often animated

OK, so I've been testing the FontSizePicker (non-FM) and Popover (FM) a bit more and it looks like they both respond well to explicit stability checks for their animated elements. Those elements would be the button highlight for the Picker and for the Popover, the popover element itself. Here's how a succesful interaction test could be written for the Picker:

// font-size-picker.test.ts

test( 'Can change size to "Small"', async ( { page } ) => {
    const button = await page.locator( 'button[aria-label="Small"]' );
    await button.click();
    const buttonHighlight = await page.waitForSelector(
        '[aria-label="Font size"] > div[role=presentation]'
    );
    await buttonHighlight?.waitForElementState( 'stable' );

    expect( button ).toHaveAttribute( 'aria-checked', 'true' );
    expect( await page.screenshot() ).toMatchSnapshot();
} );

...and for the Popover component:

// popover.test.ts

test( 'Should pop over', async ( { page } ) => {
    await page.click( 'role=button' );
    const popover = await page.waitForSelector( '.components-popover' );
    await popover.waitForElementState( 'stable' );

    expect( await page.screenshot() ).toMatchSnapshot();
} );

I'm still not sure why turning animation off doesn't work, but I don't think it's that important right now. If we can reliably test with animations on, I think we should do it as it has way more value than the other way around. It shows the intent behind those animations because we explicitly need to account for them in the tests. For example, those initial animations that I mentioned above - if we were able to just turn them off in the first place, the tests would just pass ignoring the fact that they might be an unwanted side-effect! 💪 Let me know what you think, @ciampo!

ciampo commented 2 years ago

I was reading about waitForElement( 'stable' ) and it looks like it will only work for animations that cause the element to "move", and therefore it shouldn't be able to support waiting for, i.e., opacity changes — not sure if that would cause any issues.

Out of curiosity, would the .waitForElementState( 'stable' ); technique work also for other e2e tests that are not screenshot/snapshot based, for both non-FM and FM based animating components?

Would there ever be the option to remove the disable-animation plugin and just use .waitForElementState( 'stable' ); instead?

Specifically to screenshots, it looks like the toHaveScreenshot assertion should disable animations by default, although I'm not sure it would work with FM components

WunderBart commented 2 years ago

I was reading about waitForElement( 'stable' ) and https://github.com/microsoft/playwright/issues/4055 it will only work for animations that cause the element to "move", and therefore it shouldn't be able to support waiting for, i.e., opacity changes — not sure if that would cause any issues.

If the only thing we need to wait for is the element's opacity (its bounding box is static), then for the screenshot to be done at the right moment, I suppose we'd need a custom waiter. This should be doable as we can wait for the opacity via getComputedStyle() in a similar manner as the stability check.

Out of curiosity, would the .waitForElementState( 'stable' ); technique work also for other e2e tests that are not screenshot/snapshot based, for both non-FM and FM based animating components?

Yes, as long as the bounding box of the element is moving, the stability check should keep working.

Specifically to screenshots, it looks like the toHaveScreenshot assertion should disable animations by default, although I'm not sure it would work with FM components

It doesn't look to be working ATM reliably, true. In my testing, it didn't even work reliably for the FontSizePicker component (non-FM), but as I mentioned above, it could be because the initial animation is triggered after the component is rendered. Anyway, I think we can drop the pursuit of disabling animations and include them in the tests as it provides more value and is well handled by Playwright.

WunderBart commented 2 years ago

Would there ever be the option to remove the disable-animation plugin and just use .waitForElementState( 'stable' ); instead?

Definitely for Playwright, which I think should be done by default. Are we disabling animations for Playwright as we did for Puppeteer? cc @kevin940726

kevin940726 commented 2 years ago

Nope. We are not activating that plugin in Playwright tests.

If somehow something breaks while screenshotting, friendly reminder that we can always use expect.poll to wait for something to happen as an escape hatch.

await expect.poll(
    () => locator.evaluate( ( node ) => getComputedStyle( node ).opacity )
).toBe( '1' )