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.

Sign up for GitHub

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

Jump to bottom

[EuiSelectable] Fix logic to detect the blur event coming from child popover #5021

Merged
merged 6 commits into from
Aug 16, 2021
Merged

[EuiSelectable] Fix logic to detect the blur event coming from child popover #5021

merged 6 commits into from
Aug 16, 2021

Conversation

afharo
Copy link
Member

@afharo afharo commented Aug 12, 2021 •
edited
Loading

Summary

Resolves #5020 (and probably #4147).

The current logic to detect if EuiSelectable was losing focus due to its popover was not working. Probably because the HTMLElement didn't actually contain the popover.

This PR changes the logic to check the popover's List ID to match the assigned rootId in the EuiSelectable.

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

@afharo afharo force-pushed the selectable/fix-onContainerBlur-activeOptionIndex-conflict branch from ddb3d9f to eaab94c Compare August 12, 2021 13:42
@kibanamachine
Copy link

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

@afharo afharo marked this pull request as ready for review August 12, 2021 14:21
@afharo afharo requested a review from myasonik August 12, 2021 14:22
@myasonik
Copy link
Contributor

🤔 It's hard to do any keyboard or screen reader usage as, apparently, this has become totally broken at some point and using arrow keys and enter to use this component doesn't work at all.

The code change looks fine (the bug is in master so it's definitely not related) but it makes me uncertain on how to really test this thoroughly...

@cchaos
Copy link
Contributor

cchaos commented Aug 12, 2021

Weird, I just checked out published docs, and I have no trouble using the keyboard to navigate.

Screen.Recording.2021-08-12.at.11.50.52.AM.mp4

@myasonik
Copy link
Contributor

Ugh, ignore me. This is an old problem of mine that I keep forgetting about - has bit me several times.

For whatever reason, my external keyboard (and no one else's) does this. As soon as I use any other keyboard it's all fine.

Code looks good; Tested in FF and Edge. Can be approved after checklist is completed.

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@cchaos cchaos requested a review from chandlerprall August 12, 2021 18:02
cchaos
cchaos approved these changes Aug 12, 2021
View reviewed changes
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.

Functionally tests right to me. @chandlerprall can you double-check the code?

CHANGELOG.md Outdated Show resolved Hide resolved
afharo and others added 2 commits August 12, 2021 20:29
@afharo
Add tests
db8eafd
@afharo @cchaos
Update CHANGELOG.md
56def19 
Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
@kibanamachine
Copy link

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

@afharo afharo added the bug label Aug 13, 2021
@thompsongl
Copy link
Contributor

it looks like the issue has been there since 7.12.0. So, if we could backport it to 7.14.1 with a low risk upgrade, it’d be great
― @afharo

Just noting that that we should look at backporting this once it merges

chandlerprall
chandlerprall approved these changes Aug 13, 2021
View reviewed changes
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; Tested via the test deployment

@kibanamachine
Copy link

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

@afharo afharo merged commit 47c7a21 into elastic:master Aug 16, 2021
@afharo afharo deleted the selectable/fix-onContainerBlur-activeOptionIndex-conflict branch August 16, 2021 09:20
This was referenced Aug 16, 2021
Closed
Merged
Closed
This was referenced Aug 24, 2021
Merged
Merged
@cee-chen cee-chen mentioned this pull request Mar 3, 2023
Merged
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@myasonik myasonik myasonik approved these changes

@cchaos cchaos cchaos approved these changes

@chandlerprall chandlerprall chandlerprall approved these changes

Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

EuiSelectableTemplateSitewide requires 2 clicks to trigger onChange events
6 participants
@afharo @kibanamachine @myasonik @cchaos @thompsongl @chandlerprall