-
Notifications
You must be signed in to change notification settings - Fork 841
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
[EuiDualRangeSliders] Slider handles need accessible labels in popovers and two-handle sliders #7035
Comments
…nsistent across all controls (#162651) Closes #140135 Closes #159724 ## Summary ### Changes to range slider This PR migrates the range slider control to use the `EuiDualRange` component, which both (a) removes a bunch of range slider code , thus improving DX, and (b) improves the usability of the control by simplifying the different states and making the slide animation much smoother: | Before | After | |--------|--------| | ![Aug-02-2023 09-19-26](https://github.com/elastic/kibana/assets/8698078/08efbfb8-3aff-4c44-acb3-79442d3fc831) | ![Aug-03-2023 09-10-30](https://github.com/elastic/kibana/assets/8698078/8f62936f-c15d-4ede-9925-d1b314c7a9c9) | Note that, due to the fact that migrating to the `EuiDualRange` component means we no longer control the opening/closing of the popover, this makes the "selections that extend beyond the current min/max of the data" scenario slightly more difficult to handle. Specifically, we need to decide the best time to recalculate the min/max of the slider as the user's selections change. The benefit of having control over the popover opening and closing was that we could calculate the range slider's min/max values **when the popover opened** and lock them in place so that they stayed static as the user dragged: <div align="center"><img width="500px" src="https://github.com/elastic/kibana/assets/8698078/1bf552d4-e70e-41ce-bf85-ab9ed0262398" /></div><br/> However, because the `EuiDualRange` handles the behaviour of the popover and we therefore do not know when it opens/closes, it is not currently possible to fully replicate this old behaviour. We have two main options: 1. Recalculate the min/max of the range slider the user drags, which gives a "sliding" effect: <div align="center"><img width="500px" src="https://github.com/elastic/kibana/assets/8698078/4bb32b22-990e-40e9-a7df-78afca16bd27" /></div> 2. Debounce this min/max calculation - with this, we avoid the "sliding" effect shown above, and the min/max values **only get adjusted** when the pin has been static for a long enough period of time (specifically, `750ms`) or when the pin is dropped (i.e. the user lets go of the mouse): <div align="center"><img width="500px" src="https://github.com/elastic/kibana/assets/8698078/ac8943e6-4bcc-42dd-8cbb-1cdb627ba9f2" /></div> Ultimately, I went with option 2 in this PR. As a future enhancement, we could replicate the old behaviour by adding some sort of `onPopoverClosed` or `onPopoverOpened` logic to the `EuiDualRange` component - however, this would need discussion with the EUI team to ensure that this is the best way to handle it and not too specific to our use case. Since EUI changes take awhile to propagate to KIbana, it's not worth holding up this PR even further. ### Consistency with other control types To keep things consistent, this PR also switches both the options list and time slider controls to use the `EuiInputPopover` component and removes the redundant control titles from the popover header: | Before | After | |--------|--------| | ![image](https://github.com/elastic/kibana/assets/8698078/dce0fc28-0714-48ce-a6df-3aed2e9749d8) | ![image](https://github.com/elastic/kibana/assets/8698078/0619a19a-073f-4d2e-ae40-1d77e9f9a7ae) | | ![image](https://github.com/elastic/kibana/assets/8698078/c7ff43ba-b94f-402a-b61a-ee238065acb4) | ![image](https://github.com/elastic/kibana/assets/8698078/07e95ef5-5db8-490e-9bc8-7f9df4513da5) | | ![image](https://github.com/elastic/kibana/assets/8698078/20a30dd9-2f0c-49f5-a035-37f827c5c1a2) | ![image](https://github.com/elastic/kibana/assets/8698078/64acea5a-5120-4d88-bac3-14f45be98b0d) | ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [x] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [x] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) - Technically, it does - but this will be resolved by the @elastic/eui-team team adding an `aria-label` to the thumb `div`s (elastic/eui#7035). This is purely for the sake of axe and/or automated `a11y` testing - it does not actually impact `a11y` of the range slider, because the components missing an `aria-label` are never accessible via the keyboard. - [x] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [x] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) --------- Co-authored-by: Jatin Kathuria <[email protected]> Co-authored-by: Kibana Machine <[email protected]>
👋 Hi there - this issue hasn't had any activity in 6 months. If the EUI team has not explicitly expressed that this is something on our roadmap, it's unlikely that we'll pick this issue up. We would sincerely appreciate a PR/community contribution if this is something that matters to you! If not, and there is no further activity on this issue for another 6 months (i.e. it's stale for over a year), the issue will be auto-closed. |
❌ Per our previous message, this issue is auto-closing after having been open and inactive for a year. If you strongly feel this is still a high-priority issue, or are interested in contributing, please leave a comment or open a new issue linking to this one for context. |
Describe the problem, pt. 1 - dual range sliders in popovers
The inputs with range in dropdown dual range sliders have two range slider handles that need accessible labels. Screenshot attached below.
These dropdowns are not keyboard accessible by design. Keyboard users (and screen reader users who navigate with a keyboard) interact with the ranges by changing values of the number inputs. However, users who use a mouse with screen reader would have a harder time using this interface because they could still use the unlabeled range slider controls.
Describe the problem, pt. 2- dual range sliders
The dual range sliders not in popovers have an accessible
aria-label
but it's being inherited from theEuiRange
and it's not as meaningful as labeling the sliders individually. This doesn't feel as critical for singleelem[role="slider"]
range sliders, but moreso when there are two.Steps to reproduce
Possible code change
Environment
WCAG or Vendor Guidance (optional)
Screenshots or Trace Logs
The text was updated successfully, but these errors were encountered: