-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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.
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
[InfraUI] Fix group by "x" icon keyboard controls #31960
[InfraUI] Fix group by "x" icon keyboard controls #31960
Conversation
Pinging @elastic/infrastructure-ui |
💚 Build Succeeded |
…ensure menu doesn't open / close for no reason
@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 |
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? |
@skh Thanks for the review.
Works for me given the fix PR is open, I'll keep an eye on #32159 👍
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 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 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. |
@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. |
c17bd9c
to
a2a2a6a
Compare
💚 Build Succeeded |
@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.
No, there is no explicit need for this.
Yep, makes sense 👍
Nice, I like the mockup you've shared. The summary row makes things a lot clearer, and a lot less cramped.
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). |
Yes, totally agree. |
@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. |
I do agree. I think, however, that if we only take away the closing I'm not sure what a good alternative for now could be. Ideas I have right now:
Would any of these be doable in a reasonable amount of time and in a cleaner way than the current fix? |
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?
This one would be easy to implement, and only requires code removal, no additions.
I think it would be possible to add this custom option relatively easily as the
I can't see an easy way to implement this. 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 |
No. I only know that I do use the
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. |
💔 Build Failed |
💔 Build Failed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works well now. Great job exploring all the options, even if we decided against them in the end!
💚 Build Succeeded |
* First pass at ensuring click events do not swallow keyUp event, also ensure menu doesn't open / close for no reason * Add comment * Add key * Revert changes * Remove icons on badges within context menu * Remove unused translation with script
* First pass at ensuring click events do not swallow keyUp event, also ensure menu doesn't open / close for no reason * Add comment * Add key * Revert changes * Remove icons on badges within context menu * Remove unused translation with script
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, butEnter
falls foul of the following order:EUI's internal
<KeyboardAccessibility />
component, which enhances the<EUIIcon />
components inside of the<EUIBadge />
components, useskeyUp
events. As such our use of theonClick
handler on the<EuiFilterButton />
was "swallowing" the event and not allowing thekeyUp
event to fire or be handled.Our
onClick
handler in this case does the following:this.setState(state => ({ isPopoverOpen: !state.isPopoverOpen }));
, it's thesetState
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 thekeyUp
handler). Remove thesetState
call andkeyUp
will fire, but obviously we need to have both handled.I agree with Chandler that just swapping
keyUp
forkeyDown
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 akeyDown
event using Spacebar / Enter on the inner<EUIIcon />
.The gross bits here are:
A) Using raw DOM node properties (
className
andtabIndex
) to infer if theevent.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 ARIArole
. But the<span />
here has norole
. It's better to have norole
, than an incorrectrole
. Also, I've typedel
asany
as despite the fact VS Code correctly picks up the property type ((property) HTMLOrSVGElement.tabIndex: number
) when usingSVGElement
, the pre-commit hook scripts fail with an error regardingtabIndex
.Checklist
Use
strikethroughsto 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, asmaster
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