[material-ui][Select] Fix Select when using the spacebar #43966
Conversation
sai6855
added
bug 🐛
sai6855
changed the title
Netlify deploy previewhttps://deploy-preview-43966--material-ui.netlify.app/ Bundle size reportDetails of bundle changes (Toolpad) |
| const child = childrenArray.find( | ||
| (childItem) => childItem.props.value === event.target.dataset.value, | ||
| ); |
There was a problem hiding this comment.
Should be handled like onClick: handleItemClick(child), no?
There was a problem hiding this comment.
Yes, this is much better. updated in this commit 9e01896
| if (child.props.onKeyDown) { | ||
| child.props.onKeyDown(event); | ||
| } |
There was a problem hiding this comment.
Why do we call the custom onKeyDown handler only on space, shouldn't this be outside of the condition?
There was a problem hiding this comment.
Oops sorry, i overlooked this. moved child.props.onKeyDown(event); out of condition and modified test to reflect same here e9f5d5b
Co-authored-by: Marija Najdova <[email protected]> Signed-off-by: sai chand <[email protected]>
There was a problem hiding this comment.
I don't think this is the correct solution to this problem.
The reason the spacebar doesn't select items is because we're blocking it here: https://github.com/mui/material-ui/blob/master/packages/mui-material/src/Select/SelectInput.js#L401-L404. If we remove line 404's preventDefault, then the spacebar selects items as it triggers onClick.
This change might also open a can of worms of behavior changes that users might consider breaking, in a very important component as Select. This behavior has been there from v5 at least. I wonder if it's better to wait for the Base UI refactor, given that we will have those changes coming shortly.
But we can't remove it, because it causes the other issue described in the comment. Adding the onKeyDown, actually allow us to capture an event and add the functionality without regressing the behavior.
I think regardless of how the solution looks like, we can investigate further if there is a better solution, we should fix the behavior. This is a basic accessibility feature that is broken on the component. It being broken for a long time is even more of a reason to fix it faster rather then waiting longer. |
preview: https://deploy-preview-43966--material-ui.netlify.app/material-ui/react-select/
closes #43874