Skip to content

[Select] Implement pointer cancellation #45789

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

Open
wants to merge 23 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
2874b92
fix(Select): resolve focus, drag, pointer issues
Kartik-Murthy Apr 2, 2025
a757802
fix: revert import order
Kartik-Murthy Apr 2, 2025
8f7e282
fix(Select): resolve pointer cancellation issues for WCAG 2.5.2 compl…
Kartik-Murthy Apr 15, 2025
d91d5ba
chore(Select): remove unused startedOn property from dragSelectRef
Kartik-Murthy Apr 16, 2025
011310a
Merge branch 'master' into fix/select-focus-drag-pointer
ZeeshanTamboli Jun 23, 2025
c322fa9
Merge branch 'master' into fix/select-focus-drag-pointer
ZeeshanTamboli Jun 23, 2025
d72df91
try another better logic
ZeeshanTamboli Jun 25, 2025
5cc8671
Merge branch 'master' into fix/select-focus-drag-pointer
ZeeshanTamboli Jun 25, 2025
a3a58a9
Add more logic
ZeeshanTamboli Jun 25, 2025
1d111f1
Add tests
ZeeshanTamboli Jun 25, 2025
de2f809
try out user-event
ZeeshanTamboli Jun 26, 2025
b82657c
fix
ZeeshanTamboli Jun 26, 2025
431d820
Merge branch 'master' into fix/select-focus-drag-pointer
ZeeshanTamboli Jun 26, 2025
869a18c
add beforeEach
ZeeshanTamboli Jun 26, 2025
8e5ec3f
Merge branch 'master' into fix/select-focus-drag-pointer
ZeeshanTamboli Jul 7, 2025
57fe123
select item on mouse up
ZeeshanTamboli Jul 9, 2025
636a26a
Revert "select item on mouse up"
ZeeshanTamboli Jul 9, 2025
ba010c3
merge ref coming from paper props
ZeeshanTamboli Jul 9, 2025
e7b93c4
fix test
ZeeshanTamboli Jul 9, 2025
89b5273
Merge branch 'master' into fix/select-focus-drag-pointer
ZeeshanTamboli Jul 9, 2025
ae8127f
select item on mouse up
ZeeshanTamboli Jul 9, 2025
449e7a9
check if pointer is down to skip mouseup when clicking item
ZeeshanTamboli Jul 10, 2025
d3daad7
Add tests
ZeeshanTamboli Jul 10, 2025
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
183 changes: 183 additions & 0 deletions packages/mui-material/src/Select/Select.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,74 @@ describe('<Select />', () => {
skip: ['componentProp', 'componentsProp', 'themeVariants', 'themeStyleOverrides'],
}));

describe('Pointer Cancellation', () => {
beforeEach(function beforeEachCallback() {
// Run these tests only in browser because JSDOM doesn't have getBoundingClientRect() API
if (/jsdom/.test(window.navigator.userAgent)) {
this.skip();
}
});

it('should close the menu when mouse is outside the select', () => {
render(
<Select value="" MenuProps={{ slotProps: { backdrop: { 'data-testid': 'backdrop' } } }}>
<MenuItem value="">none</MenuItem>
<MenuItem value={10}>Ten</MenuItem>
</Select>,
);
const trigger = screen.getByRole('combobox');

// Open the menu
fireEvent.mouseDown(trigger);
expect(screen.getByRole('listbox')).not.to.equal(null);

// Simulate mouse up outside the menu. The mouseup target is the backdrop when the menu is opened.
fireEvent.mouseUp(screen.getByTestId('backdrop'), { clientX: 60, clientY: 10 });

// Menu should be closed now
expect(screen.queryByRole('listbox', { hidden: false })).to.equal(null);
});

it('should not close the menu when mouse is inside the trigger', () => {
render(
<Select value="">
<MenuItem value="">none</MenuItem>
<MenuItem value={10}>Ten</MenuItem>
</Select>,
);
const trigger = screen.getByRole('combobox');

// Open the menu
fireEvent.mouseDown(trigger);
expect(screen.getByRole('listbox')).not.to.equal(null);

// Simulate mouse up inside the trigger
fireEvent.mouseUp(trigger, { clientX: 20, clientY: 20 });

// Menu should still be open
expect(screen.queryByRole('listbox', { hidden: false })).not.to.equal(null);
});

it('should not close the menu when releasing on menu paper', () => {
render(
<Select value="" MenuProps={{ slotProps: { paper: { 'data-testid': 'paper' } } }}>
<MenuItem value="">none</MenuItem>
<MenuItem value={10}>Ten</MenuItem>
</Select>,
);
const trigger = screen.getByRole('combobox');

// Open the menu
fireEvent.mouseDown(trigger);

// Simulate mouse up on menu paper
fireEvent.mouseUp(screen.getByTestId('paper'));

// Menu should still be open
expect(screen.getByRole('listbox')).not.to.equal(null);
});
});

describe('prop: inputProps', () => {
it('should be able to provide a custom classes property', () => {
render(
Expand Down Expand Up @@ -931,6 +999,23 @@ describe('<Select />', () => {

expect(backdrop.style).to.have.property('backgroundColor', 'red');
});

it('should merge ref coming from paper props', () => {
const paperRef = React.createRef();

render(
<Select
defaultValue="10"
open
MenuProps={{ slotProps: { paper: { 'data-testid': 'paper', ref: paperRef } } }}
>
<MenuItem value="10">Ten</MenuItem>
<MenuItem value="20">Twenty</MenuItem>
</Select>,
);

expect(paperRef.current).to.equal(screen.getByTestId('paper'));
});
});

describe('prop: SelectDisplayProps', () => {
Expand Down Expand Up @@ -1352,6 +1437,64 @@ describe('<Select />', () => {
combinedStyle,
);
});

it('should be able to select the items on click', () => {
render(
<Select defaultValue={[10]} multiple>
<MenuItem value={10}>Ten</MenuItem>
<MenuItem value={20}>Twenty</MenuItem>
<MenuItem value={30}>Thirty</MenuItem>
</Select>,
);

const trigger = screen.getByRole('combobox');

expect(trigger).to.have.text('Ten');

// open the menu
fireEvent.mouseDown(trigger);

const listbox = screen.queryByRole('listbox');
expect(listbox).not.to.equal(null);

const options = screen.getAllByRole('option');
// Click second option
fireEvent.click(options[1]);

expect(trigger).to.have.text('Ten, Twenty');

// Menu is still open in case of multiple
expect(listbox).not.to.equal(null);
});

it('should be able to select the items on mouseup', () => {
render(
<Select defaultValue={[10]} multiple>
<MenuItem value={10}>Ten</MenuItem>
<MenuItem value={20}>Twenty</MenuItem>
<MenuItem value={30}>Thirty</MenuItem>
</Select>,
);

const trigger = screen.getByRole('combobox');

expect(trigger).to.have.text('Ten');

// open the menu
fireEvent.mouseDown(trigger);

const listbox = screen.queryByRole('listbox');
expect(listbox).not.to.equal(null);

const options = screen.getAllByRole('option');
// Mouse up on second option
fireEvent.mouseUp(options[1]);

expect(trigger).to.have.text('Ten, Twenty');

// Menu is still open in case of multiple
expect(listbox).not.to.equal(null);
});
});

describe('prop: autoFocus', () => {
Expand Down Expand Up @@ -1461,6 +1604,22 @@ describe('<Select />', () => {
expect(onClick.callCount).to.equal(1);
});

it('should pass onMouseUp prop to MenuItem', () => {
const onMouseUp = spy();
const { getAllByRole } = render(
<Select open value="30">
<MenuItem onMouseUp={onMouseUp} value={30}>
Thirty
</MenuItem>
</Select>,
);

const options = getAllByRole('option');
fireEvent.mouseUp(options[0]);

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

// https://github.com/testing-library/react-testing-library/issues/322
// https://x.com/devongovett/status/1248306411508916224
it('should handle the browser autofill event and simple testing-library API', () => {
Expand Down Expand Up @@ -1839,4 +1998,28 @@ describe('<Select />', () => {
expect(container.querySelector('.MuiSelect-iconFilled')).not.to.equal(null);
expect(container.querySelector('.MuiSelect-filled ~ .MuiSelect-icon')).not.to.equal(null);
});

it('should select the item on mouse up', () => {
render(
<Select defaultValue={10}>
<MenuItem value={10}>Ten</MenuItem>
<MenuItem value={20}>Twenty</MenuItem>
<MenuItem value={30}>Thirty</MenuItem>
</Select>,
);

const trigger = screen.getByRole('combobox');

// open the menu
fireEvent.mouseDown(trigger);
expect(screen.queryByRole('listbox')).not.to.equal(null);

const options = screen.getAllByRole('option');
fireEvent.mouseUp(options[1]);

expect(trigger).to.have.text('Twenty');

// Menu should be closed now
expect(screen.queryByRole('listbox', { hidden: false })).to.equal(null);
});
});
73 changes: 62 additions & 11 deletions packages/mui-material/src/Select/SelectInput.js
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,11 @@ const SelectInput = React.forwardRef(function SelectInput(props, ref) {
...other
} = props;

const paperProps = {
...MenuProps.PaperProps,
...MenuProps.slotProps?.paper,
};

const [value, setValueState] = useControlled({
controlled: valueProp,
default: defaultValue,
Expand All @@ -146,10 +151,15 @@ const SelectInput = React.forwardRef(function SelectInput(props, ref) {

const inputRef = React.useRef(null);
const displayRef = React.useRef(null);
const paperRef = React.useRef(null);
const didPointerDownRef = React.useRef(false);

const [displayNode, setDisplayNode] = React.useState(null);
const { current: isOpenControlled } = React.useRef(openProp != null);
const [menuMinWidthState, setMenuMinWidthState] = React.useState();

const handleRef = useForkRef(ref, inputRefProp);
const handlePaperRef = useForkRef(paperProps.ref, paperRef);

const handleDisplayRef = React.useCallback((node) => {
displayRef.current = node;
Expand Down Expand Up @@ -233,6 +243,36 @@ const SelectInput = React.forwardRef(function SelectInput(props, ref) {
event.preventDefault();
displayRef.current.focus();

const doc = ownerDocument(event.currentTarget);

function handleMouseUp(mouseEvent) {
if (!displayRef.current) {
return;
}

// mouse is over the options/menuitem, don't close the menu
if (paperRef.current.contains(mouseEvent.target)) {
return;
}

const triggerElement = displayRef.current.getBoundingClientRect();

// mouse is inside the trigger, don't close the menu
if (
mouseEvent.clientX >= triggerElement.left &&
mouseEvent.clientX <= triggerElement.right &&
mouseEvent.clientY >= triggerElement.top &&
mouseEvent.clientY <= triggerElement.bottom
) {
return;
}

// close the menu
update(false, mouseEvent);
}

doc.addEventListener('mouseup', handleMouseUp, { once: true });

update(true, event);
};

Expand All @@ -257,7 +297,7 @@ const SelectInput = React.forwardRef(function SelectInput(props, ref) {
}
};

const handleItemClick = (child) => (event) => {
const handleItemSelect = (child) => (event) => {
let newValue;

// We use the tabindex attribute to signal the available options.
Expand All @@ -277,10 +317,6 @@ const SelectInput = React.forwardRef(function SelectInput(props, ref) {
newValue = child.props.value;
}

if (child.props.onClick) {
child.props.onClick(event);
}

if (value !== newValue) {
setValueState(newValue);

Expand Down Expand Up @@ -394,7 +430,26 @@ const SelectInput = React.forwardRef(function SelectInput(props, ref) {

return React.cloneElement(child, {
'aria-selected': selected ? 'true' : 'false',
onClick: handleItemClick(child),
onPointerDown: () => {
didPointerDownRef.current = true;
},
onClick: (event) => {
didPointerDownRef.current = false;
if (child.props.onClick) {
child.props.onClick(event);
}
handleItemSelect(child)(event);
},
onMouseUp: (event) => {
if (didPointerDownRef.current) {
didPointerDownRef.current = false;
return;
}
if (child.props.onMouseUp) {
child.props.onMouseUp(event);
}
handleItemSelect(child)(event);
},
onKeyUp: (event) => {
if (event.key === ' ') {
// otherwise our MenuItems dispatches a click event
Expand Down Expand Up @@ -482,11 +537,6 @@ const SelectInput = React.forwardRef(function SelectInput(props, ref) {

const classes = useUtilityClasses(ownerState);

const paperProps = {
...MenuProps.PaperProps,
...MenuProps.slotProps?.paper,
};

const listProps = {
...MenuProps.MenuListProps,
...MenuProps.slotProps?.list,
Expand Down Expand Up @@ -572,6 +622,7 @@ const SelectInput = React.forwardRef(function SelectInput(props, ref) {
},
paper: {
...paperProps,
ref: handlePaperRef,
style: {
minWidth: menuMinWidth,
...(paperProps != null ? paperProps.style : null),
Expand Down