[EuiDataGrid] Fix cell popover close on cell click unintentionally affecting complex cell popovers#5797
Conversation
- in a more targeted manner that doesn't affect popovers with complex focus children (e.g. modals, nested popovers)
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5797/ |
|
jenkins test this |
|
Testing this a backported version of this now against 8.2 Kibana to confirm that it fixes elastic/kibana#128626 |
| // Close the cell popover if the popover was open and the user clicked the cell | ||
| if (this.props.popoverContext.popoverIsOpen) { | ||
| this.props.popoverContext.closeCellPopover(); | ||
| } |
There was a problem hiding this comment.
Based on the comment, should this be in onClick instead of onFocus? Or is onFocus an analog for a click event in the context of a data grid cell?
There was a problem hiding this comment.
Each data grid cell is clickable/focusable so onFocus will also fire on clicks. I don't think it's super necessary to specify a separate onClick event - in theory the only realistic way that a user could focus the cell while the popover is open is through either clicking or tapping the cell (keyboard users should not be able to exit the popover in theory, but it is possible if the focus trap has been broken).
I'm also bogarting the existing onFocus event because it has a nice event.target check that I also want for this behavior (i.e. only firing on the cell parent and not on cell children focus).
tl;dr there's no real downside to using onFocus, it's broader and will catch more possible side effects/users, even if the main use case is mouse clicks
There was a problem hiding this comment.
in theory the only realistic way that a user could focus the cell while the popover is open is through either clicking or tapping the cell
That's kind of what I was thinking too. Sounds good
|
I can also confirm this is now working on Kibana 8.2:
|
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5797/ |
- because I put the new logic inside the setTimeout, Cypress was opening the popover too quickly and the new logic was instantly closing it *derp* - the new logic probably doesn't need to be in a timeout, so moving it out
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5797/ |
|
@thompsongl @chandlerprall would love to get this merged in and backported today if y'all have some time to review! 🙏 Last BC is next Wednesday so we are on a small timeline |
thompsongl
left a comment
thompsongl
left a comment
There was a problem hiding this comment.
LGTM
I'm good with the backport strategy unless @chandlerprall would rather have the security team adjust their architecture.
|
Based on their latest response in elastic/kibana#128626 I don't think that's realistic, but we can always re-revert in main later on for future majors. |
|
Good to go with the proposed backport(s)! |
4 participants
Summary
This PR essentially reverts the approach used in #5681 as it is causing the Security team unintentional headaches/bugs, detailed in elastic/kibana#128626 (comment).
This PR will need to be backported to v53.0.2 and then further backported to Kibana's 8.2 branch before the next BC.
Checklist
- [ ] Checked in both light and dark modes- [ ] Checked in mobileonFocusshould work for tap events as well- [ ] Props have proper autodocs and playground toggles- [ ] Added documentation- [ ] Checked Code Sandbox works for any docs examples- [ ] Checked for breaking changes and labeled appropriately- [ ] Updated the Figma library counterpart- [ ] Checked for accessibility including keyboard-only and screenreader modesApplies to mouse clicks only, not possible to focus originating cell without popover already having been closed otherwise