mui / material-ui

Material UI: Comprehensive React component library that implements Google's Material Design. Free forever.
https://mui.com/material-ui/
MIT License
93.67k stars 32.22k forks source link

Autocomplete with freeSolo and custom onChange breaks on Enter key pressed #18123

Closed oziniak closed 4 years ago

oziniak commented 4 years ago

Current Behavior 😯

I use Autocomplete with redux, to simulate async search. search is provided from the redux store, when action fired with onSearchCustom gets to the reducer and changes the state. Everything works, I see results populated into Combobox.

        <Autocomplete
            freeSolo
            filterOptions={(option) => option}
            options={search}
            renderInput={(params) => {
                const {
                    inputProps: { onChange: ownOnChange }
                } = params as any;

                return (
                    <TextField
                        {...params}
                        inputProps={{
                            ...params.inputProps,
                            onChange(event: any) {
                                onSearchCustom({ limit: 5, search: event.currentTarget.value });
                                return ownOnChange(event);
                            }
                        }}
                    />
                );
            }}
        />

But when I press Enter, everything breaks with this error. image

And I can't use event.keyCode, event.charCode, event.key in my onChange handler to input, because everything is undefined there.

Expected Behavior 🤔

It would be great if I could use a component to fit my needs. I was hoping that if I put any plug on Enter key in my onChange handler to input, but I can't even check if that would work, because I can't tell if Enter is pressed or any other key.

Steps to Reproduce 🕹

https://codesandbox.io/s/create-react-app-with-typescript-1te6e Steps:

1.Write any text, it works ok

  1. Press enter, it breaks with error in console

Context 🔦

Your Environment 🌎

Tech Version
Material-UI v4.3.2
Material-UI lab ^4.0.0-alpha.30
React ^16.8.6
Browser chrome 78.0.3904.70
TypeScript 3.5.2
etc.
oliviertassinari commented 4 years ago

@oziniak Thank you for the report! What do you think of this fix?

diff --git a/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js b/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js
index 7f28ddca2..d7394c18b 100644
--- a/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js
+++ b/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js
@@ -166,6 +166,8 @@ export default function useAutocomplete(props) {
     let newInputValue;
     if (multiple) {
       newInputValue = '';
+    } else if (freeSolo && typeof newValue === 'string') {
+      newInputValue = newValue;
     } else {
       newInputValue = newValue != null ? getOptionLabel(newValue) : '';
     }

Do you want to submit a pull request? :)

ad-das commented 4 years ago

Is PR #18117 related to this? If yes, has it been shipped yet?

oliviertassinari commented 4 years ago

@ad-das #18117 solves a different problem (with the multi-select). You can check the added test case to get a better idea of the problem it solves.

This bug is different, it's about using free solo with rich (object) options, it's a combination I didn't encounter during the initial development of the component.

oziniak commented 4 years ago

@oliviertassinari I'll be able to submit a PR in an hour or so

oliviertassinari commented 4 years ago

@oziniak Awesome! If you struggle with the addition of a test case, I can have a look at it.

oziniak commented 4 years ago

@oliviertassinari not sure why, but I can't make a test to fail.

  describe.only('free solo', () => {
    it('should not crash with custom onChange for input', () => {
      const noop = () => {};
      const { container } = render(
        <Autocomplete freeSolo renderInput={(params) => (
          <TextField
            {...params}
            label="defaultProps"
            inputProps={{
              ...params.inputProps,
              onChange(event) {
                noop(); // simulate custom onChange logic
                return params.inputProps.onChange(event);
              }
            }}
          />
        )} />);
      const input = container.querySelector('input');
      fireEvent.keyPress(input, {key: "Enter", code: 13, charCode: 13});
    });
  });
eps1lon commented 4 years ago

@oziniak Try focusing the input first and then fireEvent.keyDown(document.activeElement)

oziniak commented 4 years ago

@eps1lon @oliviertassinari is it possible that there's some caching in built/compiled files? because I'm facing the near-to-magic issue. I've removed the fix from useAutocomplete.js (and double check that changes are not in browser files). Currently, when I do git status, I get:

    modified:   docs/src/pages/components/autocomplete/FreeSolo.js
    modified:   docs/src/pages/components/autocomplete/FreeSolo.tsx

But the issue is FIXED somehow, and it drives me crazy 😄 Maybe that's why I can't manage to break the tests.

oliviertassinari commented 4 years ago

@oziniak I'm not aware of any caching logic that might impact here. Can you try this reproduction?

  describe.only('free solo', () => {
    it('should accept any value', () => {
      const handleChange = spy();
      const { container } = render(
        <Autocomplete
          freeSolo
          options={[{ name: 'test' }, { name: 'foo' }]}
          onChange={handleChange}
          renderInput={params => <TextField {...params} />}
        />,
      );
      const input = container.querySelector('input');
      fireEvent.change(input, { target: { value: 'a' } });
      fireEvent.keyDown(input, { key: 'Enter' });
      expect(handleChange.callCount).to.equal(1);
      expect(handleChange.args[0][1]).to.equal('a');
    });
  });
ronaldporch commented 4 years ago

I've also ran into a similar issue, but I assume it's rooted from the same problem. You can even reproduce it on this example. Select another choice, then press Enter. The error is different from what was reported on this though.

TypeError: Cannot read property 'title' of undefined

According to the sample of code, the first two AutoCompletes have no custom onChange or freeSolo.

Edit: I see that there are other closed issues related to this, so assume that it is well known.

https://github.com/mui-org/material-ui/issues/18110

oziniak commented 4 years ago

@ronaldporch yeah, in my example I used this hack to intercept onChange for text input.

                        inputProps={{
                            ...params.inputProps,
                            onChange(event: any) {
                                // onSearchCustom(....);
                                return params.inputProps.onChange(event);
                            }
                        }}

@oliviertassinari I've ran into issue where I can no longer reproduce a bug on local dev environment. I'm trying to test it on docs/src/pages/components/autocomplete/FreeSolo.tsx. I'm running yarn start in one tab and yarn docs:typescript --watch in another and testing on a page http://localhost:3000/components/autocomplete#free-solo. And I can't reproduce the issue from the OP post. This is some kind of magic. I have no changes in packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js. I've even tried to clone the second time. Still can't reproduce. Magic.

oziniak commented 4 years ago

Ok, so I investigated: the issue with breaking of autocomplete with freeSolo on enter pressed, appears only when you have options as an array of objects, for example [{name: "a", id: 0}, {name: "b", id: 1}], and using getOptionLabel={option => option.name}

oziniak commented 4 years ago

OK, nevermind my above comments. I wasn't reading the thread carefully and only now I understood that the issue from the start was in having options as not an array of strings.

ad-das commented 4 years ago

it looks like somehow the demo page of freeSolo component doesn't break now on pressing enter. It was definitely breaking if you enter text -> selected an option, -> press enter at least a couple days ago. Did it somehow get fixed? 🤔

oliviertassinari commented 4 years ago

@ad-das This issue is not visible in the documentation. What you report is different.

vash500 commented 4 years ago

I am running into the exact same issue.

If I remove either of freeSolo or onChange custom values, the problem is not visible (but neither is the expected functionality). This seems related to the this pull request https://github.com/mui-org/material-ui/pull/18161, but I am using @material-ui/core 4.11.0 (which should already have this patch) and I can still reproduce this consistently.

oliviertassinari commented 4 years ago

@vash500 Do you have a reproduction?

vash500 commented 4 years ago

Sorry for the delay, I had not worked on this project for a whole week. So, it turns out that I am full of stupid and what I had is not a reproduction of this case.

When tried to construct a minimal working example I found that it was my onInputChange function that had a typo:

    const onInputChange = function(event, value) {
        if (typeof value === "unefined" || value === null) {
            return;
        }
        setMovieTitle(value.title)
    }

It says unefied instead of undefined. This led to setMovieTitle to be triggered with undefined, which ultimately was being queried for length. I caught it only thanks to the linter https://eslint.org/docs/rules/valid-typeof Entirely my bad.