[InfraUI] Fix group by "x" icon keyboard controls

[Kerry350]

Summary

Addresses: #28159

This fixes the issue described in the ticket, plus stopping the outer context menu opening and closing every time a group by item was removed (regardless of whether it was a click or key event).

Overview:

TL;DR event order issue. Hard to fix elegantly without changing EUI components.

This was a bit more involved than it appears. The bug boiled down to classic browser event order — there is a writeup from Chandler here. Removal with spacebar is unaffected due to the event order used, but Enter falls foul of the following order:

• Keydown
• Click
• Keyup

EUI's internal <KeyboardAccessibility /> component, which enhances the <EUIIcon /> components inside of the <EUIBadge /> components, uses keyUp events. As such our use of the onClick handler on the <EuiFilterButton /> was "swallowing" the event and not allowing the keyUp event to fire or be handled.

Our onClick handler in this case does the following: this.setState(state => ({ isPopoverOpen: !state.isPopoverOpen })); , it's the setState call itself which effectively halts the execution of the events somewhere (setState causes a re-render, components are unmounted, and therefore the synthetic event doesn't make it back to the keyUp handler). Remove the setState call and keyUp will fire, but obviously we need to have both handled.

I agree with Chandler that just swapping keyUp for keyDown in the <KeyboardAccessibility /> component could cause unwanted side effects, especially with so many components working together within EUI.

This defers a solution to our side, which makes things a little bit harder as we can't just add new event handlers or props to the EUI components. I even considered extending from the class and using super but these are all functional components.

Therefore, with that limitation, this PR uses a "buffer" component to capture the events as they bubble, decides whether the event originated from the icon (the "x"), and if so either A) stops propagation directly if handling click events (the context menu should not open / close after the removal) or B) sets an instance property flag so that the onClick handler on <EuiFilterButton /> can opt-out of running it's function body if we've just intercepted a keyDown event using Spacebar / Enter on the inner <EUIIcon />.

The gross bits here are:

A) Using raw DOM node properties (className and tabIndex) to infer if the event.target is the EUI Icon. Given this particular part of the tree doesn't render too much, this should be okay, but of course it becomes brittle if the EUI implementation changes...

B) Using an instance property. Technically, this isn't that bad. Instance properties still have a place, and fulfil certain things that don't make sense with setState (timers, animation loops etc).

I played around with a couple of solutions, and given the limitations we have here, this was the best of the solutions. Others ended up only fixing one of the issues, not both, or using "magic numbers" (eww) in setTimeout.

A quick note on the tslint:disable-next-line, this was because elements with event handlers are expected to have a ARIA role. But the <span /> here has no role. It's better to have no role, than an incorrect role. Also, I've typed el as any as despite the fact VS Code correctly picks up the property type ((property) HTMLOrSVGElement.tabIndex: number) when using SVGElement, the pre-commit hook scripts fail with an error regarding tabIndex.

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

• [ ] This was checked for cross-browser compatibility, including a check against IE11 (No, as master is broken in IE11 currently)
• [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
• [ ] Documentation was added for features that require explanation or tutorials
• [ ] Unit or functional tests were updated or added to match the most common scenarios
• This was checked for keyboard-only and screenreader accessibility

[elasticmachine]

Pinging @elastic/infrastructure-ui

[elasticmachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request

[skh]

@Kerry350 Thanks for the detailed write-up as well as the comments in the code 👍

Your solution looks good to me, I would prefer to be able to verify the fix in IE though, once master is unbroken again, before we merge this. What do you think?

[skh]

Thinking further about the grossness of the fix, maybe it is an indication that the solution of clickable badges within a popup menu widget is maybe not the most accessible solution in the first place. I'm not suggesting we should change it now.

@formgeist @makwarth what do you think? Do you know if there have been any discussions about this already somewhere?

(For context, it's about this element:
)

[Kerry350]

@skh Thanks for the review.

Your solution looks good to me, I would prefer to be able to verify the fix in IE though, once master is unbroken again, before we merge this. What do you think?

Works for me given the fix PR is open, I'll keep an eye on #32159 👍

Thinking further about the grossness of the fix, maybe it is an indication that the solution of clickable badges within a popup menu widget is maybe not the most accessible solution in the first place.

Hmm, maybe. I don't really think there's an issue with the design / UX here though. A pills / badges listing within a wider context menu is a fairly common paradigm, and it seems to fit the use case. But I'll let others weigh in there.

From a developer perspective, and the icky code, I'd say the problem is not having access to the inner workings of these EUI components (which we shouldn't...). But what's exposed, props wise, doesn't allow us the flexibility of an elegant solution. For instance, if we imagined that the <EUIBadge /> component had a iconOnKeydown prop along with the iconOnClick prop, this code would look much nicer, and we'd not need to ride on things like raw DOM node properties, or a buffer "event catcher" component, instead we'd just set the flag or stop propagation.

But then the issue is "just" adding the prop isn't that simple. It'd require adding the prop to the badge, but then also changing the wrapping keyboard accessibility component to pass on that information (as it does to trigger the click prop). It becomes a question of whether that is useful for everyone (all EUI consumers) or just us I guess.

With such nuance in the EUI components (so many components being composed together in different ways) it's hard to stop the need to sometimes have to inspect events and stop propagation in some way manually. In this case EUIBadges can be used independently, but because we put them within a EuiFilterButton we have a bubbling issue. It'd be hard for EUI to guess and handle all combinations automatically, especially given EUI doesn't know the context and nuance of what the handler may do on the consumer side.

I feel like if this scenario were to appear again, in any app code not just infra, it'd be a good indication that EUI does need to expand on the props on offer.

Sorry, a lot of thoughts here that were hard to unpick eloquently. Hard to balance the A) Keeping EUI components streamlined / flexible and B) Ensuring all app contexts / pairings / nestings work with limited intervention.

[formgeist]

@skh @Kerry350 We're currently working on a similar pattern in APM, where we want to display permanent filters as selectors so customers can filter by transaction type and environment without having to use the search bar. Current mocks are available in #30943 and still WIP. I think it's the intention that we try and align as much as possible on these patterns across Observability, so I'm sure when we have a solution ready, we will review together with you and the other Obs teams on how to achieve consistency.

Not knowing the background for the implementation of this filter, I'm curious why we choose to use EuiBadges inside the filter button. Does the user really need to remove directly on the select element, when they might as well open the select and deselect, much like Github's multi-select where the selected items are floated to the top. I would add that I see that there's a state missing in the filter button that displays the chosen values, instead of only showing "Grouped by (2)" or something similar. I'm intending to raise this with the Kibana Design team when we're looking at the APM filters solution.

Typically there isn't enough horizontal space to display selected values inside the element itself, so we'd need some kind of summary row that possibly displays the selected values as text or EuiBadges that you can remove. That's similar to the filters available in the Filter bar i.e. in Discover.

Possibly something along these lines:

[elasticmachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request

[Kerry350]

@formgeist Thanks for your input 👌Thinking more on this and what you've said there indeed seems to be some issues with the way this works currently.

Does the user really need to remove directly on the select element, when they might as well open the select and deselect, much like Github's multi-select where the selected items are floated to the top.

No, there is no explicit need for this.

I would add that I see that there's a state missing in the filter button that displays the chosen values, instead of only showing "Grouped by (2)" or something similar. I'm intending to raise this with the Kibana Design team when we're looking at the APM filters solution.

Yep, makes sense 👍

Typically there isn't enough horizontal space to display selected values inside the element itself, so we'd need some kind of summary row that possibly displays the selected values as text or EuiBadges that you can remove. That's similar to the filters available in the Filter bar i.e. in Discover.
Possibly something along these lines:

Nice, I like the mockup you've shared. The summary row makes things a lot clearer, and a lot less cramped.

I think it's the intention that we try and align as much as possible on these patterns across Observability, so I'm sure when we have a solution ready, we will review together with you and the other Obs teams on how to achieve consistency.

Yep, sounds good 👍The more consistency we can achieve the better.

@skh Given that this is ongoing and a WIP currently, I'd suggest we merge this PR once it looks okay in IE11. And then look to implement the same solution as APM if viable once that's ready. I don't think it makes sense to have an intermediate step where we go from what we have now, to something improved, to the new APM solution (especially as this is related to a bug, and not user complaints).

[skh]

@skh Given that this is ongoing and a WIP currently, I'd suggest we merge this PR once it looks okay in IE11. And then look to implement the same solution as APM if viable once that's ready.

Yes, totally agree.

[formgeist]

@Kerry350 @skh I'll make sure to keep you updated on any progress we make on the dedicated filters implementation, so can hopefully update or align Infra and possibly other Observability UI's with similar solutions.

[Kerry350]

@skh I've given this a test in IE11 by using a branch with my changes plus Chandler's fixes in his PR.

These changes / fixes do work in IE11.

However, #29627 is not fixed by the changes here, and is still broken in IE11. I had hoped that issue might have been fixed too by proxy.

Given all of the feedback on this from you, me and Casper I'm tempted to say let's revert my changes so we don't have to maintain horrible code and remove the use of the little 'x' icon on the badges (that's optional functionality in the component). So what we'd be left with is the badges for visual information only, and you add and remove items in the contextual menu — this would align with how other popular solutions work like Casper said. Then down the line we align with APM with their new filter designs.

What do you think? Otherwise, we merge this, and then the other ticket needs to be worked on by someone. But, ultimately, I'm now at a point where I feel the whole paradigm is just too flaky and brittle.

[skh]

But, ultimately, I'm now at a point where I feel the whole paradigm is just too flaky and brittle.

I do agree. I think, however, that if we only take away the closing x on the badges we might get bug reports because it's not immediately clear how to get rid of a grouping.

I'm not sure what a good alternative for now could be. Ideas I have right now:

• when a grouping is selected, keep showing the badges but without the x (as you suggest)
• when the dropdown is opened and a grouping is selected, offer a "Clear all" option at the top
• and/or when the dropdown opens and a selected grouping is hovered over, turn the checkmark into an x to indicate a second click will remove it

Would any of these be doable in a reasonable amount of time and in a cleaner way than the current fix?

[Kerry350]

@skh

that if we only take away the closing x on the badges we might get bug reports because it's not immediately clear how to get rid of a grouping.

Hmm, I wonder if that is planning for a problem that doesn't exist. Do we know if people have struggled with the EUI context menu in general?

Would any of these be doable in a reasonable amount of time and in a cleaner way than the current fix?

when a grouping is selected, keep showing the badges but without the x (as you suggest)

This one would be easy to implement, and only requires code removal, no additions.

when the dropdown is opened and a grouping is selected, offer a "Clear all" option at the top

I think it would be possible to add this custom option relatively easily as the title prop of EuiContextMenuPanel accepts any node so there could be a custom button that handles this. However, no other contextual menus offer this option, and it does take up room in the title area. Given we can only select up to two groupings I don't think this really adds much.

and/or when the dropdown opens and a selected grouping is hovered over, turn the checkmark into an x to indicate a second click will remove it

I can't see an easy way to implement this. EuiContextMenuItem accepts an icon prop, however there's no way to specify that this should change on hover. As well as hover, focussing with keyboard controls would also need show the other icon. And that customisability is not available with EuiContextMenuItem.

I'm keen on Casper's solution, which introduces the group by badges in a new row. I'm working on this at the moment though, which shuffles the elements around in that area. Once that work is finished it'd be possible to have what Casper suggested sit alongside to the right of the context menu. However this would be quite a large visual change, and when the APM solution comes along if we'd like to use it things would change again.

For now shall we go with when a grouping is selected, keep showing the badges but without the x? I'm very hesitant to file issues against EUI for new functionality or awkwardly hack anything in that isn't natively supported when it's possible this will change again soon.

[skh]

Hmm, I wonder if that is planning for a problem that doesn't exist. Do we know if people have struggled with the EUI context menu in general?

No. I only know that I do use the x on the badges when I test groupings, that's a sample size of one and not representative.

For now shall we go with when a grouping is selected, keep showing the badges but without the x?

Yes, let's start with that. If it makes users unhappy we'll notice quickly.

[Kerry350]

@skh

Yes, let's start with that. If it makes users unhappy we'll notice quickly.

This is ready for another look. I've reverted the original changes, and made the amendments agreed (removing the removal icon from the badges). I've tested the adding / removing of items (with the context menu) in IE11 and everything works fine.

[elasticmachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request

[elasticmachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request

[skh]

This works well now. Great job exploring all the options, even if we decided against them in the end!

[elasticmachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request