[Lens] Prevent KQL Popovers From Stacking

[drewdaemon]

Summary

Fixes #105518 .

Does this by using the parent FilterList component to manage open/close popover state. This allows us to make sure that only one can be open at a time.

Screen.Recording.2021-11-10.at.3.07.31.PM.mov

[elasticmachine]

Pinging @elastic/kibana-vis-editors @elastic/kibana-vis-editors-external (Team:VisEditors)

[flash1293]

@elasticmachine merge upstream

[flash1293]

Other popovers work fine now, but clicking the same button again doesn't close the popover anymore - seems like this worked accidentally before. Can we get this behavior back?

[drewdaemon]

@flash1293 good catch. I've updated the PR to restore that behavior.

[flash1293]

Sorry, missed this during first review - to make it easy to use this via keyboard only, the focus should jump back to the "Add filter" button if the user presses enter in the KQL field in the popover:

However, on this PR it's jumping to the close button instead:

I'm not 100% sure why this happens - the logic to put the focus back to the last button is handled automatically by the EuiPopover - it stores a reference to the last focused element on opening, then on closing it tries to focus it again.

Maybe it's because the newly introduced effect takes an additional render cycle to get the "open" state to the filter popover? https://github.com/elastic/kibana/pull/118258/files#diff-2d0f89c22addaee8aa9d430ad3ae303df53874d506b65f1c820371c727b17cb1R182

On your PR the user presses the "Add filter" button, it adds the filter to the state, renders, sets the "activeFilterId" via the new effect hook, renders, then sets the actual popover open state in

https://github.com/elastic/kibana/pull/118258/files#diff-e127eadc5836b29048ab36c9a7f2cb28fe940484d66736e81cba2b5a092713b6R38

[mbondyra]

Good job with this PR, these things are really tricky! Apart from Joe's comment, there's two more issues:

1. Could you also implement this behavior for interval ranges? It should be very similar as it uses similar components. The issue didn't mention that 🙈

2. This is not introduced by this PR, but when a user has a filter popover open and presses escape, it closes the popover and the flyout. Could you check if simple stopPropagation would stop the flyout from closing if popover is open?

[drewdaemon]

@elasticmachine merge upstream

[drewdaemon]

For the record, @flash1293 and I found that the focus bug is present on main as well as this branch, so these changes don't seem to be the culprit.

@mbondyra —

Could you also implement this behavior for interval ranges?

Sure! Would you recommend I do that as part of this PR, or shall I create a separate issue?

but when a user has a filter popover open and presses escape, it closes the popover and the flyout. Could you check if simple stopPropagation would stop the flyout from closing if popover is open?

I see what you mean. I'll have a look.

[drewdaemon]

@mbondyra it looks like the escape key behavior should be handled by EUI (see this PR). However, for some reason it isn't working, so I provided a stop-gap solution that you can review.

This makes me wonder if we've found two EUI bugs.

Edit: it looks like they're working on a major version bump right now #117242. Maybe this will resolve something...

[kibana-ci]

💚 Build Succeeded

• Buildkite Build
• Commit: f2f3473
• Storybooks Preview

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
lens	957.9KB	958.0KB	+188.0B

History

• 💔 Build #7091 failed ac8edf9
• 💔 Build #7078 failed 7ee6244
• 💚 Build #6712 succeeded 4b614b8
• 💚 Build #6604 succeeded 144f1e1
• 💚 Build #5989 succeeded 6807518

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

[mbondyra]

Sure! Would you recommend I do that as part of this PR, or shall I create a separate issue?

As you prefer! I'm gonna approve this now and ask me for rereview in case if you choose to continue in this PR :)

The 'focus to close button' bug happens on 7.16 too so if you choose not to fix it here, please create an issue 🙏🏼