Skip to content
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

fix(ComboBox): prevent invalid option retention on Enter #8476

Conversation

emyarod
Copy link
Member

@emyarod emyarod commented Apr 21, 2021

Closes #8170
Related #2823
Related #3862

This PR prevents an invalid option from remaining in the ComboBox on Enter keypress while retaining the functionality introduced in #3862 in response to #2823 (Enter expands the closed ComboBox menu). Alternatively, we may be able to remove the Enter key menu expansion due to this issue being closed as a wontfix #3861), but curious to hear others' thoughts on this

Testing / Reviewing

perform the test described in the referenced issue and confirm that pressing Enter when an invalid value is in the input field does not close the menu while retaining the previous selection (if one was made)

@netlify
Copy link

netlify bot commented Apr 21, 2021

Deploy preview for carbon-elements ready!

Built with commit 9c3329a

https://deploy-preview-8476--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented Apr 21, 2021

Deploy preview for carbon-components-react ready!

Built without sensitive environment variables with commit 9c3329a

https://deploy-preview-8476--carbon-components-react.netlify.app

@jnm2377
Copy link
Contributor

jnm2377 commented Apr 23, 2021

I can see why #3861 was closed, since that one gets a bit more complicated with multiselect/open/close on enter, but I think having an event listener for enter on combo box is fine since you're only selecting one item. That being said, when I tested the component, I realized enter only opens the menu box if there is no item already selected. If I've selected an item and the menu is closed, then I press enter to try to open again, it doesn't open. Is that expected?

Copy link
Contributor

@jnm2377 jnm2377 left a comment

Choose a reason for hiding this comment

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

Looks like it's mostly working. When I was testing, I noticed that if I selected an item, then tried to select an invalid item (xxx) and pressed enter, it didn't close. Great! But then if I deleted the xxx and closed the menu, the original selected item text was no longer displaying even though it shows as still selected when the menu is open. See video attached.

Screen.Recording.2021-04-23.at.7.58.37.AM.mov

Copy link
Contributor

@andreancardona andreancardona left a comment

Choose a reason for hiding this comment

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

also seeing Josefina's comment / bug :/

@emyarod emyarod force-pushed the 8179-prevent-invalid-option-retention branch from 7003a07 to e30dbd2 Compare April 28, 2021 15:17
@emyarod
Copy link
Member Author

emyarod commented Apr 28, 2021

@jnm2377 @andreancardona this does not appear to be a regression, unless I am misunderstanding you. can you clarify what the expected behavior should be and what the existing behavior is WRT #8170? I can address it separately but it doesn't seem to be related to the issue and is not a regression from this PR

@emyarod
Copy link
Member Author

emyarod commented Apr 28, 2021

That being said, when I tested the component, I realized enter only opens the menu box if there is no item already selected. If I've selected an item and the menu is closed, then I press enter to try to open again, it doesn't open. Is that expected?

regarding this question, yes this is expected as Enter typically does not expand a combobox (ref https://www.w3.org/TR/wai-aria-practices/examples/combobox/aria1.1pattern/listbox-combo.html)

@emyarod emyarod force-pushed the 8179-prevent-invalid-option-retention branch from e30dbd2 to a409c86 Compare April 28, 2021 17:02
@jnm2377
Copy link
Contributor

jnm2377 commented Apr 29, 2021

@emyarod you're right. After testing the current component, it seems that the bug already exists. I can open an issue for it.

Screen.Recording.2021-04-29.at.3.09.40.PM.mov

Copy link
Member

@tw15egan tw15egan left a comment

Choose a reason for hiding this comment

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

I think just ensuring enter does not close the menu with an invalid option is enough for this PR. I think if we want to change how the list-box menus are opened/closed via keyboard (Enter vs space vs down arrow) we should track in a separate issue and have a consistent pattern throughout.

This LGTM in reference to the original ticket 👍🏻 ✅

@kodiakhq kodiakhq bot merged commit fe9d56a into carbon-design-system:main Apr 30, 2021
@nertzy
Copy link

nertzy commented May 3, 2021

This also closes #6613.

@emyarod emyarod deleted the 8179-prevent-invalid-option-retention branch May 3, 2021 15:54
@emyarod emyarod mentioned this pull request May 17, 2021
94 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ComboBox retains invalid option after typing and clicking enter
5 participants