Skip to content
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

fix(a11y): restore focus after popover close with color picker #1272

Merged
merged 21 commits into from
Aug 31, 2021

Conversation

rshen91
Copy link
Contributor

@rshen91 rshen91 commented Jul 28, 2021

Summary

If navigating through a chart with a legend and color picker, when closing the color picker, focus will be returned to the color of the legend item.

Jul-28-2021 08-32-19

TODO

Details

This is a bug fix for accessibility focus management. The old color picker included an EuiWrappingPopover and was changed to EuiPopover. The EuiPopover element from eui returns focus back to the anchor

Issues

Fixes #1266 and #935

@rshen91 rshen91 added :accessibility Accessibility related issue :legend Legend related issue labels Jul 28, 2021
@rshen91 rshen91 requested review from myasonik and nickofthyme July 28, 2021 14:23
@rshen91 rshen91 marked this pull request as ready for review July 28, 2021 14:23
@rshen91 rshen91 marked this pull request as draft July 29, 2021 17:25
@rshen91
Copy link
Contributor Author

rshen91 commented Aug 2, 2021

Working on mocking up a non eui color picker to see if the focus() is able to work.

@rshen91 rshen91 marked this pull request as ready for review August 9, 2021 20:41
Copy link
Collaborator

@nickofthyme nickofthyme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this in the right solution here because the EuiPopover is expected to be passed a child (aka button) to be rendered and clicked to open the popover. See the empty <div> below in the list of legend items.

image

The EuiWrappingPopover is different in that it latches onto a given button element. This is intentional on the charts side to control what people are able to render as legend items.

I know you tried a million things to get it to work with the EuiWrappingPopover, do you think there is some change that eui could make to allow this to work as is?

@rshen91
Copy link
Contributor Author

rshen91 commented Aug 11, 2021

Thanks for all your help! I reverted the changes I made in 12b52dc to have the story use the euiWrappingPopover instead and opened issue elastic/eui#5015 for the focus management of the Done button issue.

@rshen91
Copy link
Contributor Author

rshen91 commented Aug 17, 2021

Will reopen when above linked issue is resolved

@rshen91 rshen91 closed this Aug 17, 2021
@rshen91 rshen91 reopened this Aug 31, 2021
@rshen91 rshen91 requested review from nickofthyme and removed request for myasonik August 31, 2021 14:40
Copy link
Member

@markov00 markov00 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally it works fine in Chrome and Firefox.
I've tested in also in Safari and I've noticed that the color icon is not focusable, could you please check and open an issue + fix for that?

@rshen91
Copy link
Contributor Author

rshen91 commented Aug 31, 2021

Checked in safari to see what's up. It looks like the same functionality is working in below video.

keycastr.in.safari.mp4

@rshen91 rshen91 merged commit 0c6f945 into elastic:master Aug 31, 2021
@rshen91 rshen91 deleted the focus-manage branch August 31, 2021 18:11
nickofthyme pushed a commit that referenced this pull request Sep 13, 2021
# [35.0.0](v34.2.1...v35.0.0) (2021-09-13)

### Bug Fixes

* **a11y:** restore focus after popover close with color picker ([#1272](#1272)) ([0c6f945](0c6f945)), closes [#1266](#1266) [#935](#935)
* **build:** fix license in package.json ([#1362](#1362)) ([d524fdf](d524fdf))
* **deps:** update dependency @elastic/eui to ^37.5.0 ([#1341](#1341)) ([fb05c98](fb05c98))
* **deps:** update dependency @elastic/eui to ^37.6.1 ([#1359](#1359)) ([2ae90ce](2ae90ce))
* **deps:** update dependency @elastic/eui to ^37.7.0 ([#1373](#1373)) ([553b6b0](553b6b0))
* **heatmap:** filter out tooltip picked shapes in x-axis area ([#1351](#1351)) ([174047d](174047d)), closes [#1215](#1215)
* **heatmap:** remove values when brushing only over axes ([#1364](#1364)) ([77ff8d3](77ff8d3))

### Features

* **annotations:** add onClickHandler for annotations ([#1293](#1293)) ([48198be](48198be)), closes [#1211](#1211)
* **heatmap:** add text color contrast to heatmap cells ([#1342](#1342)) ([f9a26ef](f9a26ef)), closes [#1296](#1296)
* **heatmap:** reduce font size to fit label within cells ([#1352](#1352)) ([16b5546](16b5546))
* **xy:** mutilayer time axis step 1 ([#1326](#1326)) ([867b1f5](867b1f5))

### BREAKING CHANGES

* **xy:** - feat: removes the axis deduplication feature
- fix: `showDuplicatedTicks` causes a duplication check on the actual axis tick label (possibly yielded by `Axis.tickLabel` rather than the more general `tickFormat`)
* **heatmap:** the `config.label.fontSize` prop is replaced by `config.label.minFontSize` and `config.label.maxFontSize`. You can specify the same value for both properties to have a fixed font size. The `config.label.align` and `config.label.baseline` props are removed from the `HeatmapConfig` object.
@nickofthyme
Copy link
Collaborator

🎉 This PR is included in version 35.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:accessibility Accessibility related issue :legend Legend related issue released Issue released publicly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[a11y] legend loses focus after closing its popover
3 participants