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

Allow keyboard navigation of single select combobox #1619

Conversation

chandlerprall
Copy link
Contributor

@chandlerprall chandlerprall commented Feb 27, 2019

Summary

Fixes #1615. Moves the highlighting-of-active-single-selection logic to the list open action instead of on any prop/state change. This allows controlling the active item via keyboard arrows.

EuiComboBox should get another pass in the future to get a better way of tracking & managing the current active item, there's a lot of interwoven logic in here.

Checklist

- [ ] This was checked in mobile
- [ ] This was checked in IE11
- [ ] This was checked in dark mode
- [ ] Any props added have proper autodocs
- [ ] Documentation examples were added

  • A changelog entry exists and is marked appropriately
  • This was checked for breaking changes and labeled appropriately
    - [ ] Jest tests were updated or added to match the most common scenarios
  • This was checked against keyboard-only and screenreader scenarios
    - [ ] This required updates to Framer X components

@thompsongl
Copy link
Contributor

This happens when trying to use the arrow keys to navigate the list after:

  • make a selection with the keyboard
  • refocus, then search
  • attempt to use the down arrow key

kapture 2019-02-27 at 13 09 17

Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

See comment outside review (I should have done it in the review)

@chandlerprall
Copy link
Contributor Author

@thompsongl great catch! fixed

Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

Yep that works.
Local build and manual keyboard testing

@cchaos
Copy link
Contributor

cchaos commented Feb 27, 2019

The other half of that issue is that after having selected an item, you can keep typing and even leave it in a state where you've typed in something and it persists after leaving the combobox.

@cchaos
Copy link
Contributor

cchaos commented Feb 27, 2019

Semantic UI handles this idea pretty nicely with their searchable dropdown. https://semantic-ui.com/modules/dropdown.html#search-selection

…t; hide selected item if user has search text entered and is a singleSelection box
@chandlerprall
Copy link
Contributor Author

Updated with:

  • hide selected item if user is entering text into a singleSelection box
  • show as invalid if focus leaves the box but there is still search text entered

@cchaos
Copy link
Contributor

cchaos commented Feb 27, 2019

So one thing is weird is that, after having a valid selection, then typing a non-valid suggestion, then backspacing to delete the value, it reverts to the old selection. I would think it would just remain blank.

Also, I can seem to tab out of the combobox input when a selection has been made?

@cchaos
Copy link
Contributor

cchaos commented Feb 27, 2019

Also, the esc key now does this weird thing where it removes the dropdown but you can still type in the input.

@thompsongl
Copy link
Contributor

Also, I can seem to tab out of the combobox input when a selection has been made?

That's intentional. I remember seeing that behavior in code comments and fixed a semi-related tabFocus bug a week or two ago

@cchaos
Copy link
Contributor

cchaos commented Feb 27, 2019

Even though tabbing out of the multi-selection version works?

@thompsongl
Copy link
Contributor

thompsongl commented Feb 27, 2019

As long as the dropdown is open, tabbing out of the box is prevented. (Not saying this can't be changed if we determine a better way)

Update: I was wrong
There has to be an active/focused option item for tab prevention to occur

@chandlerprall
Copy link
Contributor Author

Tabbing through a singleSelection box now works, and options list is displayed again when typing after pressing esc

Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

  1. Active/chosen option/item is no longer highlighted on initial open when keyboard is the trigger (mouse click seems ok)
    kapture 2019-02-27 at 17 24 11

  2. Oddly, active/chosen option/item loses highlight after making another selection. Also, the dropdown no longer closes after making said selection. Outside clicks also don't close the dropdown list.
    kapture 2019-02-27 at 17 25 21

@cchaos
Copy link
Contributor

cchaos commented Feb 28, 2019

Sorry this PR is sort of exploding in scope a bit, but I think these improvements will be invaluable. So thank you!

I'm going to push a couple quick CSS changes your way.

@chandlerprall
Copy link
Contributor Author

Oddly, active/chosen option/item loses highlight after making another selection. Also, the dropdown no longer closes after making said selection. Outside clicks also don't close the dropdown list.

Great catch, thanks! Pushed a fix for this.

Active/chosen option/item is no longer highlighted on initial open when keyboard is the trigger (mouse click seems ok)

This is a trade-off with the tabbing bug fix. The logic is to prevent tabs if any item is selected, but if we pre-select an item when the box auto-opens, then tabbing through the element is prevented. FWIW this is how it was until recently when the pre-select logic was added, which broke the tabbing.

@cchaos
Copy link
Contributor

cchaos commented Feb 28, 2019

Active/chosen option/item is no longer highlighted on initial open when keyboard is the trigger (mouse click seems ok)

Mouse clicking on the input works, but not on the arrow.

@cchaos
Copy link
Contributor

cchaos commented Feb 28, 2019

Tabbing works well now. I think the only thing left is the highlighting if possible... but it's a small potato compared to the rest of the improvements.

@chandlerprall
Copy link
Contributor Author

Highlighting on tab isn't possible without other changes, which will lead into other changes. From the PR description:

EuiComboBox should get another pass in the future to get a better way of tracking & managing the current active item, there's a lot of interwoven logic in here.

We can definitely prioritize making those changes, but I'd prefer doing so in a separate pass on the combo box.

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

TY!

CHANGELOG.md Outdated Show resolved Hide resolved
Co-Authored-By: chandlerprall <[email protected]>
@chandlerprall chandlerprall merged commit 7f6ddf7 into elastic:master Feb 28, 2019
@chandlerprall chandlerprall deleted the 1615-combobox-singleselection-plaintext branch February 28, 2019 19:29
@chandlerprall chandlerprall restored the 1615-combobox-singleselection-plaintext branch February 28, 2019 19:29
@chandlerprall chandlerprall deleted the 1615-combobox-singleselection-plaintext branch February 28, 2019 19:29
@chandlerprall chandlerprall restored the 1615-combobox-singleselection-plaintext branch February 28, 2019 19:29
Shigawire pushed a commit to Shigawire/eui that referenced this pull request May 10, 2019
* Allow keyboard navigation of single select combobox

* changelog

* Guard against invalid activeOptionIndex

* Mark combobox as invalid if it loses focus and search value is present; hide selected item if user has search text entered and is a singleSelection box

* Open combobox options list on search value change

* Allow tabbing off of singleSelection combo boxes when tabbing through form elements

* Fix padding

* Fix combo box not closing when selecting an option

* Update CHANGELOG.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants