Generated by 🚫 dangerJS against 5018f21

I added some tests (SarthakC#1) how I think this new behavior should work.

I added some tests (SarthakC#1) how I think this new behavior should work.

Thank you. I will work on this and revert.

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 0280b6b:

I added some tests (SarthakC#1) how I think this new behavior should work.

It worked. I added a few more tests based on yours.

@SarthakC Hi. Do you think that you will be able to handle the regressions?

@SarthakC Hi. Do you think that you will be able to handle the regressions?

@oliviertassinari Hello! Sorry for AWOL. I have been down with the flu. I shall be able to get back to this as soon as I get better.

@SarthakC No problem, I hope you feel better soon :)

@SarthakC I have pushed a two-line fixe and rebased with an update of the test. I hope it will do it and that you are doing better.

@SarthakC I have pushed a two-line fixe and rebased with an update of the test. I hope it will do it and that you are doing better.

Thank you @oliviertassinari
I am recovering now. My period of convalescence was longer than I expected. Need to change my city 😅

@SarthakC Thank you

Wanted to mention we had similar breakage happen due to the Select onClick change from this PR, inside the renderValue component:

<Select
  renderValue={() => (
    <TextField
      // this no longer fires
      onClick={e => doSomething(e)}
    />
  )}
/>

the solution is to now use onMouseDown instead of onClick here, but would this be recommended for any element you use as the renderValue?
seems like something worth making a little more noise about.

@herrethan What's your use case for rendering a TextField here, with on click?

@eps1lon What do you think of reverting these changes? We have seen a couple of negative effects from this change:

• people using onClick in their unit tests
• people rendering custom elements inside the select

Does it worth it? And if it does, released under a patch, is this right? 🤔

@oliviertassinari thanks!

We have a unique input field that is a combo between text input and select dropdown, where the select options are predefined values meant to be convenience selections that populate the text input.

On the TextField onClick we e.stopPropagation() to allow the user to enter text in the text input. There are surely other ways of achieving this but I was wondering more generally about anyone trying to capture onClick from inside the renderValue.

Right, the counter-arguments to revert are that 1. the patch was meant to improve user experience 2. the test regression impact unit tests (not e2e), tests that could be less dependent on the event but only consider the user actions. 3. People rendering custom interactive elements in the select, likely want to use https://material-ui.com/components/autocomplete/ instead.

people using onClick in their unit tests

This would be bad precedent. It was a bug. People relying on this bug (especially in a test) should not block the fix.

people rendering custom elements inside the select

We have to stop at some point. Customization doesn't mean you should do everything that's possible.

Especially the example from @herrethan is a classic combobox. It was never intended to use the Select in that way.

This is why it is important in open source to give back. Have a use case that isn't explicitly documented? Contribute a test case or documentation or open an issue if the use case is not trivial and let maintainers handle the integration. Not sure if it's trivial or not? Open anyway.

I also want to emphasize that we have to be more confident in our judgement. We can't consider a revert because of a single issue report. Instead figure out how to best move forward.

We had the following reports: #18658, #18655.

[...] #18655.

I explicitly addressed this one. It was relying on a bug. This should never block a fix.

The special use case should be considered though.

OK thanks. I think that we should leave it more time, to see how things settle. My main concern now (releasing it under a patch, that's too later) is with how people write unit tests, we are making it a bit harder for them but that's DX based and not UX based.

we are making it a bit harder for them

No we are fixing a bug. You are now making an argument to never release fixes because people might rely on them in their software.

I use the Select component inside a DataGrid cell. This change broke the logic to enter editing mode in the cell. Attempting to fix it breaks other parts of the DataGrid logic. I will have to spend a few hours getting everything working again. Given that the difference is perceived performance (between click and mouse down) is not even noticeable (at least to me), I wonder if this change was worth it.