feat(legend): add keyboard navigation #880

rshen91 · 2020-10-27T16:41:27Z

Summary

Closes #613
This PR adds keyboard navigation to the legend. Screen reader tested using VoiceOver on local mac to read the label and extra if relevant.

Checklist

Delete any items that are not applicable to this PR.

Unit tests were updated or added to match the most common scenarios

codecov-io · 2020-11-03T17:39:09Z

Codecov Report

Merging #880 (547f69d) into master (bda63f6) will increase coverage by 0.52%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #880      +/-   ##
==========================================
+ Coverage   70.14%   70.66%   +0.52%     
==========================================
  Files         343      359      +16     
  Lines       11035    11341     +306     
  Branches     2392     2436      +44     
==========================================
+ Hits         7740     8014     +274     
- Misses       3281     3307      +26     
- Partials       14       20       +6

Flag	Coverage Δ
unittests	`70.13% <100.00%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/components/legend/color.tsx	`100.00% <100.00%> (ø)`
src/components/legend/label.tsx	`100.00% <100.00%> (ø)`
src/components/legend/legend_item.tsx	`92.40% <100.00%> (ø)`
src/mocks/series/series.ts	`90.00% <0.00%> (ø)`
src/mocks/store/store.ts	`92.59% <0.00%> (ø)`
src/mocks/series/utils.ts	`100.00% <0.00%> (ø)`
src/mocks/series/series_identifiers.ts	`100.00% <0.00%> (ø)`
src/mocks/specs/specs.ts	`82.95% <0.00%> (ø)`
src/mocks/specs/index.ts	`100.00% <0.00%> (ø)`
src/mocks/annotations/annotations.ts	`73.33% <0.00%> (ø)`
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bda63f6...547f69d. Read the comment docs.

rshen91 · 2020-11-06T22:27:56Z

There are vrts failing for small pixel amounts is it ok to update the stories @markov00 @nickofthyme ?

nickofthyme · 2020-11-09T19:45:35Z

@rshen91 Sorry, yes. I would say it's always appropriate to update the vrt screenshots, then on the PR we can just review what changed in screenshot. Sometimes minor changes somehow create small pixel differences.

markov00

Thanks Rachel, with just few changes to the code it looks great!
I'd like to note a few issues:

the focus style doesn't looks like the focus style used in EUI. Here I'd like to ask @miukimiu her opinion about that and if we need background + border in the same style as EUI
I've noticed an issue with the keyboard: if you enable the color picker with the keyboard we are not passing the focus to the color picker element. Moreover, as shown on the gif, tabbing to the legend item label with the color picker open and clicking on that label cause a crash on the chart
Can you prepare a story that "pass" the focus to the color picker? I'm not sure if this is something that happens automatically or we should implement that through our code
if possible instead of updating all these screenshot, it could be better to fix the difference in the padding of the updated DOM structure
@myasonik I'd like your opinion on the area-labels and the role applied to the legend.

src/chart_types/xy_chart/renderer/canvas/xy_chart.tsx

src/components/legend/color.tsx

src/components/legend/extra.tsx

src/components/legend/label.tsx

src/components/legend/legend.tsx

stories/legend/11_legend_actions.tsx

nickofthyme

Looking good, just a few comments on top of @myasonik's

src/components/legend/color.tsx

stories/legend/11_legend_actions.tsx

nickofthyme · 2020-12-04T16:20:42Z

integration/page_objects/common.ts

+    await page.evaluateOnNewDocument(() => {
+      const style = document.createElement('style');
+      style.type = 'text/css';
+      style.innerHTML = `
+      *,
+      *::after,
+      *::before,
+      .echLegendItem {
+        transition-delay: 0s !important;
+        transition-duration: 0s !important;
+        animation-delay: -0.0001s !important;
+        animation-duration: 0s !important;
+        animation-play-state: paused !important;
+        caret-color: transparent !important;
+      };`;
+      const storyRoot = document.getElementById('#story-root');
+      if (storyRoot) {
+        storyRoot.appendChild(style);
+      }
+    });


Rather than adding all the css here you could just add a class to the #story-root element. No need to fuss with creating a style tag and appending it to the dom.

const storyRoot = document.getElementById('story-root'); if (storyRoot) { storyRoot.classList.add('disable-animations') }

Sidenote: getElementById implies the type of selector so using # won't work

nickofthyme · 2020-12-04T16:23:44Z

.storybook/style.scss

+  // *,
+  // *::after,
+  // *::before,
+  // .echLegendItem {
+  //   transition-delay: 0s !important;
+  //   transition-duration: 0s !important;
+  //   animation-delay: -0.0001s !important;
+  //   animation-duration: 0s !important;
+  //   animation-play-state: paused !important;
+  //   caret-color: transparent !important;
+  // }


By adding the class you can then do...

#story-root { &.disable-animations { *, *::after, *::before{ transition-delay: 0s !important; transition-duration: 0s !important; animation-delay: -0.0001s !important; animation-duration: 0s !important; animation-play-state: paused !important; caret-color: transparent !important; } } }

rshen91 · 2020-12-04T20:49:52Z

jenkins test this please

rshen91 · 2020-12-04T21:44:27Z

@nickofthyme and @markov00 in 3631426 I added a workaround for the vrts that were failing in legend_stories and area_stories with disableAnimations(). Can you let me know your thoughts?

nickofthyme · 2020-12-05T00:14:49Z

integration/page_objects/common.ts

@@ -460,6 +440,13 @@ class CommonPage {
    });
  }

+  async disableAnimations(url: string) {
+    await this.loadElementFromURL(url, '#story-root');


This is a good thought. It must replace the #story-root after each navigation.

But as long as you use this in the action function you don't need to call loadElementFromURL.

Suggested change

await this.loadElementFromURL(url, '#story-root');

nickofthyme · 2020-12-05T00:16:17Z

integration/tests/area_stories.test.ts

+        await common.disableAnimations(
+          'http://localhost:9001/?path=/story/area-chart--with-negative-and-positive&knob-Y scale=log',
+        );


No need to repeat the URL navigation again. The disableAnimations method can just be used as a helper functions in the action.

Suggested change

await common.disableAnimations(

'http://localhost:9001/?path=/story/area-chart--with-negative-and-positive&knob-Y scale=log',

);

await common.disableAnimations();

🎉 Good point - please see 5bdb611

nickofthyme · 2020-12-05T00:16:49Z

integration/tests/area_stories.test.ts

+        await common.disableAnimations(
+          'http://localhost:9001/?path=/story/area-chart--with-negative-and-positive&knob-Y scale=log',
+        );


Same as above. Lmk if this doesn't work.

Suggested change

await common.disableAnimations(

'http://localhost:9001/?path=/story/area-chart--with-negative-and-positive&knob-Y scale=log',

);

await common.disableAnimations();

It works! Thank you! Please see 5bdb611

nickofthyme · 2020-12-05T00:17:06Z

integration/tests/legend_stories.test.ts

+    beforeEach(async () => {
+      await common.disableAnimations('http://localhost:9001/?path=/story/legend--right');
+    });


Is this needed?

Good call - totally not needed - removed in 5bdb611

markov00

LGTM, tested locally and works perfectly.

markov00 · 2020-12-09T14:16:42Z

integration/page_objects/common.ts

@@ -383,6 +440,13 @@ class CommonPage {
    });
  }

+  async disableAnimations() {
+    // await this.loadElementFromURL(url, '#story-root');


remove this line

removed in 55f1628 thanks!

markov00 · 2020-12-09T14:17:14Z

integration/tests/area_stories.test.ts

+    it('shows only positive values when hiding negative one', async () => {
+      const action = async () => {
+        await common.disableAnimations();
+        // 'http://localhost:9001/?path=/story/area-chart--with-negative-and-positive&knob-Y scale=log',


remove these commented URLs

removed in 55f1628 thanks!

nickofthyme

The final result is SUPER clean and smooth, way to go!

I have a few nits about the vrt helper methods. Also, I wonder if we could move the mouse before the tab to show the focus state only and not the focused and hover state.

nickofthyme · 2020-12-09T15:40:37Z

.storybook/style_test.scss

@@ -0,0 +1,76 @@
+@import '../node_modules/@elastic/eui/src/theme_light.scss';


Doesn't look like you are using this file

changed in a5acd93

nickofthyme · 2020-12-09T15:41:23Z

integration/page_objects/common.ts

@@ -121,7 +131,7 @@ type ScreenshotElementAtUrlOptions = ScreenshotDOMElementOptions & {
  /**
   * any desired action to be performed after loading url, prior to screenshot
   */
-  action?: () => void | Promise<void>;
+  action?: () => void | Promise<void> | Iterable<unknown>;


What is this for?

nickofthyme · 2020-12-09T15:42:15Z

integration/page_objects/common.ts

+// eslint-disable-next-line eslint-comments/disable-enable-pair
+/* eslint-disable no-await-in-loop */


Can you just disable the line not the whole file?

Yep this makes sense - made changes in 02a06fe

nickofthyme · 2020-12-09T15:42:39Z

integration/page_objects/common.ts

+    if (actionLabel === 'tab') {
+      let i = 0;
+      while (i < count) {
+        await page.keyboard.press('Tab');


Suggested change

await page.keyboard.press('Tab');

// eslint-disable-next-line no-await-in-loop

await page.keyboard.press('Tab');

changed in a5acd93

nickofthyme · 2020-12-09T15:42:49Z

integration/page_objects/common.ts

+    } else if (actionLabel === 'enter') {
+      let i = 0;
+      while (i < count) {
+        await page.keyboard.press('Enter');


Suggested change

await page.keyboard.press('Enter');

// eslint-disable-next-line no-await-in-loop

await page.keyboard.press('Enter');

nickofthyme · 2020-12-09T15:44:58Z

integration/page_objects/common.ts

+  ) {
+    const action = async () => {
+      await this.disableAnimations();
+      await this.clickMouseRelativeToDOMElement({ top: 242, left: 910 }, this.chartSelector);


This should be a generic method, why are there specific top and left values? Is the purpose of this to just click the element to the tap?

Yep! Just to make sure that storybook tabbing doesn't go down all the individual stories in the pane before reaching the iframe

# [24.5.0](v24.4.0...v24.5.0) (2021-01-30) ### Bug Fixes * add theme min radius to point shape ([#996](#996)) ([eb37175](eb37175)) * align tooltip z-index to EUI tooltip z-index ([#931](#931)) ([ffd626b](ffd626b)) * chart state and series functions cleanup ([#989](#989)) ([944ac6c](944ac6c)) * create unique ids for dot icons ([#971](#971)) ([e1ce768](e1ce768)) * external tooltip legend extra value sync ([#993](#993)) ([13ad05a](13ad05a)) * **legend:** disable focus and keyboard navigation for legend in partition ch… ([#952](#952)) ([03bd2f7](03bd2f7)) * **legend:** hierarchical legend order should follow the tree paths ([#947](#947)) ([f9218ad](f9218ad)), closes [#944](#944) * **legend:** remove ids for circles ([#973](#973)) ([b3f4f90](b3f4f90)) ### Features * **cursor:** improve theme styling for crosshair ([#980](#980)) ([6c4dafd](6c4dafd)) * **legend:** display pie chart legend extra ([#939](#939)) ([d14de01](d14de01)) * **legend:** add keyboard navigation ([#880](#880)) ([87c227d](87c227d)) * **partition:** Flame and icicle chart ([#965](#965)) ([3df73d0](3df73d0)) * **partition:** legend hover options ([#978](#978)) ([f810d94](f810d94)) * **xy:** support multiple point shapes on line, area and bubble charts ([#988](#988)) ([1392b7d](1392b7d))

markov00 · 2021-01-30T15:43:07Z

🎉 This PR is included in version 24.5.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

feat: add tabbing into legend

8846dd2

rshen91 added :legend Legend related issue :accessibility Accessibility related issue labels Oct 27, 2020

rshen91 self-assigned this Oct 27, 2020

rshen91 added 6 commits October 27, 2020 10:42

Merge remote-tracking branch 'upstream/master' into legend-keyboard

3f66d55

feat: add focus to navigation of legend

b85af45

style: fixing echLegendLlistContainer orientation

5342cc8

fix: remove outer button fixing style issues

5bfd8ab

test: update unit tests

7ded2df

fix: improve navigation for action story

f783dba

rshen91 added 3 commits November 6, 2020 10:02

feat: add screen reader for extra

16fcfa7

Merge remote-tracking branch 'upstream/master' into legend-keyboard

d0d4a2e

Merge remote-tracking branch 'upstream/master' into legend-keyboard

c4b60e1

rshen91 marked this pull request as ready for review November 6, 2020 22:26

rshen91 requested a review from myasonik November 9, 2020 18:15

fix: change aria label and unit test snapshots

cfd84a6

test: update vrts

8bafadd

markov00 requested review from markov00 and miukimiu November 11, 2020 09:34

markov00 reviewed Nov 11, 2020

View reviewed changes

rshen91 added 3 commits November 11, 2020 08:10

Merge remote-tracking branch 'upstream/master' into legend-keyboard

09c1048

refactor: add some cleanup

69c528f

test: revert vrt updates

835655f

myasonik suggested changes Nov 11, 2020

View reviewed changes

nickofthyme reviewed Nov 11, 2020

View reviewed changes

src/components/legend/color.tsx Outdated Show resolved Hide resolved

src/components/legend/color.tsx Outdated Show resolved Hide resolved

src/components/legend/color.tsx Outdated Show resolved Hide resolved

src/components/legend/color.tsx Outdated Show resolved Hide resolved

feat: initial review changes

4637ccc

myasonik reviewed Nov 11, 2020

View reviewed changes

src/components/legend/color.tsx Outdated Show resolved Hide resolved

src/components/legend/color.tsx Outdated Show resolved Hide resolved

src/components/legend/color.tsx Outdated Show resolved Hide resolved

stories/legend/11_legend_actions.tsx Show resolved Hide resolved

Merge remote-tracking branch 'upstream/master' into legend-keyboard

c260426

nickofthyme reviewed Dec 4, 2020

View reviewed changes

rshen91 added 2 commits December 4, 2020 13:26

test: add disable-animations to common and area stories

3631426

Merge remote-tracking branch 'upstream/master' into legend-keyboard

64e99f8

test: add missing area vrts

02a2555

nickofthyme reviewed Dec 5, 2020

View reviewed changes

refactor: remove url parameter from disableAnimations

5bdb611

rshen91 requested a review from nickofthyme December 6, 2020 19:46

Merge remote-tracking branch 'upstream/master' into legend-keyboard

8932129

markov00 approved these changes Dec 9, 2020

View reviewed changes

style: remove old comments

55f1628

nickofthyme requested changes Dec 9, 2020

View reviewed changes

style: disable eslint in lines vs entire file

02a06fe

nickofthyme approved these changes Dec 9, 2020

View reviewed changes

rshen91 added 2 commits December 9, 2020 11:25

refactor: pr feedback changes

a5acd93

Merge branch 'master' into legend-keyboard

547f69d

rshen91 merged commit 87c227d into elastic:master Dec 9, 2020

rshen91 deleted the legend-keyboard branch December 9, 2020 19:55

rshen91 mentioned this pull request Dec 16, 2020

fix(legend): disable focus and keyboard navigation for legend in partition ch… #952

Merged

markov00 added the released Issue released publicly label Jan 30, 2021

gvnmagni mentioned this pull request May 30, 2024

[Legend] Focus on data series after click #2443

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(legend): add keyboard navigation #880

feat(legend): add keyboard navigation #880

rshen91 commented Oct 27, 2020 •

edited

Loading

codecov-io commented Nov 3, 2020 •

edited

Loading

rshen91 commented Nov 6, 2020

nickofthyme commented Nov 9, 2020

markov00 left a comment •

edited

Loading

nickofthyme left a comment

nickofthyme Dec 4, 2020 •

edited

Loading

nickofthyme Dec 4, 2020 •

edited

Loading

rshen91 commented Dec 4, 2020

rshen91 commented Dec 4, 2020

nickofthyme Dec 5, 2020

nickofthyme Dec 5, 2020

rshen91 Dec 6, 2020

nickofthyme Dec 5, 2020 •

edited

Loading

rshen91 Dec 6, 2020

nickofthyme Dec 5, 2020

rshen91 Dec 6, 2020

markov00 left a comment

markov00 Dec 9, 2020

rshen91 Dec 9, 2020

markov00 Dec 9, 2020

rshen91 Dec 9, 2020

nickofthyme left a comment

nickofthyme Dec 9, 2020

rshen91 Dec 9, 2020

nickofthyme Dec 9, 2020

nickofthyme Dec 9, 2020

rshen91 Dec 9, 2020

nickofthyme Dec 9, 2020

rshen91 Dec 9, 2020

nickofthyme Dec 9, 2020

nickofthyme Dec 9, 2020

rshen91 Dec 9, 2020

markov00 commented Jan 30, 2021

		@@ -0,0 +1,76 @@
		@import '../node_modules/@elastic/eui/src/theme_light.scss';

		// eslint-disable-next-line eslint-comments/disable-enable-pair
		/* eslint-disable no-await-in-loop */

	await page.keyboard.press('Tab');
	// eslint-disable-next-line no-await-in-loop
	await page.keyboard.press('Tab');

	await page.keyboard.press('Enter');
	// eslint-disable-next-line no-await-in-loop
	await page.keyboard.press('Enter');

feat(legend): add keyboard navigation #880

feat(legend): add keyboard navigation #880

Conversation

rshen91 commented Oct 27, 2020 • edited Loading

Summary

Checklist

codecov-io commented Nov 3, 2020 • edited Loading

Codecov Report

rshen91 commented Nov 6, 2020

nickofthyme commented Nov 9, 2020

markov00 left a comment • edited Loading

Choose a reason for hiding this comment

nickofthyme left a comment

Choose a reason for hiding this comment

nickofthyme Dec 4, 2020 • edited Loading

Choose a reason for hiding this comment

nickofthyme Dec 4, 2020 • edited Loading

Choose a reason for hiding this comment

rshen91 commented Dec 4, 2020

rshen91 commented Dec 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickofthyme Dec 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markov00 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickofthyme left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markov00 commented Jan 30, 2021

rshen91 commented Oct 27, 2020 •

edited

Loading

codecov-io commented Nov 3, 2020 •

edited

Loading

markov00 left a comment •

edited

Loading

nickofthyme Dec 4, 2020 •

edited

Loading

nickofthyme Dec 4, 2020 •

edited

Loading

nickofthyme Dec 5, 2020 •

edited

Loading