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

[EuiSelectable] Prevent onFocus from negating selection #5117

Merged
merged 3 commits into from Sep 1, 2021

Conversation

thompsongl
Copy link
Contributor

@thompsongl thompsongl commented Aug 31, 2021

Summary

Fixes #4147, in which a click event on an unfocused EuiSelectable would prevent the intended option click and instead scroll to the currently selected option (i.e., two clicks were required because the first was captured by onFocus).

The fix is to look for click events on the element that watches for onFocus and prevent the handler from running. The subsequent events triggered by the option selection will correctly place focus inside the list and allow for keyboard navigation. Keyboard navigation is otherwise unaffected.

Repro:

  1. Go the Single Selection example.
  2. Check the last item.
  3. Click away from the list (outside the component)
  4. Click the first item in the list.
  5. The first item should be selected.
  6. Repeat with the Popover example.

Checklist

- [ ] Check against all themes for compatibility in both light and dark modes
- [ ] Checked in mobile

  • Checked in Chrome, Safari, Edge, and Firefox

- [ ] Props have proper autodocs and playground toggles
- [ ] 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
  • Revert 2fbc966

@kibanamachine
Copy link

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

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

Change LGTM, pulled & tried my hardest to break this locally.

We've dealt with differentiating the tab-vs-click focus response a few times, perhaps we should look at a more formal way to detect & handle them.

@thompsongl
Copy link
Contributor Author

perhaps we should look at a more formal way to detect & handle them

That's a good point. I'm going to merge this as-is, but keep this in mind as I continue my spike on EuiSelectable things.

@thompsongl thompsongl merged commit 005f9e3 into elastic:master Sep 1, 2021
@cchaos
Copy link
Contributor

cchaos commented Sep 1, 2021

😬 I think you forgot to revert the Revert me

@thompsongl
Copy link
Contributor Author

thompsongl commented Sep 1, 2021

Sure did 🤦

#5123

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.

[EuiSelectable] Sometimes needs double click when used inside EuiPopover
4 participants