Skip to content

Commit

Permalink
[material-ui] Performance: lazy Ripple (#41061)
Browse files Browse the repository at this point in the history
  • Loading branch information
romgrk committed Jul 9, 2024
1 parent 43cb327 commit 68381cf
Show file tree
Hide file tree
Showing 27 changed files with 460 additions and 500 deletions.
1 change: 1 addition & 0 deletions packages-internal/test-utils/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
"@emotion/react": "^11.11.4",
"@testing-library/dom": "^10.3.1",
"@testing-library/react": "^16.0.0",
"@testing-library/user-event": "^14.5.2",
"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(),
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 '../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]);
}

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. */
<TouchRipple ref={handleRippleRef} center={centerRipple} {...TouchRippleProps} />
) : null}
</ButtonBaseRoot>
Expand Down
Loading

0 comments on commit 68381cf

Please sign in to comment.