[Autocomplete] Add ability to override key down events handlers

[hessaam]

Closes #19887

[mui-pr-bot]

Details of bundle changes

Generated by 🚫 dangerJS against 88be7f9

[oliviertassinari]

@hessaam Do you have a reproduction?

[hessaam]

When the freeSolo prop was true, if a user clears an input, "option.length" causes the Uncaught TypeError error.

[oliviertassinari]

@hessaam Could you provide a codesandbox or add a regression test case?

[eps1lon]

Why can't we use preventDefault() and .defaultPrevented?

[eps1lon]

Also: Please revert the changes to existing tests unless this is actually a breaking change.

[oliviertassinari]

Why can't we use preventDefault() and .defaultPrevented?

@eps1lon For this reason #14991. I thought it would be assuming too much, so I went with Downshift's solution: downshift-js/downshift#358.

Also: Please revert the changes to existing tests unless this is actually a breaking change.

The change in the tests is related to testing the form submission behavior. This is a refactoring that allows writing more resilient tests. Without it, the way the event handlers are written in the Autocomplete can interfere. For instance, it might call prevent default, but if there is a return before calling the onKeydown handler, you won't get the correct assertion.

[eps1lon]

Also: Please revert the changes to existing tests unless this is actually a breaking change.

The change in the tests is related to testing the form submission behavior. This is a refactoring that allows writing more resilient tests. Without it, the way the event handlers are written in the Autocomplete can interfere. For instance, it might call prevent default, but if there is a return before calling the onKeydown handler, you won't get the correct assertion.

Sure, please extract it to a separate PR.

[eps1lon]

For this reason #14991. I thought it would be assuming too much

I don't understand what the reason is. I see a PR targetting modals onClose. But the issue is not talking about Escape key behavior as far as I can tell. What's the concrete issue with preventDefault? Note that it doesn't make sense to just look at defaultPrevented. You should check key as well.

[oliviertassinari]

Sure, please extract it to a separate PR.

Ok, happy to do it. But note that this pull request would need to be based on the extraction. As the new logic breaks the tests.

I don't understand what the reason is.

First, I would like to raise that this isn't about a specific key, the purpose is to allow developers to override any key, it could be Escape.
From my perspective, the motivation is that the default behavior of the browser and the default behavior of the Autocomplete are two independent notions. For instance, a developer might want to override the Enter behavior of the Autocomplete will still have the form submitted. It seems to be the use case of #19887. @ivan-jelev could you share more about the problem you had? :)

[eps1lon]

We can't solve abstract problems, only concrete ones. So in absent of a concrete problem, let's go with the API that already exists on the web: preventDefault().

As the new logic breaks the tests.

So it is a breaking change. We should label it as such.

[oliviertassinari]

We can't solve abstract problems, only concrete ones

#19887 and downshift-js/downshift#330 are concrete problems. They seem to want to submit the form no matter what, so preventDefault doesn't seem to be an option. However, happy to try it out, and learn from developers' feedback.

So it is a breaking change. We should label it as such.

The tests were indirectly relying on it, it wasn't testing the behavior on purpose. It could have been a bug. But no objection to labeling it a breaking change if you think strongly about it.

[eps1lon]

#19887

That we are solving here. Not sure why you consider it a separate problem

downshift-js/downshift#330

That is still solvable with DOM API. No need to learn new mui-only expando properties on well defined interfaces.

[oliviertassinari]

So it is a breaking change. We should label it as such.

@eps1lon Ok label added.

Sure, please extract it to a separate PR.

Done in #23704.

That we are solving here. Not sure why you consider it a separate problem

From how I understand #19887, @ivan-jelev wants to validate the form as soon as Enter is pressed, no matter the state of it: case 'Enter': submitChangesAndFocusOut();. He's interested in bypassing this line:

https://github.com/mui-org/material-ui/blob/dfc94e03d391384155cbdd1a0e3b0fe16e2e1042/packages/material-ui/src/useAutocomplete/useAutocomplete.js#L727-L728

So I think this makes a solution based on event.preventDefault() is not possible. This could be supported by two other aspects:

• In Enter event on form wrapped input should not have e.preventDefault() downshift-js/downshift#330, the problem is almost the same, the use case is to prevent downshift to call event.preventDefault() internally. They went with this solution:
  event.preventDownshiftDefault || event.defaultPrevented.
• In event.stopPropagation() on a text input inside of a dialog should prevent the dialog from closing #13470 (comment), @stevemao made the point that event.preventDefault() should be only used when the objective is to disable a native behavior, not a third-party module. I don't know how much that holds when reading the spec, but I think that it makes sense.

If you think strongly about this, happy to encourage preventDefault() in the documentation, for a first iteration. However, I doubt it will be enough, I would expect somebody to come with this requirement. I think that a submit on the first Enter can have its use cases.

[eps1lon]

He's interested in bypassing this line:

Sure, but we're exhibiting some cognitive dissonance here. On one hand we strongly declare "We don't want to validate the form. " but are ok with people wanting to validate.

So either we don't want to validate it or we do. What we're now doing is punting to problem instead of clearly drawing requirements and coming up with an implementation for that.

Right now we insist on a particular implementation and try to come up with requirements for it. That doesn't lead to a robust implementation.

So I urge people again to not say "how" they want problems to be solved but start with the problems they have. We have more context to come up with a better implementation that solves this particular problem.

What this means is that you shouldn't mention anything about events, bubbling, default behavior etc but rather how the user story looks. From that we can come up with solutions because that's our job. We're not here to paste the code you wish into the codebase but solve problems your users are facing by providing a component library.

The only actual user story in the underlying issues seems to be

Need to be able to select options in dropdown with Space key and on Enter key drop down should close and blur out

and I have to ask: Why?

This is behavior that doesn't match the default web behavior for <input list="" /> nor the behavior specified in the aria-practices. Which means that any user visiting your site is now confronted with an UI element that looks similar but behaves different. So we have to decide whether we want you to be able to do that. After all, we're a component library. If you can do anything you want with the components, maybe even create a worse UX, then why do we even exist? And maybe you've done the user research and came to the conclusion that this is the superior UX. But I think it's only fair that you share that research so that we can understand what we're actually implementing.

[oliviertassinari]

Sure, but we're exhibiting some cognitive dissonance here. On one hand, we strongly declare "We don't want to validate the form. " but are ok with people wanting to validate.

@eps1lon What if we reword this comment? I think that the idea is that the default should be to request Enter x2 as safest in most cases. However, when you have a single entry Combobox in your form, I think that it makes sense:

diff --git a/packages/material-ui/src/useAutocomplete/useAutocomplete.js b/packages/material-ui/src/useAutocomplete/useAutocomplete.js
index 9ac3d25e9d..cbf6e0cf34 100644
--- a/packages/material-ui/src/useAutocomplete/useAutocomplete.js
+++ b/packages/material-ui/src/useAutocomplete/useAutocomplete.js
@@ -724,7 +724,7 @@ export default function useAutocomplete(props) {
             const option = filteredOptions[highlightedIndexRef.current];
             const disabled = getOptionDisabled ? getOptionDisabled(option) : false;

-            // We don't want to validate the form.
+            // Avoid early form validation, let the end-users continue filling the form.
             event.preventDefault();

             if (disabled) {

So I urge people again to not say "how" they want problems to be solved but start

Agree, the iconic: "I want a faster horse", nop, you want a car.

So we have to decide whether we want you to be able to do that.

From my perspective the answer is straightforward: we definitely do, here and everywhere: "provide sane defaults, allow developers to do advanced things". I think that the feeling of freedom should be as if you wrote the component from scratch. I think it's why the hook/headless approach is appreciated by front-end developers. If a developer can't get this feeling, he will avoid using the library: https://twitter.com/markdalgleish/status/1328683488250195968, https://twitter.com/chofter/status/1332091815823020037.

If you can do anything you want with the components, maybe even create a worse UX, then why do we even exist?

What about sometimes, amazing things happen, being able to do worse also mean developers can do better?

rebased and improved test case

[eps1lon]

@oliviertassinari Could you clarify what user story this PR is addressing? It's unclear from the linked issue.

[oliviertassinari]

The developer story is taking full control of the handlers. A possible user story is a short form where the validation of an option (Enter) in the combo box directly triggers the form.