Skip to content

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

Merged
afharo merged 6 commits intoelastic:masterfrom
afharo:selectable/fix-onContainerBlur-activeOptionIndex-conflict
Aug 16, 2021
Merged

[EuiSelectable] Fix logic to detect the blur event coming from child popover#5021
afharo merged 6 commits intoelastic:masterfrom
afharo:selectable/fix-onContainerBlur-activeOptionIndex-conflict

Conversation

@afharo
Copy link
Copy Markdown
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
Copy Markdown

kibanamachine commented Aug 12, 2021

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
Copy Markdown
Contributor

myasonik commented Aug 12, 2021

🤔 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
Copy Markdown
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
Copy Markdown
Contributor

myasonik commented Aug 12, 2021

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
Copy Markdown

kibanamachine commented Aug 12, 2021

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

@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Aug 12, 2021

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
Copy Markdown
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?

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
Copy Markdown

kibanamachine commented Aug 12, 2021

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

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

thompsongl commented Aug 13, 2021

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
Copy Markdown
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
Copy Markdown

kibanamachine commented Aug 15, 2021

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

3 more reviewers

@chandlerprall chandlerprall chandlerprall approved these changes

@cchaos cchaos cchaos approved these changes

@myasonik myasonik myasonik approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

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