[Controls] Range slider a11y and performance improvements #159271

Heenawter · 2023-06-07T21:55:28Z

Summary

The main goal of this PR is to fix the serious "Buttons must have discernible text" a11y failure - this is accomplished by switching from building our own range slider button using EuiFlexGroup to instead using EuiFormControlLayoutDelimited, which both resolves these a11y issues and also fixes a rendering regression:

Before	After

As part of this, I also took some time to clean up some of the range slider code, which hasn't really been touched in awhile - this includes...

moving the debounce on range selections from the embeddable's input$ subscription to the component itself, as described here.
fixing a bug where resetting the range slider would unnecessarily cause unsaved changes, as described here.
improving the onClick behaviour (with some notable limitations), as described here.

As a follow up, we need to move the "clear selections" button to a hover action, which will enable us to then fully move forward with our transition to the EuiDualRange component.

Checklist

Unit or functional tests were updated or added to match the most common scenarios
~~Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)~~

Note
Details provided here on why only partial keyboard support is currently supported
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

For maintainers

This was checked for breaking API changes and was labeled appropriately

Heenawter · 2023-06-12T15:55:15Z

src/plugins/controls/public/range_slider/components/range_slider_button.tsx

+        <EuiFieldNumber
+          controlOnly
+          fullWidth
+          value={value[0] === String(min) ? '' : value[0]}


This makes it so that, if you manually select the min/max values, they are treated as "no selection" - this did not happen before:

Before After

Heenawter · 2023-06-12T15:56:04Z

src/plugins/controls/public/range_slider/embeddable/range_slider_embeddable.tsx

    this.subscriptions.add(
      this.getInput$()
        .pipe(
-          debounceTime(400),
-          distinctUntilChanged((a, b) => isEqual(a.value, b.value)),
+          distinctUntilChanged((a, b) => isEqual(a.value ?? ['', ''], b.value ?? ['', ''])),


I moved the debounce to the component itself - this limits the amount of input changes that are fired, which makes the dragging feel a lot smoother (although it's still not perfect).

Heenawter · 2023-06-12T16:39:09Z

src/plugins/controls/public/range_slider/components/range_slider_button.tsx

+  const onClick = useCallback(
+    (event) => {
+      // the popover should remain open if the click/focus target is one of the number inputs
+      if (isPopoverOpen && event.target instanceof HTMLInputElement) {
+        return;
+      }
+      setIsPopoverOpen(true);
+    },
+    [isPopoverOpen, setIsPopoverOpen]
+  );


This is meant to replicate the behaviour of the EuiDualRange component:

EuiDualRange Before After

Note that this doesn't currently respond to focus/blur events as it should, which means keyboard interactivity is compromised.

Basically, the disableFocusTrap on the EuiInputPopover prevents the popover from being actually focusable/interactive via keyboard input. We could remove the disableFocusTrap prop, but this means that someone using the keyboard controls can only make changes to their selected range via the up/down arrow keys and they cannot manually type in a given number - this is much worse UX IMO!
Because of the disableFocusTrap prop, this also means that someone using keyboard controls cannot access the "clear selections" button - this is another reason why moving this to a hover action should be a priority.

We will be able to clean all this up + improve the keyboard accessibility once we are officially able to make the transition to the EuiDualRange component rather than building our own :)

This keyboard interactivity downside is totally fine for the time being, especially since this is still better than it was, and we may not even release in this intermediate state. Nice work!

elasticmachine · 2023-06-12T20:14:11Z

Pinging @elastic/kibana-accessibility (Project:Accessibility)

elasticmachine · 2023-06-12T20:14:12Z

Pinging @elastic/kibana-presentation (Team:Presentation)

ThomThomson

Looked through the code, and ran this locally!

The code all looks good, but I was able to run into some strange states with a control group with a options list -> range slider -> options list. I'll keep trying to see if I can reproduce this, or if it's a problem in main.

Anyway, I added a few comments as well.

ThomThomson · 2023-06-13T19:35:13Z

src/plugins/controls/common/control_group/control_group_panel_diff_system.ts

+  [RANGE_SLIDER_CONTROL]: {
+    getPanelIsEqual: (initialInput, newInput) => {
+      if (!deepEqual(omit(initialInput, 'explicitInput'), omit(newInput, 'explicitInput'))) {
+        return false;
+      }
+
+      const { value: valueA = ['', ''] }: Partial<RangeSliderEmbeddableInput> =
+        initialInput.explicitInput;
+      const { value: valueB = ['', ''] }: Partial<RangeSliderEmbeddableInput> =
+        newInput.explicitInput;
+      return isEqual(valueA, valueB);
+    },
+  },


Nice to see these stability improvements!

ThomThomson · 2023-06-13T19:37:53Z

src/plugins/controls/public/range_slider/components/range_slider_button.tsx

+  const onClick = useCallback(
+    (event) => {
+      // the popover should remain open if the click/focus target is one of the number inputs
+      if (isPopoverOpen && event.target instanceof HTMLInputElement) {
+        return;
+      }
+      setIsPopoverOpen(true);
+    },
+    [isPopoverOpen, setIsPopoverOpen]
+  );


This keyboard interactivity downside is totally fine for the time being, especially since this is still better than it was, and we may not even release in this intermediate state. Nice work!

ThomThomson · 2023-06-13T19:42:29Z

src/plugins/controls/public/range_slider/components/range_slider_control.tsx

-  const hasUpperBoundSelection = value[1] !== '';
+  const debouncedOnChange = useMemo(
+    () =>
+      debounce((newRange: RangeValue) => {


Nice usage of the lodash debounce to keep some of this in the react component rather than putting it into the embeddable class. Also great to see the debounce added!

ThomThomson

Update: I can reproduce the above issue in main, so it isn't a problem caused by this PR.

Additionally, ideally this will be solved when we move to EuiDualRange.

andreadelrio

Design changes LGTM. Great improvements!

kibana-ci · 2023-06-15T16:47:18Z

💚 Build Succeeded

Buildkite Build
Commit: 4f45393

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`controls`	157	158	+1

Async chunks

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

id	before	after	diff
`controls`	190.9KB	190.3KB	-623.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`controls`	36.8KB	37.0KB	+222.0B

Unknown metric groups

ESLint disabled line counts

id	before	after	diff
`enterpriseSearch`	13	15	+2
`securitySolution`	410	414	+4
total			+6

Total ESLint disabled count

id	before	after	diff
`enterpriseSearch`	14	16	+2
`securitySolution`	493	497	+4
total			+6

History

💛 Build #135417 was flaky d077aae
💔 Build #135339 failed a0c1024
💔 Build #135335 failed 0ef557d
💚 Build #134576 succeeded 297da5a
💚 Build #134555 succeeded c353f58
💛 Build #134528 was flaky dd2e4b6

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

cc @Heenawter

Closes #159395 Closes #153383 ## Summary This PR moves the "clear selections" button for all controls (options list, range slider, and time slider) from inside their respective popovers to a general hover action - this not only saves users a click for this common interaction (which has actually been brought in user feedback up as a downside of the current controls compared to the legacy controls), it also allows us to fully move forward with migrating the range slider control to the `EuiDualRange` component. This will be done in a follow up PR, which should both (1) clean up our range slider code significantly and (2) fix the [bug discussed here](#159271 (review)). The related issue can be tracked [here](#159724), since we might not be able to get to it right away. This "clear selections" action is available in both view and edit mode, like so: | | Edit mode | View mode | |--------|--------|--------| | **Range slider** | ![image](https://github.com/elastic/kibana/assets/8698078/83cb1e1a-0b20-43aa-a37b-14484b5f4945) | ![image](https://github.com/elastic/kibana/assets/8698078/0d28ce03-5242-4f3a-8a05-d447bca50ddb) | | **Options list** | ![image](https://github.com/elastic/kibana/assets/8698078/066257f6-c0ce-4e33-a193-5bbc62e341a6) | ![image](https://github.com/elastic/kibana/assets/8698078/d1ec124c-f5ee-4137-9eb9-33e06d522435) | | **Time slider** | ![image](https://github.com/elastic/kibana/assets/8698078/33b8bb80-fa0c-4281-ae81-f1e1b44086f3) | ![image](https://github.com/elastic/kibana/assets/8698078/bd7c41ae-706c-45f3-8b49-9bd4d259e5cf) | You may notice in the above screenshots that the "delete" action is now represented with a red trash icon rather than a red cross, and the tooltip text was also changed to use the word "Delete" rather than the word "Remove" - these changes were both made to be more consistent with the "Delete panel" action available on dashboards: | Delete control - Before | Delete control - After | Delete panel | |--------|--------|--------| | ![Screenshot 2023-06-13 at 5 32 22 PM](https://github.com/elastic/kibana/assets/8698078/2600b197-653b-43ea-a043-a50be7e6a796) | ![image](https://github.com/elastic/kibana/assets/8698078/5ef80380-2575-45fc-ba11-c59f3f252ac3) | <img src="https://github.com/elastic/kibana/assets/8698078/a7f65777-45cf-44f2-96a7-f1042cb25e02"/> | Beyond these changes, I also made a few quick changes to the time slider control, including: 1. Fixing the appearance so that the background is once again white, as described [here](#159526 (comment)) 2. Adding comparison logic so that clearing selections no longer causes unsaved changes unnecessarily, as described [here](#159526 (comment)) ### Videos **Before** https://github.com/elastic/kibana/assets/8698078/96365c85-748e-4fd7-ae5d-589aa11a23ef **After** https://github.com/elastic/kibana/assets/8698078/68352559-e71b-4b5e-8709-587016f0b35a ### Checklist - [x] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [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)) - [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: kibanamachine <[email protected]>

Replace with EuiFormControlLayoutDelimited

3b1cd48

Heenawter added Project:Accessibility Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas loe:medium Medium Level of Effort impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. Project:Controls labels Jun 7, 2023

Heenawter self-assigned this Jun 7, 2023

Heenawter force-pushed the fix-range-slider-a11y_2023-06-01 branch 2 times, most recently from 46908fb to 1ab47c1 Compare June 8, 2023 20:18

Heenawter added 3 commits June 8, 2023 14:19

Add debounce through unpublished changes

2c26893

Add error message

b38dd7a

Clean up

b62fa6c

Heenawter force-pushed the fix-range-slider-a11y_2023-06-01 branch 3 times, most recently from 08d548f to aefe1d9 Compare June 8, 2023 22:59

Fix reset dashboard bug

d859706

Heenawter force-pushed the fix-range-slider-a11y_2023-06-01 branch from aefe1d9 to d859706 Compare June 8, 2023 23:08

Re-add accidentally deleted data-test-subj

4f7a0f0

Heenawter changed the title ~~[Controls] Fix range slider a11y~~ [Controls] Range slider a11y and performance improvements Jun 9, 2023

Heenawter mentioned this pull request Jun 9, 2023

[Controls] Make "clear selections" a hover action for all data controls #159395

Closed

Fix default min and max

02acc60

Heenawter force-pushed the fix-range-slider-a11y_2023-06-01 branch from 6bf0aff to 02acc60 Compare June 9, 2023 14:53

Heenawter added 3 commits June 9, 2023 12:47

Fix functional test + error state

90fee54

Move debounce back to component

d4e8729

Fix loading + prevent double update on reset

cbce8a0

Heenawter force-pushed the fix-range-slider-a11y_2023-06-01 branch from 7fffa6f to cbce8a0 Compare June 9, 2023 21:56

Cleaner fix

a982e6f

Heenawter force-pushed the fix-range-slider-a11y_2023-06-01 branch from 18275fd to 8a339a6 Compare June 12, 2023 15:43

Fix replace control tests

15f3495

Heenawter force-pushed the fix-range-slider-a11y_2023-06-01 branch from 8a339a6 to 15f3495 Compare June 12, 2023 15:46

Heenawter commented Jun 12, 2023

View reviewed changes

Fix keyboard controls for popover

21a3c4b

Heenawter commented Jun 12, 2023

View reviewed changes

Heenawter added 2 commits June 12, 2023 10:56

Remove focus/blur logic

5fcfae2

Fix close popover for range slider

dd2e4b6

Heenawter marked this pull request as ready for review June 12, 2023 20:14

Heenawter requested review from a team as code owners June 12, 2023 20:14

Merge branch 'main' into fix-range-slider-a11y_2023-06-01

c353f58

Heenawter added release_note:fix v8.9.0 labels Jun 12, 2023

Fix reload infinite loading bug

297da5a

ThomThomson reviewed Jun 13, 2023

View reviewed changes

ThomThomson approved these changes Jun 13, 2023

View reviewed changes

This was referenced Jun 13, 2023

[Controls] Move "clear selections" to hover action #159526

Merged

[Controls] Fully migrate range slider to EuiDualRange component #159724

Closed

Fix error state when there is no data

0ef557d

andreadelrio approved these changes Jun 14, 2023

View reviewed changes

Heenawter and others added 5 commits June 14, 2023 12:18

Clean up logs

a0c1024

Better fix for error state

ae4638d

Cleaner implementation

6ac785a

Clean up more

d077aae

Merge branch 'main' into fix-range-slider-a11y_2023-06-01

4f45393

Heenawter merged commit 9b0f106 into elastic:main Jun 15, 2023

kibanamachine added the backport:skip This commit does not require backporting label Jun 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Controls] Range slider a11y and performance improvements #159271

[Controls] Range slider a11y and performance improvements #159271

Heenawter commented Jun 7, 2023 •

edited

Loading

Heenawter Jun 12, 2023

Heenawter Jun 12, 2023 •

edited

Loading

Heenawter Jun 12, 2023 •

edited

Loading

ThomThomson Jun 13, 2023

elasticmachine commented Jun 12, 2023

elasticmachine commented Jun 12, 2023

ThomThomson left a comment

ThomThomson Jun 13, 2023

ThomThomson Jun 13, 2023

ThomThomson Jun 13, 2023

ThomThomson left a comment

andreadelrio left a comment

kibana-ci commented Jun 15, 2023

ESLint disabled line counts

Total ESLint disabled count

[Controls] Range slider a11y and performance improvements #159271

[Controls] Range slider a11y and performance improvements #159271

Conversation

Heenawter commented Jun 7, 2023 • edited Loading

Summary

Checklist

For maintainers

Heenawter Jun 12, 2023

Choose a reason for hiding this comment

Heenawter Jun 12, 2023 • edited Loading

Choose a reason for hiding this comment

Heenawter Jun 12, 2023 • edited Loading

Choose a reason for hiding this comment

ThomThomson Jun 13, 2023

Choose a reason for hiding this comment

elasticmachine commented Jun 12, 2023

elasticmachine commented Jun 12, 2023

ThomThomson left a comment

Choose a reason for hiding this comment

ThomThomson Jun 13, 2023

Choose a reason for hiding this comment

ThomThomson Jun 13, 2023

Choose a reason for hiding this comment

ThomThomson Jun 13, 2023

Choose a reason for hiding this comment

ThomThomson left a comment

Choose a reason for hiding this comment

andreadelrio left a comment

Choose a reason for hiding this comment

kibana-ci commented Jun 15, 2023

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

Page load bundle

ESLint disabled line counts

Total ESLint disabled count

History

Heenawter commented Jun 7, 2023 •

edited

Loading

Heenawter Jun 12, 2023 •

edited

Loading

Heenawter Jun 12, 2023 •

edited

Loading