-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Fix combobox issue handling tab/enter custom value is allowed #2670
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
Conversation
* Share logic so that tab/enter on combobx share same behaviour as when isFocused is false which is working as expected
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.
LGTM, thanks for adding the tests! Left a comment for the team to note a change in behavior which I feel is more correct
it('retains selected key on enter', function () { | ||
let {getByRole} = renderComboBox({allowsCustomValue: true, selectedKey: '2'}); | ||
|
||
let combobox = getByRole('combobox'); | ||
act(() => { | ||
userEvent.click(combobox); | ||
jest.runAllTimers(); | ||
}); | ||
|
||
act(() => { | ||
fireEvent.keyDown(combobox, {key: 'Enter', code: 13, charCode: 13}); | ||
fireEvent.keyUp(combobox, {key: 'Enter', code: 13, charCode: 13}); | ||
jest.runAllTimers(); | ||
}); | ||
|
||
expect(onSelectionChange).not.toHaveBeenCalled(); | ||
}); |
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.
This is a change from prior behavior, but I think this new behavior is more correct.
Previous behavior:
- User selects an option from the dropdown.
- User hits enter on the field
- onSelectionChange is fired with null to remove the selected key since we figured the user was committing a custom value
New behavior
- User selects an option from the dropdown.
- User hits enter on the field
- Previous selection is not removed since the text matches the selected key text
I figure the scenario of a user selecting a option and then immediately hitting enter to submit it as a custom value doesn't really happen.
GET_BUILD |
What would it take to get this merged? Happy to help. |
close shouldnt always commit the focused value (e.g when clicking to blur) so changing it back to commitValue. We should revist the commit and close logic to clean it up and clarify when to commit and when not to.
@garand I've pushed up a fix to the logic that should unblock this PR. |
GET_BUILD |
Build successful! 🎉 |
GET_BUILD |
Build successful! 🎉 |
## API Changes
unknown top level export { type: 'identifier', name: 'Column' } |
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.
LGTM
isFocused is false which is working as expected
Closes #2413, #4728
✅ Pull Request Checklist:
📝 Test Instructions:
🧢 Your Project:
Adobe