[EuiComboBox] Fix rowHeight and singleSelection focus issues by wenchonglee · Pull Request #4072 · elastic/eui

wenchonglee · 2020-09-24T05:16:43Z

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 · 2020-09-24T05:16:45Z

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 · 2020-09-24T05:16:47Z

💚 CLA has been signed

elizabetdev · 2020-09-24T11:17:03Z

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 · 2020-09-24T11:17:14Z

jenkins test this

wenchonglee · 2020-09-24T11:35:06Z

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 · 2020-09-24T11:40:28Z

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

cchaos · 2020-09-28T22:23:58Z

@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 · 2020-09-29T14:03:32Z

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.

…he input

wenchonglee · 2020-09-29T16:48:59Z

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 · 2020-09-30T01:21:57Z

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 · 2020-09-30T16:56:51Z

@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 · 2020-09-30T17:35:42Z

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 · 2020-09-30T17:40:38Z

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.

…ing on the input" This reverts commit 6b993ad.

wenchonglee · 2020-10-01T01:49:04Z

👍 I've reverted the last commit

chandlerprall · 2020-10-01T02:28:02Z

jenkins test this

kibanamachine · 2020-10-01T02:49:47Z

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.

…#4072) * Fixed EuiComboBox rowHeight causing height to go over max-height * Fixed EuiComboBox not focusing on the selected option * Added changelog entry * Added conditions to focus on selected option other than clicking on the input * Revert "Added conditions to focus on selected option other than clicking on the input" This reverts commit 6b993ad.

cchaos requested a review from chandlerprall September 28, 2020 22:24

wenchonglee added 3 commits September 29, 2020 21:22

Fixed EuiComboBox rowHeight causing height to go over max-height

6bf2c62

Fixed EuiComboBox not focusing on the selected option

bd2b970

Added changelog entry

8c9e09c

wenchonglee force-pushed the fix-euicombobox branch from 3dc3012 to 8c9e09c Compare September 29, 2020 13:54

chandlerprall suggested changes Sep 29, 2020

View reviewed changes

Added conditions to focus on selected option other than clicking on t…

6b993ad

…he input

chandlerprall suggested changes Sep 29, 2020

View reviewed changes

chandlerprall mentioned this pull request Sep 30, 2020

[EuiComboBox] Allow options to have a unique key #4048

Merged

5 tasks

Revert "Added conditions to focus on selected option other than click…

1f43cf2

…ing on the input" This reverts commit 6b993ad.

chandlerprall approved these changes Oct 1, 2020

View reviewed changes

chandlerprall merged commit 22ce712 into elastic:master Oct 1, 2020

wenchonglee mentioned this pull request Oct 10, 2020

[EuiSelectableTemplateSitewide] Clicking on any virtualized options causes scroll to top #4128

Closed

wenchonglee mentioned this pull request Jul 14, 2021

[EuiComboBox] Regression in singleSelection scrolling behavior #4951

Closed

Conversation

wenchonglee commented Sep 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

kibanamachine commented Sep 24, 2020

Uh oh!

cla-checker-service bot commented Sep 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elizabetdev commented Sep 24, 2020

Uh oh!

elizabetdev commented Sep 24, 2020

Uh oh!

wenchonglee commented Sep 24, 2020

Uh oh!

kibanamachine commented Sep 24, 2020

Uh oh!

cchaos commented Sep 28, 2020

Uh oh!

wenchonglee commented Sep 29, 2020

Uh oh!

chandlerprall left a comment

Choose a reason for hiding this comment

Uh oh!

wenchonglee commented Sep 29, 2020

Uh oh!

chandlerprall left a comment

Choose a reason for hiding this comment

Uh oh!

wenchonglee commented Sep 30, 2020

Uh oh!

chandlerprall commented Sep 30, 2020

Uh oh!

cchaos commented Sep 30, 2020

Uh oh!

chandlerprall commented Sep 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wenchonglee commented Oct 1, 2020

Uh oh!

chandlerprall commented Oct 1, 2020

Uh oh!

kibanamachine commented Oct 1, 2020

Uh oh!

chandlerprall left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

wenchonglee commented Sep 24, 2020 •

edited

Loading

cla-checker-service bot commented Sep 24, 2020 •

edited

Loading

chandlerprall commented Sep 30, 2020 •

edited

Loading