[Select] onClick not fired on menu items

[tpeek]

• The issue is present in the latest release.
• I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

If you select the same value twice in a single Select, onChange is not called the second time.

Expected Behavior 🤔

onChange should be called every time a user selects a value, even if it's the same value as before.

Steps to Reproduce 🕹

Here's an example of functionality that worked in v4.9.8: https://codesandbox.io/s/frosty-cohen-gpk3y?file=/src/Demo.js

Here's the same code not working in v4.9.9: https://codesandbox.io/s/hungry-neumann-y0ht0?file=/src/Demo.js

Steps:

1. Click on the Select
2. Select the value that is already selected

Context 🔦

We prefer to have the single Select behave in the same way as the multi Select- click on an already selected item to deselect it. Without an event, we can't do that.

Related issue: #20351
Related PR: #20361

Your Environment 🌎

[marcosvega91]

I think is a duplicate of #20698

[tpeek]

Thanks @marcosvega91, looks like this issue was brought up before. Following the comments, changing onChange to onClick does seem to fix the issue.

However, I can't seem to find the onClick prop documented in the API docs for either TextField or Select

It might also be helpful to update the description of onChange in the Select API docs to make this new behavior more clear.

[tpeek]

Hm, the onClick solution actually doesn't work as well as I first thought- selecting the menu item that is already selected results in onClick being called with an event where event.target.value is 0. 🤔

[marcosvega91]

Hi @tpeek you can try here codesandbox
I put a console.log inside your function and it seams to work

[tpeek]

@marcosvega91 Your linked codesandbox is using v4.9.8. If you update it to v4.9.9, it does the weird "return 0" behavior mentioned above.

[marcosvega91]

I understand the problem. Is linked to #20361

[marcosvega91]

@tpeek I have fixed the issue here #20712

[oliviertassinari]

Duplicate of #20698

[oliviertassinari]

@tpeek I don't think that we need to change anything about it. This replicates the behavior of a native select. If you need to respond to the click events, you can attach a listener to the menu items.

[tpeek]

Adding an onClick prop to a MenuItem doesn't seem to work at all: https://codesandbox.io/s/eloquent-solomon-zjr2y?file=/src/Demo.js

Replacing the onChange prop with onClick in the Select component works for clicks but does not work for keyboard controls: https://codesandbox.io/s/relaxed-zhukovsky-jhykx?file=/src/Demo.js

[oliviertassinari]

@tpeek Thanks for raising, it looks like a bug. Material-UI is supposed to forward all the events. What do you think of this patch? Do you want to submit a pull request? :)

diff --git a/packages/material-ui/src/Select/SelectInput.js b/packages/material-ui/src/Select/SelectInput.js
index c6c6e3d1a..4d17eb618 100644
--- a/packages/material-ui/src/Select/SelectInput.js
+++ b/packages/material-ui/src/Select/SelectInput.js
@@ -140,6 +140,10 @@ const SelectInput = React.forwardRef(function SelectInput(props, ref) {
       newValue = child.props.value;
     }

+    if (child.props.onClick) {
+      child.props.onClick(event);
+    }
+
     if (value === newValue) {
       return;
     }
@@ -252,9 +256,9 @@ const SelectInput = React.forwardRef(function SelectInput(props, ref) {
           // the select to close immediately since we open on space keydown
           event.preventDefault();
         }
-        const { onKeyUp } = child.props;
-        if (typeof onKeyUp === 'function') {
-          onKeyUp(event);
+
+        if (child.props.onKeyUp) {
+          child.props.onKeyUp(event);
         }
       },
       role: 'option',

[tpeek]

I think we might have to do a bit more with the onKeyUp part- I've tested passing onKeyUp to each MenuItem, and it works for the arrow keys but not for the 'Enter' key. Example: https://codesandbox.io/s/divine-violet-jucyq?file=/src/Demo.js

My guess is that the Menu closes onKeyDown, so the MenuItem isn't around for the onKeyUp event.

[oliviertassinari]

Enter will trigger onClick, we are good.