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

[Button][base] Prevent too many ref updates #33882

Merged
merged 7 commits into from Aug 29, 2022
26 changes: 17 additions & 9 deletions packages/mui-base/src/ButtonUnstyled/useButton.ts
@@ -1,6 +1,5 @@
import * as React from 'react';
import {
unstable_setRef as setRef,
unstable_useForkRef as useForkRef,
unstable_useIsFocusVisible as useIsFocusVisible,
} from '@mui/utils';
Expand All @@ -9,7 +8,15 @@ import extractEventHandlers from '../utils/extractEventHandlers';
import { EventHandlers } from '../utils/types';

export default function useButton(parameters: UseButtonParameters) {
const { disabled = false, focusableWhenDisabled, href, ref, tabIndex, to, type } = parameters;
const {
disabled = false,
focusableWhenDisabled,
href,
ref: externalRef,
tabIndex,
to,
type,
} = parameters;
const buttonRef = React.useRef<HTMLButtonElement | HTMLAnchorElement | HTMLElement>();

const [active, setActive] = React.useState<boolean>(false);
Expand Down Expand Up @@ -148,13 +155,14 @@ export default function useButton(parameters: UseButtonParameters) {
}
};

const handleOwnRef = useForkRef(focusVisibleRef, buttonRef);
const handleRef = useForkRef(ref, handleOwnRef);

const updateRef = (instance: HTMLElement | null) => {
const updateHostElementName = React.useCallback((instance: HTMLElement | null) => {
setHostElementName(instance?.tagName ?? '');
setRef(handleRef, instance);
};
}, []);

const handleRef = useForkRef(
updateHostElementName,
useForkRef(externalRef, useForkRef(focusVisibleRef, buttonRef)),
);

interface AdditionalButtonProps {
type?: React.ButtonHTMLAttributes<HTMLButtonElement>['type'];
Expand Down Expand Up @@ -209,7 +217,7 @@ export default function useButton(parameters: UseButtonParameters) {
onMouseDown: createHandleMouseDown(externalEventHandlers),
onMouseLeave: createHandleMouseLeave(externalEventHandlers),
onMouseUp: createHandleMouseUp(externalEventHandlers),
ref: updateRef as React.Ref<any>,
ref: handleRef,
};
};

Expand Down
147 changes: 147 additions & 0 deletions packages/mui-base/src/ListboxUnstyled/useControllableReducer.test.tsx
@@ -0,0 +1,147 @@
import { expect } from 'chai';
import * as React from 'react';
import { spy } from 'sinon';
import { createRenderer } from 'test/utils';
import useControllableReducer from './useControllableReducer';
import {
ActionTypes,
ListboxAction,
ListboxState,
UseListboxPropsWithDefaults,
} from './useListbox.types';

describe('useControllableReducer', () => {
const { render } = createRenderer();

describe('dispatch', () => {
it('calls the provided internal reducer', () => {
// eslint-disable-next-line @typescript-eslint/no-unused-vars
const reducer = spy((state: ListboxState<string>, action: ListboxAction<string>) => {
return state;
});

const actionToDispatch = { type: ActionTypes.setValue as const, value: 'b' };
const TestComponent = () => {
const props: UseListboxPropsWithDefaults<string> = {
options: ['a', 'b', 'c'],
defaultValue: 'a',
isOptionDisabled: () => false,
disableListWrap: false,
disabledItemsFocusable: false,
optionComparer: (a, b) => a === b,
optionStringifier: (option) => option,
multiple: false,
};
const [, dispatch] = useControllableReducer(reducer, undefined, props);
React.useEffect(() => dispatch(actionToDispatch), [dispatch]);
return null;
};

render(<TestComponent />);
expect(reducer.getCalls()[0].args[1]).to.equal(actionToDispatch);
});

it('calls the provided external reducer', () => {
// eslint-disable-next-line @typescript-eslint/no-unused-vars
const internalReducer = spy((state: ListboxState<string>, action: ListboxAction<string>) => {
return state;
});

// eslint-disable-next-line @typescript-eslint/no-unused-vars
const externalReducer = spy((state: ListboxState<string>, action: ListboxAction<string>) => {
return state;
});

const actionToDispatch = { type: ActionTypes.setValue as const, value: 'b' };
const TestComponent = () => {
const props: UseListboxPropsWithDefaults<string> = {
options: ['a', 'b', 'c'],
defaultValue: 'a',
isOptionDisabled: () => false,
disableListWrap: false,
disabledItemsFocusable: false,
optionComparer: (a, b) => a === b,
optionStringifier: (option) => option,
multiple: false,
};
const [, dispatch] = useControllableReducer(internalReducer, externalReducer, props);
React.useEffect(() => dispatch(actionToDispatch), [dispatch]);
return null;
};

render(<TestComponent />);
expect(internalReducer.notCalled).to.equal(true);
expect(externalReducer.getCalls()[0].args[1]).to.equal(actionToDispatch);
});

it('calls onChange when the reducer returns a modified selected value', () => {
const reducer = spy((state: ListboxState<string>) => {
return {
...state,
selectedValue: 'b',
};
});

const actionToDispatch = { type: ActionTypes.setValue as const, value: 'b' };
const handleChange = spy();
const handleHighlightChange = spy();

const TestComponent = () => {
const props: UseListboxPropsWithDefaults<string> = {
options: ['a', 'b', 'c'],
defaultValue: 'a',
isOptionDisabled: () => false,
disableListWrap: false,
disabledItemsFocusable: false,
optionComparer: (a, b) => a === b,
optionStringifier: (option) => option,
multiple: false,
onChange: handleChange,
onHighlightChange: handleHighlightChange,
};
const [, dispatch] = useControllableReducer(reducer, undefined, props);
React.useEffect(() => dispatch(actionToDispatch), [dispatch]);
return null;
};

render(<TestComponent />);
expect(handleChange.getCalls()[0].args[0]).to.equal('b');
expect(handleHighlightChange.notCalled).to.equal(true);
});

it('calls onHighlightChange when the reducer returns a modified highlighted value', () => {
const reducer = spy((state: ListboxState<string>) => {
return {
...state,
highlightedValue: 'b',
};
});

const actionToDispatch = { type: ActionTypes.setHighlight as const, highlight: 'b' };
const handleChange = spy();
const handleHighlightChange = spy();

const TestComponent = () => {
const props: UseListboxPropsWithDefaults<string> = {
options: ['a', 'b', 'c'],
defaultValue: 'a',
isOptionDisabled: () => false,
disableListWrap: false,
disabledItemsFocusable: false,
optionComparer: (a, b) => a === b,
optionStringifier: (option) => option,
multiple: false,
onChange: handleChange,
onHighlightChange: handleHighlightChange,
};
const [, dispatch] = useControllableReducer(reducer, undefined, props);
React.useEffect(() => dispatch(actionToDispatch), [dispatch]);
return null;
};

render(<TestComponent />);
expect(handleHighlightChange.getCalls()[0].args[0]).to.equal('b');
expect(handleChange.notCalled).to.equal(true);
});
});
});
19 changes: 16 additions & 3 deletions packages/mui-base/src/ListboxUnstyled/useControllableReducer.ts
Expand Up @@ -45,12 +45,16 @@ function useStateChangeDetection<TOption>(
nextState: ListboxState<TOption>,
internalPreviousState: ListboxState<TOption>,
propsRef: React.RefObject<UseListboxPropsWithDefaults<TOption>>,
hasDispatchedActionRef: React.MutableRefObject<boolean>,
) {
React.useEffect(() => {
if (!propsRef.current) {
if (!propsRef.current || !hasDispatchedActionRef.current) {
// Detect changes only if an action has been dispatched.
return;
}

hasDispatchedActionRef.current = false;

const previousState = getControlledState(internalPreviousState, propsRef.current);
const { multiple, optionComparer } = propsRef.current;

Expand All @@ -71,7 +75,7 @@ function useStateChangeDetection<TOption>(
onChange?.(nextSelectedValue);
}
}
}, [nextState.selectedValue, internalPreviousState, propsRef]);
}, [nextState.selectedValue, internalPreviousState, propsRef, hasDispatchedActionRef]);

React.useEffect(() => {
if (!propsRef.current) {
Expand Down Expand Up @@ -101,6 +105,8 @@ export default function useControllableReducer<TOption>(
const propsRef = React.useRef(props);
propsRef.current = props;

const hasDispatchedActionRef = React.useRef(false);

const initialSelectedValue =
(value === undefined ? defaultValue : value) ?? (props.multiple ? [] : null);

Expand All @@ -111,6 +117,8 @@ export default function useControllableReducer<TOption>(

const combinedReducer = React.useCallback(
(state: ListboxState<TOption>, action: ListboxAction<TOption>) => {
hasDispatchedActionRef.current = true;

if (externalReducer) {
return externalReducer(getControlledState(state, propsRef.current), action);
}
Expand All @@ -127,6 +135,11 @@ export default function useControllableReducer<TOption>(
previousState.current = nextState;
}, [previousState, nextState]);

useStateChangeDetection<TOption>(nextState, previousState.current, propsRef);
useStateChangeDetection<TOption>(
nextState,
previousState.current,
propsRef,
hasDispatchedActionRef,
);
return [getControlledState(nextState, propsRef.current), dispatch];
}
Expand Up @@ -77,7 +77,7 @@ function useUtilityClasses(ownerState: MultiSelectUnstyledOwnerState<any>) {
*/
const MultiSelectUnstyled = React.forwardRef(function MultiSelectUnstyled<TValue>(
props: MultiSelectUnstyledProps<TValue>,
ref: React.ForwardedRef<any>,
forwardedRef: React.ForwardedRef<any>,
) {
const {
autoFocus,
Expand Down Expand Up @@ -122,15 +122,11 @@ const MultiSelectUnstyled = React.forwardRef(function MultiSelectUnstyled<TValue
const ListboxRoot = components.Listbox ?? 'ul';
const Popper = components.Popper ?? PopperUnstyled;

const handleButtonRefChange = (element: HTMLElement | null) => {
buttonRef.current = element;
const handleButtonRefChange = React.useCallback((element: HTMLElement | null) => {
setButtonDefined(element != null);
}, []);

if (element != null) {
setButtonDefined(true);
}
};

const handleButtonRef = useForkRef(ref, handleButtonRefChange);
const handleButtonRef = useForkRef(forwardedRef, useForkRef(buttonRef, handleButtonRefChange));

React.useEffect(() => {
if (autoFocus) {
Expand Down
14 changes: 5 additions & 9 deletions packages/mui-base/src/SelectUnstyled/SelectUnstyled.tsx
Expand Up @@ -69,7 +69,7 @@ function useUtilityClasses(ownerState: SelectUnstyledOwnerState<any>) {
*/
const SelectUnstyled = React.forwardRef(function SelectUnstyled<TValue>(
props: SelectUnstyledProps<TValue>,
ref: React.ForwardedRef<any>,
forwardedRef: React.ForwardedRef<any>,
) {
const {
autoFocus,
Expand Down Expand Up @@ -115,15 +115,11 @@ const SelectUnstyled = React.forwardRef(function SelectUnstyled<TValue>(
const ListboxRoot = components.Listbox ?? 'ul';
const Popper = components.Popper ?? PopperUnstyled;

const handleButtonRefChange = (element: HTMLElement | null) => {
buttonRef.current = element;
const handleButtonRefChange = React.useCallback((element: HTMLElement | null) => {
setButtonDefined(element != null);
}, []);

if (element != null) {
setButtonDefined(true);
}
};

const handleButtonRef = useForkRef(ref, handleButtonRefChange);
const handleButtonRef = useForkRef(forwardedRef, useForkRef(buttonRef, handleButtonRefChange));

React.useEffect(() => {
if (autoFocus) {
Expand Down