Skip to content

Conversation

@atomiks
Copy link
Contributor

@atomiks atomiks commented Sep 9, 2025

Fixes #2703

Previously:

  1. store.state.setInputValue(...)
  2. Return inputValue from useControlled
  3. React.useEffect + store.apply({ inputValue }) executes
  4. inputValue updates too late in Combobox.Input - it must be sync (from the onChange call) for the caret position to be preserved.
function Bug() {
  const [value, setValue] = React.useState('');
  return (
    <input
      value={value}
      onChange={(event) => {
        const value = event.target.value;
        // This is similar to what we're doing; the onChange sets the value from useControlled,
        // which returns a value that is updated asynchronously (with useEffect) to apply to the store,
        // which then informs useStore subscribers. But this happens too late and doesn't work for onChange.
        queueMicrotask(() => {
          setValue(value);
        });
      }}
    />
  );
}

Now:

  1. store.state.setInputValue(...)
  2. Return inputValue from useControlled
  3. No synchronizing to the store with an effect; updates are sync through a derived context value

cc: @romgrk maybe you know of a better way to update the store based on derived values from other hooks, or perhaps a new context provider is required just for this special case.

@atomiks atomiks added type: bug It doesn't behave as expected. scope: autocomplete component: autocomplete Changes related to the autocomplete component. component: combobox Changes related to the combobox component. labels Sep 9, 2025
@pkg-pr-new
Copy link

pkg-pr-new bot commented Sep 9, 2025

Open in StackBlitz

pnpm add https://pkg.pr.new/mui/base-ui/@base-ui-components/react@2707
pnpm add https://pkg.pr.new/mui/base-ui/@base-ui-components/utils@2707

commit: 284041b

Comment on lines 166 to 178
onCompositionStart(event) {
isComposingRef.current = true;
setComposingValue(event.currentTarget.value);
},
onCompositionEnd(event) {
isComposingRef.current = false;
const next = event.currentTarget.value;
setComposingValue(null);
store.state.setInputValue(
next,
createBaseUIEventDetails('input-change', event.nativeEvent),
);
},
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc: @mj12albert to test with composed characters, none of this should be necessary now

Copy link
Member

@mj12albert mj12albert Sep 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still works correctly~

Also noticed in the async search autocomplete demo undo/redo using the keyboard (after typing some stuff) is messed up but works correctly in this PR now

@mui-bot
Copy link

mui-bot commented Sep 9, 2025

Bundle size report

Bundle Parsed size Gzip size
@base-ui-components/react 🔺+4B(0.00%) 🔺+24B(+0.02%)

Details of bundle changes

@netlify
Copy link

netlify bot commented Sep 9, 2025

Deploy Preview for base-ui ready!

Name Link
🔨 Latest commit 284041b
🔍 Latest deploy log https://app.netlify.com/projects/base-ui/deploys/68c280e9b52f7b0008ef91bb
😎 Deploy Preview https://deploy-preview-2707--base-ui.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

Copy link
Contributor

@romgrk romgrk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This approach isn't very clean. I would suggest either:

  • Add a callback setter in useControlled to allow updating the store at the same time as the local state hook
  • Create a useControlled that uses the store instead of a useState hook.

We're mixing reactivity models with the useControlled hook.

@atomiks
Copy link
Contributor Author

atomiks commented Sep 9, 2025

@romgrk I think the problem here is that it's the user's external React.useState() that is passing the input value down to us. The store reacts to updates from their change in useState - useControlled simply returns the controlled: inputValue state

The store can't update at the same time in controlled mode; it has to be derived?

const [value, setValue] = React.useState('');
<Autocomplete.Root value={value} onValueChange={setValue}>

I suppose a workaround would be to set the state in the store manually in setInputValue and just "bypass" the useControlled hook for controlled mode only but that's arguably even messier - plus it would break if they decided to change the value in onValueChange under a certain condition in more exotic use cases

@romgrk
Copy link
Contributor

romgrk commented Sep 9, 2025

Right, that's annoying. Have you tried updating the store in the render phase in reaction to the controlled prop change? React might be fine with it, considering it's an external store. Otherwise I guess the additional context could work. Anyway I'm quite unhappy about having to use useEffect for updates (the store itself is built to be completely synchronous, so the async nature of useEffect is not what we want), it would be nice to find a way to get rid of it.

@atomiks
Copy link
Contributor Author

atomiks commented Sep 9, 2025

Nope, it errors because it tries to update another component as the root is rendering:

Store.ts:45 Cannot update a component (ForwardRef(ComboboxInput)) while rendering a different component

I agree the effect is annoying to synchronize values but I wonder how we can restructure the code to avoid it - it seems difficult

@mj12albert
Copy link
Member

mj12albert commented Sep 10, 2025

I found an issue when pressing Enter to pick the first IME suggestion doesn't work:

Screen.Recording.2025-09-10.at.10.36.59.PM.mov

Same in all 3 browsers and also happens in master, using a number key to pick the first option works so it's not that bad

@mj12albert
Copy link
Member

onInputValueChange is firing incorrectly during composition now:

Screen.Recording.2025-09-11.at.12.46.03.AM.mov

Here's the same input on master:

Screen.Recording.2025-09-11.at.12.50.15.AM.mov

@atomiks atomiks force-pushed the fix/combobox-input-value-delay branch from c7b159b to 947900d Compare September 10, 2025 16:58
@atomiks atomiks force-pushed the fix/combobox-input-value-delay branch from 947900d to 9dace8e Compare September 10, 2025 16:59
@atomiks
Copy link
Contributor Author

atomiks commented Sep 10, 2025

@mj12albert ah right, makes sense - the composition handling also fixed the issue Material UI has without the handling added. I've restored it back.

Copy link
Member

@mj12albert mj12albert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Controlled mode works correctly now, though better add a test for the correct caret position when inserting text in the middle of the input string ~

@atomiks atomiks force-pushed the fix/combobox-input-value-delay branch from d66a9c3 to 284041b Compare September 11, 2025 07:57
@atomiks atomiks merged commit a3f7a2f into mui:master Sep 11, 2025
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component: autocomplete Changes related to the autocomplete component. component: combobox Changes related to the combobox component. type: bug It doesn't behave as expected.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[combobox][autocomplete] Input cursor jumps to end with async suggestions

4 participants