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

[material-ui] Performance: lazy Ripple #41061

Merged
merged 30 commits into from
Jul 9, 2024
Merged
Show file tree
Hide file tree
Changes from 22 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion packages-internal/test-utils/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,9 @@
"@babel/runtime": "^7.24.7",
"@emotion/cache": "^11.11.0",
"@emotion/react": "^11.11.4",
"@testing-library/dom": "^10.2.0",
"@testing-library/dom": "10.2.0",
"@testing-library/react": "^16.0.0",
"@testing-library/user-event": "^14.5.2",
romgrk marked this conversation as resolved.
Show resolved Hide resolved
"chai": "^4.4.1",
"chai-dom": "^1.12.0",
"dom-accessibility-api": "^0.6.3",
Expand Down
3 changes: 3 additions & 0 deletions packages-internal/test-utils/src/createRenderer.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import {
within,
RenderResult,
} from '@testing-library/react/pure';
import { userEvent } from '@testing-library/user-event';
import { useFakeTimers } from 'sinon';

interface Interaction {
Expand Down Expand Up @@ -268,6 +269,7 @@ interface ServerRenderConfiguration extends RenderConfiguration {
export type RenderOptions = Partial<RenderConfiguration>;

export interface MuiRenderResult extends RenderResult<typeof queries & typeof customQueries> {
user: ReturnType<typeof userEvent.setup>;
forceUpdate(): void;
/**
* convenience helper. Better than repeating all props.
Expand Down Expand Up @@ -296,6 +298,7 @@ function render(
);
const result: MuiRenderResult = {
...testingLibraryRenderResult,
user: userEvent.setup(),
Copy link
Member

Choose a reason for hiding this comment

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

This will be super useful for all tests in the future 🎉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I'll post about it in slack once this is merged.

Copy link
Member

@oliviertassinari oliviertassinari Aug 30, 2024

Choose a reason for hiding this comment

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

This looks wrong in two separate ways:

  1. The API methods exposed from render() are supposed to be scoped to the element. It's not the case with user.
  2. The testing strategy contradicts [code-infra] Add react-testing-library eslint plugin mui-public#173 where the objective is to use a global import to simplify the testing DX.

It feels like we should deprecate this:

Suggested change
user: userEvent.setup(),
/** @deprecated import { user } instead */
user: userEvent.setup(),

and we export user as a module like we do for screen.

cc @Janpot for awareness. I landed here from: mui/mui-x#14142 (comment) from KevinVandy/material-react-table#1231 from https://x.com/XCSme/status/1829522518517956996

forceUpdate() {
traceSync('forceUpdate', () =>
testingLibraryRenderResult.rerender(
Expand Down
4 changes: 4 additions & 0 deletions packages-internal/test-utils/src/focusVisible.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,10 @@ export function simulatePointerDevice() {
fireEvent.pointerDown(document.body);
}

export function simulateKeyboardDevice() {
fireEvent.keyDown(document.body, { key: 'TAB' });
}

/**
* See https://bugs.chromium.org/p/chromium/issues/detail?id=1127875 for more details.
*/
Expand Down
1 change: 1 addition & 0 deletions packages-internal/test-utils/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ export * from './createRenderer';
export {
default as focusVisible,
simulatePointerDevice,
simulateKeyboardDevice,
programmaticFocusTriggersFocusVisible,
} from './focusVisible';
export {} from './initMatchers';
Expand Down
12 changes: 6 additions & 6 deletions packages/mui-joy/src/MenuItem/MenuItem.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -92,18 +92,18 @@ describe('Joy <MenuItem />', () => {
];

events.forEach((eventName) => {
it(`should fire ${eventName}`, () => {
it(`should fire ${eventName}`, async () => {
const handlerName = `on${eventName[0].toUpperCase()}${eventName.slice(1)}`;
const handler = spy();
render(<MenuItem {...{ [handlerName]: handler }} />);

fireEvent[eventName](screen.getByRole('menuitem'));
await act(async () => fireEvent[eventName](screen.getByRole('menuitem')));

expect(handler.callCount).to.equal(1);
});
});

it(`should fire focus, keydown, keyup and blur`, () => {
it(`should fire focus, keydown, keyup and blur`, async () => {
const handleFocus = spy();
const handleKeyDown = spy();
const handleKeyUp = spy();
Expand All @@ -119,17 +119,17 @@ describe('Joy <MenuItem />', () => {
);
const menuitem = screen.getByRole('menuitem');

act(() => {
await act(async () => {
menuitem.focus();
});

expect(handleFocus.callCount).to.equal(1);

fireEvent.keyDown(menuitem);
await act(async () => fireEvent.keyDown(menuitem));

expect(handleKeyDown.callCount).to.equal(1);

fireEvent.keyUp(menuitem);
await act(async () => fireEvent.keyUp(menuitem));

expect(handleKeyUp.callCount).to.equal(1);

Expand Down
30 changes: 14 additions & 16 deletions packages/mui-material/src/Button/Button.test.js
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
import * as React from 'react';
import { expect } from 'chai';
import { act, createRenderer, fireEvent, screen } from '@mui/internal-test-utils';
import { createRenderer, screen, simulateKeyboardDevice } from '@mui/internal-test-utils';
import { ClassNames } from '@emotion/react';
import { ThemeProvider, createTheme } from '@mui/material/styles';
import Button, { buttonClasses as classes } from '@mui/material/Button';
import ButtonBase, { touchRippleClasses } from '@mui/material/ButtonBase';
import describeConformance from '../../test/describeConformance';
import * as ripple from '../../test/ripple';

describe('<Button />', () => {
const { render, renderToString } = createRenderer();
Expand Down Expand Up @@ -555,23 +556,23 @@ describe('<Button />', () => {
expect(endIcon).not.to.have.class(classes.startIcon);
});

it('should have a ripple by default', () => {
it('should have a ripple', async () => {
const { getByRole } = render(
<Button TouchRippleProps={{ className: 'touch-ripple' }}>Hello World</Button>,
);
const button = getByRole('button');

await ripple.startTouch(button);
expect(button.querySelector('.touch-ripple')).not.to.equal(null);
});

it('can disable the ripple', () => {
it('can disable the ripple', async () => {
const { getByRole } = render(
<Button disableRipple TouchRippleProps={{ className: 'touch-ripple' }}>
Hello World
</Button>,
);
const button = getByRole('button');

await ripple.startTouch(button);
expect(button.querySelector('.touch-ripple')).to.equal(null);
});

Expand All @@ -582,7 +583,7 @@ describe('<Button />', () => {
expect(button).to.have.class(classes.disableElevation);
});

it('should have a focusRipple by default', function test() {
it('should have a focusRipple', async function test() {
if (/jsdom/.test(window.navigator.userAgent)) {
// JSDOM doesn't support :focus-visible
this.skip();
Expand All @@ -595,15 +596,13 @@ describe('<Button />', () => {
);
const button = getByRole('button');

fireEvent.keyDown(document.body, { key: 'TAB' });
act(() => {
button.focus();
});
simulateKeyboardDevice();
await ripple.startFocus(button);

expect(button.querySelector('.pulsate-focus-visible')).not.to.equal(null);
});

it('can disable the focusRipple', function test() {
it('can disable the focusRipple', async function test() {
if (/jsdom/.test(window.navigator.userAgent)) {
// JSDOM doesn't support :focus-visible
this.skip();
Expand All @@ -619,10 +618,8 @@ describe('<Button />', () => {
);
const button = getByRole('button');

act(() => {
fireEvent.keyDown(document.body, { key: 'TAB' });
button.focus();
});
simulateKeyboardDevice();
await ripple.startFocus(button);

expect(button.querySelector('.pulsate-focus-visible')).to.equal(null);
});
Expand Down Expand Up @@ -676,7 +673,7 @@ describe('<Button />', () => {
expect(container.firstChild.querySelector(`.${touchRippleClasses.root}`)).to.equal(null);
});

it("should disable ripple when MuiButtonBase has disableRipple in theme's defaultProps but override on the individual Buttons if provided", () => {
it("should disable ripple when MuiButtonBase has disableRipple in theme's defaultProps but override on the individual Buttons if provided", async () => {
const theme = createTheme({
components: {
MuiButtonBase: {
Expand All @@ -693,6 +690,7 @@ describe('<Button />', () => {
<Button>Disabled ripple 2</Button>
</ThemeProvider>,
);
await ripple.startTouch(container.querySelector('button'));
expect(container.querySelectorAll(`.${touchRippleClasses.root}`)).to.have.length(1);
});

Expand Down
56 changes: 15 additions & 41 deletions packages/mui-material/src/ButtonBase/ButtonBase.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { styled } from '../zero-styled';
import { useDefaultProps } from '../DefaultPropsProvider';
import useForkRef from '../utils/useForkRef';
import useEventCallback from '../utils/useEventCallback';
import useLazyRipple from '../useTouchRipple/useLazyRipple';
import TouchRipple from './TouchRipple';
import buttonBaseClasses, { getButtonBaseUtilityClass } from './buttonBaseClasses';

Expand Down Expand Up @@ -109,8 +110,8 @@ const ButtonBase = React.forwardRef(function ButtonBase(inProps, ref) {

const buttonRef = React.useRef(null);

const rippleRef = React.useRef(null);
const handleRippleRef = useForkRef(rippleRef, touchRippleRef);
const ripple = useLazyRipple();
const handleRippleRef = useForkRef(ripple.ref, touchRippleRef);

const [focusVisible, setFocusVisible] = React.useState(false);
if (disabled && focusVisible) {
Expand All @@ -128,19 +129,13 @@ const ButtonBase = React.forwardRef(function ButtonBase(inProps, ref) {
[],
);

const [mountedState, setMountedState] = React.useState(false);
const enableTouchRipple = ripple.shouldMount && !disableRipple && !disabled;

React.useEffect(() => {
setMountedState(true);
}, []);

const enableTouchRipple = mountedState && !disableRipple && !disabled;

React.useEffect(() => {
if (focusVisible && focusRipple && !disableRipple && mountedState) {
rippleRef.current.pulsate();
if (focusVisible && focusRipple && !disableRipple) {
ripple.pulsate();
}
}, [disableRipple, focusRipple, focusVisible, mountedState]);
}, [disableRipple, focusRipple, focusVisible, ripple]);

function useRippleHandler(rippleAction, eventCallback, skipRippleAction = disableTouchRipple) {
return useEventCallback((event) => {
Expand All @@ -149,8 +144,8 @@ const ButtonBase = React.forwardRef(function ButtonBase(inProps, ref) {
}

const ignore = skipRippleAction;
if (!ignore && rippleRef.current) {
rippleRef.current[rippleAction](event);
if (!ignore) {
ripple[rippleAction](event);
}

return true;
Expand Down Expand Up @@ -212,9 +207,9 @@ const ButtonBase = React.forwardRef(function ButtonBase(inProps, ref) {

const handleKeyDown = useEventCallback((event) => {
// Check if key is already down to avoid repeats being counted as multiple activations
if (focusRipple && !event.repeat && focusVisible && rippleRef.current && event.key === ' ') {
rippleRef.current.stop(event, () => {
rippleRef.current.start(event);
if (focusRipple && !event.repeat && focusVisible && event.key === ' ') {
ripple.stop(event, () => {
ripple.start(event);
});
}

Expand Down Expand Up @@ -243,15 +238,9 @@ const ButtonBase = React.forwardRef(function ButtonBase(inProps, ref) {
const handleKeyUp = useEventCallback((event) => {
// calling preventDefault in keyUp on a <button> will not dispatch a click event if Space is pressed
// https://codesandbox.io/p/sandbox/button-keyup-preventdefault-dn7f0
if (
focusRipple &&
event.key === ' ' &&
rippleRef.current &&
focusVisible &&
!event.defaultPrevented
) {
rippleRef.current.stop(event, () => {
rippleRef.current.pulsate(event);
if (focusRipple && event.key === ' ' && focusVisible && !event.defaultPrevented) {
ripple.stop(event, () => {
ripple.pulsate(event);
});
}
if (onKeyUp) {
Expand Down Expand Up @@ -291,20 +280,6 @@ const ButtonBase = React.forwardRef(function ButtonBase(inProps, ref) {

const handleRef = useForkRef(ref, buttonRef);

if (process.env.NODE_ENV !== 'production') {
// eslint-disable-next-line react-hooks/rules-of-hooks
React.useEffect(() => {
if (enableTouchRipple && !rippleRef.current) {
console.error(
[
'MUI: The `component` prop provided to ButtonBase is invalid.',
'Please make sure the children prop is rendered in this custom component.',
].join('\n'),
);
}
}, [enableTouchRipple]);
}
Comment on lines -294 to -306
Copy link
Contributor Author

@romgrk romgrk Jun 6, 2024

Choose a reason for hiding this comment

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

As the ripple is now lazy, using it for this devmode check doesn't make sense anymore. If someone has an idea for an alternative I can restore this warning.

Copy link
Member

@oliviertassinari oliviertassinari Jun 10, 2024

Choose a reason for hiding this comment

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

The history: #20401. I guess taking the initial issue reproduction, and adding the warning where the code crash would do it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The initial issue would not happen anymore. With the previous implementation, not rendering the ripple in a custom button component would cause a crash when calling API methods because the ripple ref would be null. With the current implementation, the ripple API calls only go through once the ripple is mounted and its ref is not null; not rendering/mounting the ripple would simply create a promise that never resolves. So the check can't be used anymore, and I don't see an easy way to add back this warning.


const ownerState = {
...props,
centerRipple,
Expand Down Expand Up @@ -345,7 +320,6 @@ const ButtonBase = React.forwardRef(function ButtonBase(inProps, ref) {
>
{children}
{enableTouchRipple ? (
/* TouchRipple is only needed client-side, x2 boost on the server. */
romgrk marked this conversation as resolved.
Show resolved Hide resolved
<TouchRipple ref={handleRippleRef} center={centerRipple} {...TouchRippleProps} />
) : null}
</ButtonBaseRoot>
Expand Down
Loading