-
-
Notifications
You must be signed in to change notification settings - Fork 32.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Autocomplete immediately changes initial inputValue state #19423
Comments
I have a feeling this goes hand-in-hand with this issue: #19318. |
Thanks for opening this issue and providing a codesandbox explaining the issue. Unfortunately I cannot observe the issue. In codesandbox.io/embed/material-demo-2quum?fontsize=14&hidenavigation=1&theme=dark nothing is logged to the console. Did you accidentally change the sandbox? I would recommend freezing the codesandbox after you opened the issue. Could you include what browser and operating system you're using? |
I apologize for the confusion - I was having issues with code sandbox saving my changes as you suggested. The posted link should be up to date now. I am on Linux/Chrome, I also tested on Linux/Firefox. |
@willwill96 I have seen at least 5 developers fall into this trap so far, e.g. #19318. There are two isolated states 1. |
@oliviertassinari Am I misunderstanding something? It seems like you are saying only use |
👋 Thanks for using Material-UI! We use GitHub issues exclusively as a bug and feature requests tracker, however, For support, please check out https://material-ui.com/getting-started/support/. Thanks! If you have a question on StackOverflow, you are welcome to link to it here, it might help others. |
I can't think of a way we could better teach this (leveraging the documentation and the component warnings). I assume StackOverflow is the best answer we can have (somebody explaining it there). Thanks for raising. |
Well, actually, we could always try a more detailed controlled docs section. |
It would be nice if one of the demos used I understand now why the ticket I linked (#19318) is not considered a bug, but the issue I posted here is not the same, and I believe I am using the The code sandbox posted in this description, uses only |
@willwill96 The value takes precedence over the inputValue. |
@oliviertassinari Why does an undefined I am not passing in the I could be mistaken, but I feel that you are addressing the ticket that I linked, and not the issue that I presented in this ticket. |
@willwill96 The value is not undefined but null by default, hence an empty input value. diff --git a/docs/pages/api/autocomplete.md b/docs/pages/api/autocomplete.md
index f6531069f..e158319ee 100644
--- a/docs/pages/api/autocomplete.md
+++ b/docs/pages/api/autocomplete.md
@@ -35,7 +35,7 @@ You can learn more about the difference by [reading this guide](/guides/minimizi
| <span class="prop-name">closeIcon</span> | <span class="prop-type">node</span> | <span class="prop-default"><CloseIcon fontSize="small" /></span> | The i
con to display in place of the default close icon. |
| <span class="prop-name">closeText</span> | <span class="prop-type">string</span> | <span class="prop-default">'Close'</span> | Override the default text for
the *close popup* icon button.<br>For localization purposes, you can use the provided [translations](/guides/localization/). |
| <span class="prop-name">debug</span> | <span class="prop-type">bool</span> | <span class="prop-default">false</span> | If `true`, the popup will ignore the
blur event if the input if filled. You can inspect the popup markup with your browser tools. Consider this option when you need to customize the component. |
-| <span class="prop-name">defaultValue</span> | <span class="prop-type">any<br>| array</span> | | The default input value. Use when the component i
s not controlled. |
+| <span class="prop-name">defaultValue</span> | <span class="prop-type">any<br>| array</span> | <span class="prop-default">props.multiple ? [] : nul
l</span> | The default input value. Use when the component is not controlled. |
| <span class="prop-name">disableClearable</span> | <span class="prop-type">bool</span> | <span class="prop-default">false</span> | If `true`, the input can't
be cleared. |
| <span class="prop-name">disableCloseOnSelect</span> | <span class="prop-type">bool</span> | <span class="prop-default">false</span> | If `true`, the popup w
on't close when a value is selected. |
| <span class="prop-name">disabled</span> | <span class="prop-type">bool</span> | <span class="prop-default">false</span> | If `true`, the input will be disab
led. |
diff --git a/packages/material-ui-lab/src/Autocomplete/Autocomplete.js b/packages/material-ui-lab/src/Autocomplete/Autocomplete.js
index 5c0ebf1ba..342c8edf0 100644
--- a/packages/material-ui-lab/src/Autocomplete/Autocomplete.js
+++ b/packages/material-ui-lab/src/Autocomplete/Autocomplete.js
@@ -247,7 +247,7 @@ const Autocomplete = React.forwardRef(function Autocomplete(props, ref) {
closeIcon = <CloseIcon fontSize="small" />,
closeText = 'Close',
debug = false,
- defaultValue,
+ defaultValue = props.multiple ? [] : null,
disableClearable = false,
disableCloseOnSelect = false,
disabled = false,
diff --git a/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js b/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js
index 6e5cfcb14..00e7ca020 100644
--- a/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js
+++ b/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js
@@ -85,7 +85,7 @@ export default function useAutocomplete(props) {
blurOnSelect = false,
clearOnEscape = false,
debug = false,
- defaultValue,
+ defaultValue = props.multiple ? [] : null,
disableClearable = false,
disableCloseOnSelect = false,
disableListWrap = false,
@@ -193,7 +193,7 @@ export default function useAutocomplete(props) {
const [value, setValue] = useControlled({
controlled: valueProp,
- default: defaultValue || (multiple ? [] : null),
+ default: defaultValue,
name: componentName,
}); |
@oliviertassinari I have put the PR up for that change, and I do think this helps clarify why the issue is happening and I appreciate the time you've put into this :) I still think the way that these set of props interact together is slightly convoluted though. This is the way I have had to write my Autocomplete to get the desired effect:
In this example, it's strange to me that I need to give the autocomplete a defaultValue prop despite it being a controlled component. Especially given that the doc for defaultValue says:
In an ideal world I would be using |
@willwill96 For context, why do you control the component for? (Your code snippet looks great). |
We use Autocomplete as a free text input box that also offers suggestions. We control the component because we have a few buttons at the top of our component that need to reset the values to certain templated values. If the snippet above looks good to you, then I think I'm fine closing this issue. I do think it is a little strange for |
Ok, thanks for the feedback. I think that we could wait a bit, to get more feedback from users before closing the issue. I would suspect that we will have more developers confused. I suspect that a new dedicated section for the controllable use cases would be beneficial and the best resolution. |
This comment has been minimized.
This comment has been minimized.
Regarding a potential solution, I would propose a diff close to this: diff --git a/docs/src/pages/components/autocomplete/autocomplete.md b/docs/src/pages/components/autocomplete/autocomplete.md
index 6ec7d561c..c64ae8d0b 100644
--- a/docs/src/pages/components/autocomplete/autocomplete.md
+++ b/docs/src/pages/components/autocomplete/autocomplete.md
@@ -30,6 +30,15 @@ Choose one country between 248.
{{"demo": "pages/components/autocomplete/CountrySelect.js"}}
+### Controllable states
+
+The component has two states that can be independently controlled:
+
+1. the "value" state with the `value`/`onChange` props combination.
+2. the "input value" state with the `inputValue`/`onInputChange` props combination.
+
+> ⚠️ Do not try to swap the usage of these props.
+
## Free solo
Set `freeSolo` to true so the textbox can contain any arbitrary value. The prop is designed to cover the primary use case of a search box with suggestions, e.g. Google search.
diff --git a/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js b/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js
index 9957ed76d..68bf22375 100644
--- a/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js
+++ b/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js
@@ -198,9 +198,12 @@ export default function useAutocomplete(props) {
name: componentName,
});
- const { current: isInputValueControlled } = React.useRef(inputValueProp != null);
- const [inputValueState, setInputValue] = React.useState('');
- const inputValue = isInputValueControlled ? inputValueProp : inputValueState;
+ const [inputValue, setInputValue] = useControlled({
+ controlled: inputValueProp,
+ default: '',
+ name: componentName,
+ value: 'inputValue',
+ });
const [focused, setFocused] = React.useState(false);
diff --git a/packages/material-ui/src/utils/useControlled.js b/packages/material-ui/src/utils/useControlled.js
index 76c455e8c..21851b6cd 100644
--- a/packages/material-ui/src/utils/useControlled.js
+++ b/packages/material-ui/src/utils/useControlled.js
@@ -1,7 +1,7 @@
/* eslint-disable react-hooks/rules-of-hooks, react-hooks/exhaustive-deps */
import React from 'react';
-export default function useControlled({ controlled, default: defaultProp, name }) {
+export default function useControlled({ controlled, default: defaultProp, name, state = 'value' }) {
const { current: isControlled } = React.useRef(controlled !== undefined);
const [valueState, setValue] = React.useState(defaultProp);
const value = isControlled ? controlled : valueState;
@@ -11,7 +11,7 @@ export default function useControlled({ controlled, default: defaultProp, name }
if (isControlled !== (controlled !== undefined)) {
console.error(
[
- `Material-UI: A component is changing ${
+ `Material-UI: A component is changing a ${state} of ${
isControlled ? 'a ' : 'an un'
}controlled ${name} to be ${isControlled ? 'un' : ''}controlled.`,
+ 'The nature of the state is determined during the first render, it's considered controlled if the value is not `undefined`.',
'Elements should not switch from uncontrolled to controlled (or vice versa).',
@@ -29,7 +29,7 @@ export default function useControlled({ controlled, default: defaultProp, name }
if (defaultValue !== defaultProp) {
console.error(
[
- `Material-UI: A component is changing the default value of an uncontrolled ${name} after being initialized. ` +
+ `Material-UI: A component is changing the default ${state} of an uncontrolled ${name} after being initialized. ` +
`To suppress this warning opt to use a controlled ${name}.`,
].join('\n'),
); |
I think I'm running into the exact same problem in my application. I created a sandbox to illustrate it (before I realized this issue already existed): https://codesandbox.io/s/loving-ritchie-ulvin
|
@goffioul That's my problem, too. As a workaround I added |
As mentioned before about defaultValue, I'm facing the same problem using the |
This comment has been minimized.
This comment has been minimized.
@oliviertassinari The PR #20403 does not really solve the problem. I've updated my codesandbox, please have a look: https://codesandbox.io/s/thirsty-hodgkin-n1ype. When you use When you use How do you recommend to handle that scenario? That is:
|
@goffioul Interesting, you have uncovered a bug, thanks for raising :). In your first example, the input should be restored on blur. What do you think of this patch? Do you want to submit a pull request? :) diff --git a/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js b/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js
index 3cb1ca0c5..891d351f1 100644
--- a/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js
+++ b/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js
@@ -758,7 +758,7 @@ export default function useAutocomplete(props) {
selectNewValue(event, filteredOptions[highlightedIndexRef.current], 'blur');
} else if (autoSelect && freeSolo && inputValue !== '') {
selectNewValue(event, inputValue, 'blur', 'freeSolo');
- } else if (!freeSolo) {
+ } else {
resetInputValue(event, value);
}
@goffioul When should the user confirm his choice? |
Well, in my scenario, the user can enter free text. The dropdown is just a list of suggestions for quick entry, but the user should be able to enter any arbitrary text. If I'm following correctly, resetting the value on blur is not gonna fit my use case. |
@goffioul |
@oliviertassinari Indeed that seems to be close to what I want. I've updated the sandbox to use
Note that this doesn't include the change you have suggested, not sure there's an easy way to do that in sandbox. Given the triviality of the change, do you really need me to do a PR for it? I don't mind not getting the credit for it :) |
@goffioul This change wouldn't have made it without you, so I think you deserve some credit, one commit sounds reasonable :). Also, it will happen, so you might as well make sure it doesn't harm your use case. |
|
|
For my own information, do you plan to do anything about point 4 above? (same control, same content, but different behavior) |
@goffioul The selected value is different, the context is different the behavior is different, 4. sound great? |
We can just agree to disagree. I understand what you're saying with the fact that the selected value is different. But from a normal user point of view (without any technical knowledge of react/material-ui internals), the only thing he/she sees is that the value is the same in the field, and on one hand the popup icon gives you nothing, on the other hand it gives you the dropdown menu. And if you try to explain to that user that the entered value hasn't been really selected yet and that he/she needs to confirm it by blurring the field first, he/she will just look at you with that what-the-hell-is-he-talking-about face. |
@goffioul In this case, the best alternative is not to use |
i am getting this error : |
@oliviertassinari It's a shame this issue was closed because it's not fixed. onInputChange is called immediately on render with an empty string as the value. This means the initial inputValue gets cleared. Look at how Twitch search works: https://www.twitch.tv/search?term=war There is no way to do this with the Autocomplete due to this bug. |
@kaldune Set |
@oliviertassinari Appreciate your response and everything you've done for us by driving this project. What ended up working is leveraging the For others who find this, the very first call to onInputChange sends "reset" as the reason. Our implementation is very customized and we only ever need to update state when the reason is "input". const [inputText, setInputText] = React.useState(getUrlParam('term') || '');
<Autocomplete
inputValue={inputText}
onInputChange={(event, value, reason) => {
if (reason === 'input') {
setInputText(value);
}
} |
@kaldune I would really encourage using the |
@kaldune thank you a ton 😍 This solved my issue as well! I'm using the Autocomplete as a Google/Bing style search box: The suggestions are populated by calls to a backend search service, with both the current searchText as well as the suggestions being kept in my Redux store, so it was also fairly customized from the get go but I couldn't for the life of me figure out why I kept getting the
error though, when my component was controlled and I never indicate that it wasn't controlled. But I think you're right, and that the 'reset' onInputChange event was must have been setting my searchText to null or undefined which would a) cause my text to not stick around and b) cause Material UI to think I wanted an uncontrolled Autocomplete and would then throw errors when the text changed? ... That's as best as I can tell ... I ended up getting it to work with roughly this: const SearchBox = props => {
const clear = () => {
dispatch(setSearchText(""))
}
const onSearchTextHandler = (event, value, reason) => {
if (reason === 'input') {
dispatch(setSearchText(event.target.value))
dispatch(getSuggestions())
}
if (reason === 'clear') {
clear();
}
}
return (
<Autocomplete
disableClearable
getOptionLabel={(option) => option ? option.ProjectName ? option.ProjectName : "" : ""}
classes={autoCompleteClasses}
options={suggestions}
loading={suggestionsLoading}
onChange={onSuggestionSelect}
freeSolo
inputValue={searchText || ""}
onInputChange={onSearchTextHandler}
noOptionsText={"Start typing to see suggestions..."}
renderOption={(option) => {
return (
<Grid container className={classes.suggestion}>
<Grid item xs={1}>
{option.TextPlain === option.ProjectName ? <Business color="disabled" /> : <AccountBox color="disabled" />}
</Grid>
<Grid item xs={10}>
<Grid container className={classes.suggestionText}>
<Grid item xs={12}>
<span className={classes.suggestionHeading} dangerouslySetInnerHTML={{ __html: option.TextMarkup }}></span>
</Grid>
<Grid item xs={11}>
{option.SecondaryText}
</Grid>
</Grid>
</Grid>
</Grid>
)
}}
renderInput={params => (
<TextField
{...params}
fullWidth
InputLabelProps={{
className: classes.inputLabel
}}
InputProps={{
...params.InputProps,
endAdornment: (
<>
<IconButton size="small" onClick={(e) => { executeSearch(); e.target.blur(); }} >
<SearchIcon className={classes.searchIcon} />
</IconButton>
<IconButton size="small" onClick={(e) => { onClear(); e.target.blur(); }} >
<CloseIcon className={classes.searchIcon} />
</IconButton>
{/* {params.InputProps.endAdornment} */}
</>
)
}}
variant="outlined"
onKeyDown={onKeyDownHandler}
label="Find Projects"
/>
)}
>
</Autocomplete>
)
} |
When using Autocomplete as a controlled component and driving that state with onInputChange, the initial state is overwritten immediately to a blank string:
''
.Current Behavior 😯
Set up
inputValue
andonInputChange
to make Autocomplete a controlled component with an initial value not equal to''
.On startup, notice onInputChange is immediately called with a blank string and thus resets the initial value.
Expected Behavior 🤔
The initial value of
inputValue
is retained.Steps to Reproduce 🕹
https://codesandbox.io/s/material-demo-2quum?fontsize=14&hidenavigation=1&theme=dark
Steps:
onInputChange
(console logged) is called on startup, resetting the initial value to''
Context 🔦
The presence of this bug prevents me from using Autocomplete as a controlled component as it doesn't preserve my initial value
Your Environment 🌎
The text was updated successfully, but these errors were encountered: