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][Select] Fix Select when using the spacebar #43966

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
26 changes: 26 additions & 0 deletions packages/mui-material/src/Select/Select.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,32 @@ describe('<Select />', () => {
expect(options[1]).to.have.attribute('data-value', '20');
});

it('should select an option when the space key is pressed', () => {
const handleChange = spy();
const handleKeyDown = spy();
const { getAllByRole, getByRole } = render(
<Select value="0" onChange={handleChange}>
<MenuItem value="0">Zero</MenuItem>
<MenuItem value="1">One</MenuItem>
<MenuItem value="2" onKeyDown={handleKeyDown}>
Two
</MenuItem>
</Select>,
);

const trigger = getByRole('combobox');
fireEvent.mouseDown(trigger);

const options = getAllByRole('option');
fireEvent.keyDown(options[0], { key: 'ArrowDown' });
fireEvent.keyDown(options[1], { key: 'ArrowDown' });
fireEvent.keyDown(options[2], { key: ' ' });

expect(handleChange.callCount).to.equal(1);
expect(handleKeyDown.callCount).to.equal(1);
expect(handleChange.firstCall.args[0].target.value).to.equal('2');
});

[' ', 'ArrowUp', 'ArrowDown', 'Enter'].forEach((key) => {
it(`should open menu when pressed ${key} key on select`, async () => {
render(
Expand Down
47 changes: 35 additions & 12 deletions packages/mui-material/src/Select/SelectInput.js
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,21 @@ const SelectInput = React.forwardRef(function SelectInput(props, ref) {
}
};

const clonedOnChange = (event, newValue, child) => {
// Redefine target to allow name and value to be read.
// This allows seamless integration with the most popular form libraries.
// https://github.com/mui/material-ui/issues/13485#issuecomment-676048492
// Clone the event to not override `target` of the original event.
const nativeEvent = event.nativeEvent || event;
const clonedEvent = new nativeEvent.constructor(nativeEvent.type, nativeEvent);

Object.defineProperty(clonedEvent, 'target', {
writable: true,
value: { value: newValue, name },
});
onChange(clonedEvent, child);
};

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

Expand Down Expand Up @@ -286,18 +301,7 @@ const SelectInput = React.forwardRef(function SelectInput(props, ref) {
setValueState(newValue);

if (onChange) {
// Redefine target to allow name and value to be read.
// This allows seamless integration with the most popular form libraries.
// https://github.com/mui/material-ui/issues/13485#issuecomment-676048492
// Clone the event to not override `target` of the original event.
const nativeEvent = event.nativeEvent || event;
const clonedEvent = new nativeEvent.constructor(nativeEvent.type, nativeEvent);

Object.defineProperty(clonedEvent, 'target', {
writable: true,
value: { value: newValue, name },
});
onChange(clonedEvent, child);
clonedOnChange(event, newValue, child);
}
}

Expand All @@ -324,6 +328,24 @@ const SelectInput = React.forwardRef(function SelectInput(props, ref) {
}
};

const handleItemKeyDown = (child) => (event) => {
if (event.key === ' ') {
if (child.props.onKeyDown) {
child.props.onKeyDown(event);
}
Copy link
Member

Choose a reason for hiding this comment

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

Why do we call the custom onKeyDown handler only on space, shouldn't this be outside of the condition?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops sorry, i overlooked this. moved child.props.onKeyDown(event); out of condition and modified test to reflect same here e9f5d5b


const newValue = child.props.value;
if (newValue !== value) {
setValueState(newValue);

if (onChange) {
clonedOnChange(event, newValue, child);
}
}
update(false, event);
}
};

const open = displayNode !== null && openState;

const handleBlur = (event) => {
Expand Down Expand Up @@ -408,6 +430,7 @@ const SelectInput = React.forwardRef(function SelectInput(props, ref) {
child.props.onKeyUp(event);
}
},
onKeyDown: handleItemKeyDown(child),
role: 'option',
selected,
value: undefined, // The value is most likely not a valid HTML attribute.
Expand Down