-
-
Notifications
You must be signed in to change notification settings - Fork 243
[autocomplete][combobox] Move CompositeList to the ComboboxList to make Input element work with composites #2883
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
base: master
Are you sure you want to change the base?
Conversation
commit: |
✅ Deploy Preview for base-ui ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Bundle size report
|
Works with the PR release preview: https://codesandbox.io/p/sandbox/2883-forked-38q7wf |
f4c8d55
to
cc9bf54
Compare
cc9bf54
to
046f911
Compare
labelsRef.current = []; | ||
} | ||
}; | ||
}, [labelsRef]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes in CompoboxList revealed an issue where refs are not cleaned up on unmount, keeps holding all references to the elements. It caused the test fail in React 18 Strict Mode, because listRef is not empty on the second+ time on open and waitForListPopulated in useListNavigation assumes it is ready to navigate as soon as it opens.
This was not an issue in React 19 Strict Mode, since IndexGuessBehavior.GuessFromOrder behaved more accurately.
This PR just moves the
CompositeList
from the Root element to the List to make Input element work with Composites, such as Toolbar.CodeSandbox: https://codesandbox.io/p/sandbox/quizzical-swanson-7q3yqh
It can move the focus from the input successfully because the keyboard event is handled by the CompositeRoot, however it cannot move back with arrow keys because the ComboboxInput is unavailable there but registered to the context in the ComboboxRoot(Internal).
As a side note, I am not sure if it is intended to enable focus roving on the input elements (or focusable elements in general) that is not rendered as CompositeItem. Somewhat relates to #2702.