[EuiComboBox] Fix rowHeight and singleSelection focus issues

[wenchonglee]

Summary

There're 2 issues that I discovered in EuiComboBox.

The first issue is the miscalculation of height when the rowHeight prop is used.

This is because the list container uses the class euiComboBoxOptionsList__rowWrap, which has a max-height of 200px. I added a condition to limit the height of react-window to 200px and updated the snapshot as well.

This also fixes a small visual issue: With the default rowHeight set to 29px, the list height was 203px (7 visible options * 29) which was actually pushing the bottom arrow of the scrollbar down by 3px.

---

The second issue is only relevant when singleSelection is used. The selected option isn't focused or scrolled to if we didn't use the options variable to reference the selected value.

This is because the activeOptionIndex is calculated using indexOf, which determines the equality between objects by reference. I replaced it with findIndex and compare labels instead. I realize this is a costlier function to run, but I saw findIndex being used in another function and thought it was okay.

Here's a codesandbox replicating the 2 problems

This is my first PR, so please let me know if I missed anything.

Checklist

• Check against all themes for compatibility in both light and dark modes
• Checked in mobile
• Checked in Chrome, Safari, Edge, and Firefox (I don't have access to Safari)
• [ ] Props have proper autodocs
• [ ] Added documentation
• [ ] Checked Code Sandbox works for the any docs examples
• Added or updated jest tests
• Checked for breaking changes and labeled appropriately
• [ ] Checked for accessibility including keyboard-only and screenreader modes
• A changelog entry exists and is marked appropriately

[kibanamachine]

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

[cla-checker-service]

💚 CLA has been signed

[elizabetdev]

Hi @wenchonglee!

Thanks for opening this PR. 🎉

Could you please sign the Contributor Agreement? You just need to use the same email address linked to your Github account.

[elizabetdev]

jenkins test this

[wenchonglee]

Hi @wenchonglee!

Thanks for opening this PR. 🎉

Could you please sign the Contributor Agreement? You just need to use the same email address linked to your Github account.

Hey,

I've signed it right after I submitted this PR. Should I redo it, or does it take quite some time?

[kibanamachine]

Preview documentation changes for this PR: https://eui.elastic.co/pr_4072/

[cchaos]

@wenchonglee You're all set with the CLA, thanks!

And thank you for finding this issues and creating a PR! The Codesandbox was really helpful to see exactly the implementation issues you ran into. I'll defer to @chandlerprall's review on this one. You'll just want to be sure to also add a Changelog entry specifying the bugs you fixed.

[wenchonglee]

Thanks @cchaos

I wasn't sure if I was supposed to write the changelog before the implementation has been reviewed.
I've just added to the changelog :)

[chandlerprall]

Height change looks good. The selection code works but does not solve all the cases, such as clicking on the dropdown arrow or when tabbing into the component.

[wenchonglee]

Height change looks good. The selection code works but does not solve all the cases, such as clicking on the dropdown arrow or when tabbing into the component.

Hey @chandlerprall,
Thanks for reviewing the PR, I did not consider those interactions at all and never noticed they weren't working before.

I made some changes to consider these interactions:

• Tab
• Key down
• Key up
• Clicking on the dropdown arrow

Please let me know if I missed anything!

[chandlerprall]

Thanks for tracking down those occurances! Looks like this has removed the ability to tab through the component - likely the call to focusSelectedOption when the options list is open is re-capturing focus.

[wenchonglee]

Hmm, I thought it was intentional that the focus is 'trapped' when an option is selected.

Even for the non singleSelection variants, you cannot tab away if you have an option selected. (e.g. using key down to select options). You can see this behavior with the current version of Eui

I'm not able to test this right now but I suspect it might be this part of the code:

eui/src/components/combo_box/combo_box.tsx

Lines 651 to 657 in 6b993ad

	case keys.TAB:
	// Disallow tabbing when the user is navigating the options.
	if (this.hasActiveOption() && this.state.isListOpen) {
	event.preventDefault();
	event.stopPropagation();
	}
	break;

Should I change this behavior?

[chandlerprall]

@cchaos @myasonik how should we handle the above comment's observation?

In multi select combo boxes, there is no active option until you arrow-down into the options list, at which point tab is prevented. There seems to be a lot of inconsistencies in our current approach (tab into the control vs click on the input vs click on the dropdown), this PR unifies the behaviours (well, it didn't until I asked for that in addition) but that behaviour seems less than ideal. One option could be to remove the unification and keep this to the original bug fix, which kicks that can down the road but certainly simplifies the issue at hand.

[cchaos]

If you are asking me what I think the ultimate and correct solution for all of this is, my answer is that EuiComboBox should use EuiSelectable. I think that's the real answer to getting functional and accessible parity between all these selection lists. So that said, it's probably not worth the effort beyond fixing just the original problem in this PR.

[chandlerprall]

my answer is that EuiComboBox should use EuiSelectable

I talked with Michail over slack and we had the same conclusion.

@wenchonglee please undo the changes I requested to unify the tab/input click/arrow click experiences - 6b993ad - after that I think this will be ready to merge.

That uneven experience exists in the component today and resolving will be a much larger effort than I expected.

[wenchonglee]

👍 I've reverted the last commit

[chandlerprall]

jenkins test this

[kibanamachine]

Preview documentation changes for this PR: https://eui.elastic.co/pr_4072/

[chandlerprall]

Thank you very much for the contribution! My apologies for the back-and-forth change at the end.