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

[code-infra] Try disabling animations when taking screenshots #42537

Merged
merged 5 commits into from
Jun 14, 2024

Conversation

Janpot
Copy link
Member

@Janpot Janpot commented Jun 5, 2024

Attempt at reducing flakeyness. This is a follow-up on #42530. I'm not aware of the reasons we could have for keeping animations enabled during screenshot taking. But would love to hear opinions on this.

  1. Emulate prefers-reduced-motion, to disable animations in libraries that respect it.
  2. Disable CSS and web animations during screenshot taking

@Janpot Janpot added the scope: code-infra Specific to the core-infra product label Jun 5, 2024
@mui-bot
Copy link

mui-bot commented Jun 5, 2024

Netlify deploy preview

https://deploy-preview-42537--material-ui.netlify.app/

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 7158887

@Janpot Janpot marked this pull request as ready for review June 5, 2024 17:26
Copy link
Member

@alexfauquette alexfauquette left a comment

Choose a reason for hiding this comment

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

Sounds good. Could you revert the react-spting/web aspect of my PR since this one would make it useless?

@Janpot
Copy link
Member Author

Janpot commented Jun 6, 2024

🙂 Well, that broke argos. Will have to investigate what's breaking here, the emulation, or react spring detection of the flag.

Would you mind retracting your review so nobody accidentally merges this?

Copy link
Member

@alexfauquette alexfauquette left a comment

Choose a reason for hiding this comment

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

oupsi

@Janpot
Copy link
Member Author

Janpot commented Jun 6, 2024

@alexfauquette Been emulating reduced motion on the charts docs with the chrome devtools. It looks like it only disables animations during transitions, not during initial render:

Screen.Recording.2024-06-06.at.17.08.17.mov

Initial render of e.g. pie chart shows animation, even with prefers-reduced-motion

Screen.Recording.2024-06-06.at.17.07.52.mov

@alexfauquette
Copy link
Member

That's weird, because I can reproduce it in production, but not on localhost 🤯

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 8, 2024
@oliviertassinari
Copy link
Member

oliviertassinari commented Jun 9, 2024

Initial render of e.g. pie chart shows animation, even with prefers-reduced-motion

@Janpot To be clear today on https://mui.com/x/react-charts/#overview, what I can observe:

  • During hydration MUI X Charts animates no matter what
  • During a regular mount client-side, it respects prefers-reduced-motion: reduce.

I don't know, this feels like behaving as expected. During visual regressions, we don't have hydration.


Now, here, with the visual regressions, the problem might be timing-related. I guess by the time https://github.com/mui/mui-x/blob/ba8d2f2d08c0d1c1e48108004de7d01128d07ca6/packages/x-charts/src/hooks/useReducedMotion.ts#L13 run on the first mount (outside of hydration), it's already too late.

I have tried to dive deep into this but couldn't get to the bottom. I just suspect that it's React.lazy( that is at the root but this doesn't trigger the issue, so I don't know:

import * as React from 'react';
import { PieChart } from '@mui/x-charts/PieChart';
import { useDrawingArea } from '@mui/x-charts/hooks';
import { styled } from '@mui/material/styles';
import NoSsr from '@mui/material/NoSsr';

const data = [
  { label: 'India', value: 50000 },
  { label: 'USA', value: 35000 },
  { label: 'Brazil', value: 10000 },
  { label: 'Other', value: 5000 },
];

interface StyledTextProps {
  variant: 'primary' | 'secondary';
}

const StyledText = styled('text', {
  shouldForwardProp: (prop) => prop !== 'variant',
})<StyledTextProps>(({ theme, variant }) => ({
  textAnchor: 'middle',
  dominantBaseline: 'central',
  fill: theme.palette.text.secondary,
  fontSize:
    variant === 'primary'
      ? theme.typography.h5.fontSize
      : theme.typography.body2.fontSize,
  fontWeight:
    variant === 'primary'
      ? theme.typography.h5.fontWeight
      : theme.typography.body2.fontWeight,
}));

interface PieCenterLabelProps {
  primaryText: string;
  secondaryText: string;
}

function PieCenterLabel({ primaryText, secondaryText }: PieCenterLabelProps) {
  console.log('render', 'PieCenterLabel')
  const { width, height, left, top } = useDrawingArea();
  const primaryY = top + height / 2 - 10;
  const secondaryY = primaryY + 24;

  return (
    <React.Fragment>
      <StyledText variant="primary" x={left + width / 2} y={primaryY}>
        {primaryText}
      </StyledText>
      <StyledText variant="secondary" x={left + width / 2} y={secondaryY}>
        {secondaryText}
      </StyledText>
    </React.Fragment>
  );
}

const colors = [
  'hsl(220, 25%, 65%)',
  'hsl(220, 25%, 45%)',
  'hsl(220, 25%, 30%)',
  'hsl(220, 25%, 20%)',
];

function ChartUserByCountry() {
  console.log('render', 'Root')
  return (
    <div>
          <PieChart
            colors={colors}
            margin={{
              left: 80,
              right: 80,
              top: 80,
              bottom: 80,
            }}
            series={[
              {
                data,
                innerRadius: 75,
                outerRadius: 100,
                paddingAngle: 0,
                highlightScope: { faded: 'global', highlighted: 'item' },
              },
            ]}
            height={260}
            width={260}
            slotProps={{
              legend: { hidden: true },
            }}
          >
            <PieCenterLabel primaryText="98.5K" secondaryText="Total" />
          </PieChart>
    </div>
  );
}

function load() {
  return new Promise(resolve => {
    setTimeout(() => resolve({
      default: ChartUserByCountry
    }), 200);
  });
}

export default function Bar() {
  const Component = React.lazy(() => load());

  return (
    <NoSsr>
      <React.Suspense fallback={<div aria-busy />}>
        <Component />
      </React.Suspense>
    </NoSsr>
  );
}

Otherwise, there are ignore rules that we can't remove, from running the test regression env locally:

SCR-20240609-tqja

⚠️ prefers-reduced-motion: reduce doesn't mean no animation. #16367 (comment). It would be prefers-reduced-motion: no-motion if it was the case (this API doesn't exist). Now, since this charts is a pretty large surface on the screen, a reduced motion does feel like no motion in this context.

@Janpot
Copy link
Member Author

Janpot commented Jun 10, 2024

I don't know, this feels like behaving as expected.

My (naive) expectation would be that it SSRs the chart content and that an initial animation is not necessary at all since there is no change happening.

Assuming that this is not possible due to screen size differences, my real expectation is that it SSRs a placeholder, renders the same placeholder during hydration and then renders the chart content on the first render after hydration. with or without animation depending on the user preferences.

Side-note: this is something that bugs me in most charting libraries, I'm interested in transitions between two states of the chart, but never in animation on the first render. It's really weird that this is the default everywhere, you wouldn't do that with e.g. a grid, animate in every row on the first render. If it were up to me the default would be to never animate the initial render of the chart content, and have an explicit option to turn that on.

Not sure if it's related to the problem, but the component in the snippet calls React.lazy inside its render function.


⚠️ prefers-reduced-motion: reduce doesn't mean no animation.

Right, you can animate without movement, like a fade in. But it also doesn't mean "just tone down the motion a bit as you see fit". As an a11y setting, I'm pretty sure "reduce" means "remove all motion except when essential for the UX".

This does mean though that we need to globally turn off react-spring animations in our tests since for stabilizing the screenshots we really do want to disable all animations, even the ones that don't create movement.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 13, 2024
@Janpot
Copy link
Member Author

Janpot commented Jun 13, 2024

@alexfauquette I'm adding back the react spring global as we really want to disable all animation for screenshots, not just motion. Are you tracking the initial animation issue already somewhere?

@alexfauquette
Copy link
Member

Are you tracking the initial animation issue already somewhere?

The issue is create 👍

@Janpot Janpot merged commit 1e5df21 into mui:next Jun 14, 2024
22 checks passed
@Janpot Janpot deleted the disable-animations branch June 14, 2024 12:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope: code-infra Specific to the core-infra product test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants