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

[Dashboard Navigation] Fix a11y concerns #174772

Merged
merged 14 commits into from
Jan 18, 2024

Conversation

Heenawter
Copy link
Contributor

@Heenawter Heenawter commented Jan 12, 2024

Summary

This PR addresses the a11y issues that were brought up as part of the a11y review via the following:

  • Added aria-current attribute to the current dashboard link

    Before After
    Screenshot 2024-01-12 at 9 49 54 AM Screenshot 2024-01-12 at 10 03 11 AM
  • Returned focus to the create flyout after adding/editing a link

    Before After
    image image
  • Added aria-label attribute to Technical preview badge

    Before After
    Screenshot 2024-01-12 at 11 16 14 AM Screenshot 2024-01-12 at 11 13 07 AM
  • Added more context to the aria-labels for the edit/delete link buttons

    Before After
    Screenshot 2024-01-12 at 11 43 45 AM Screenshot 2024-01-12 at 11 43 19 AM

Checklist

For maintainers

@Heenawter Heenawter added release_note:fix Project:Accessibility Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas loe:small Small Level of Effort impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. Project:Dashboard Navigation Related to the Dashboard Navigation Project a11yReviewNeeded Accessibility design or code review labels Jan 12, 2024
@Heenawter Heenawter self-assigned this Jan 12, 2024
@Heenawter
Copy link
Contributor Author

/ci

@Heenawter
Copy link
Contributor Author

/ci

@Heenawter
Copy link
Contributor Author

/ci

@Heenawter
Copy link
Contributor Author

/ci

@Heenawter Heenawter marked this pull request as ready for review January 12, 2024 22:46
@Heenawter Heenawter requested a review from a team as a code owner January 12, 2024 22:46
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

@Heenawter Heenawter requested a review from 1Copenut January 15, 2024 15:52
@nickpeihl nickpeihl self-requested a review January 16, 2024 13:18
Copy link
Member

@nickpeihl nickpeihl left a comment

Choose a reason for hiding this comment

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

Added a couple of nits. Otherwise lgtm. Nice a11y tests! Code review and tested a11y concerns.

Since it was brought up in the review, do you mind changing here the embeddable.type to links. This should hide the customized time range in the panel settings. It was missed when we did the embeddable type rename.

@botelastic botelastic bot added the Feature:Embedding Embedding content via iFrame label Jan 16, 2024
@Heenawter
Copy link
Contributor Author

Heenawter commented Jan 16, 2024

@nickpeihl

This should hide the customized time range in the panel settings. It was missed when we did the embeddable type rename.

Thanks for reminding me!! I meant to address that but totally forgot 🤦 Fixed in d7c4f58 🎉

Copy link
Contributor

@1Copenut 1Copenut left a comment

Choose a reason for hiding this comment

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

This is a big win for a11y @Heenawter! I have one question and one change request (hopefully small). If the change request to set focus after deleting a link from the modal is large or difficult to refactor, let's move it to a follow-on ticket of its own and I'll change my review to Approved.

I appreciate you helping me prove out this collaborative flow for testing accessibility in smaller, feature-size engagements.

@@ -135,10 +139,12 @@ export async function openEditorFlyout(
{ theme: coreServices.theme, i18n: coreServices.i18n }
),
{
tabIndex: 1,
Copy link
Contributor

Choose a reason for hiding this comment

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

@Heenawter Which flyout is this tabIndex being assigned to? I wasn't able to find it in the DOM using a quick document.querySelectorAll('[tabindex="1"]') but had some unexpected tabbing behavior with Firefox + VO. It's entirely possible the new MacOS or VO has disrupted that flow; Firefox seemed okay by itself.

If there is a positive tabindex being applied, that could disrupt the natural tabbing order, so I'd like to evaluate that separately if it does appear in a place I didn't find.

Copy link
Contributor Author

@Heenawter Heenawter Jan 18, 2024

Choose a reason for hiding this comment

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

Oh, good catch - I think I accidentally left this in when I was testing different things for refocusing the main flyout after editing/adding links. I've removed it, and it doesn't seem to impact the focusing behaviour 🙇

@@ -44,6 +46,12 @@ export async function openLinkEditorFlyout({
// wait for close animation before unmounting
setTimeout(() => {
if (ref.current) ReactDOM.unmountComponentAtNode(ref.current);

// return focus to the main flyout div to align with a11y standards
const flyoutElement = document.getElementById(mainFlyoutId);
Copy link
Contributor

Choose a reason for hiding this comment

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

This works well when I add and edit links. Can we include logic to also reset this focus to the modal container when I delete a link using the inline trashcan icon?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 11468ac 🎉

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id before after diff
links 24.9KB 25.1KB +206.0B

Page load bundle

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

id before after diff
embeddable 60.6KB 60.6KB -5.0B
links 34.1KB 34.3KB +201.0B
total +196.0B

History

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

cc @Heenawter

Copy link
Contributor

@1Copenut 1Copenut left a comment

Choose a reason for hiding this comment

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

👍 LGTM!

@Heenawter Heenawter merged commit f2ec534 into elastic:main Jan 18, 2024
17 checks passed
@Heenawter Heenawter deleted the links-a11y-fixes_2024-01-12 branch January 18, 2024 19:49
@kibanamachine kibanamachine added v8.13.0 backport:skip This commit does not require backporting labels Jan 18, 2024
CoenWarmer pushed a commit to CoenWarmer/kibana that referenced this pull request Feb 15, 2024
## Summary

This PR addresses the a11y issues that were brought up as part of the
[a11y review](elastic#163802) via the
following:

- **Added `aria-current` attribute to the current dashboard link**
  | Before | After |
  |--------|--------|
| ![Screenshot 2024-01-12 at 9 49
54 AM](https://github.com/elastic/kibana/assets/8698078/975f6acf-fd87-47f3-a12f-02abeb2e5dce)
| ![Screenshot 2024-01-12 at 10 03
11 AM](https://github.com/elastic/kibana/assets/8698078/102b3e12-f98e-4f17-9d51-ac8a70d2f34e)
|

- **Returned focus to the create flyout after adding/editing a link**
  | Before | After |
  |--------|--------|
|
![image](https://github.com/elastic/kibana/assets/8698078/4d74f3b3-788b-47e7-9dcb-d54a6383026b)
|
![image](https://github.com/elastic/kibana/assets/8698078/6b057ee5-094a-49c5-92c2-10deba15b7db)
|

- **Added `aria-label` attribute to `Technical preview` badge**
  | Before | After |
  |--------|--------|
| ![Screenshot 2024-01-12 at 11 16
14 AM](https://github.com/elastic/kibana/assets/8698078/06b454ce-07e9-438e-9d41-1d8505ca0e28)
| ![Screenshot 2024-01-12 at 11 13
07 AM](https://github.com/elastic/kibana/assets/8698078/44ee37c6-18cc-4918-b127-0b9583630612)
|



- **Added more context to the `aria-label`s for the edit/delete link
buttons**
  | Before | After |
  |--------|--------|
| ![Screenshot 2024-01-12 at 11 43
45 AM](https://github.com/elastic/kibana/assets/8698078/f90db90f-3da6-4ab6-807d-fba73315ee66)
| ![Screenshot 2024-01-12 at 11 43
19 AM](https://github.com/elastic/kibana/assets/8698078/2d9e6ae2-6bd4-4957-ab6d-93fda0708471)
|



### 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] [Flaky Test
Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was
used on any tests changed
([Link](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/4856))

![image](https://github.com/elastic/kibana/assets/8698078/74c173dc-2b59-43e8-bc9b-33a1ae8297b0)
- [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 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)
wayneseymour added a commit to wayneseymour/kibana that referenced this pull request Oct 11, 2024
Contributes to: elastic#194817

Why I assigned them:

Assigned advanced_settings due to elastic#175255

Assigned dashboard_controls due to elastic#190797

Assigned dashboard_links due to elastic#174772

Assigned dashboard_panel_options due to elastic#178596

Assigned grok_debugger due to https://github.com/elastic/kibana/blob/main/x-pack/plugins/grokdebugger/kibana.jsonc#L4

Assigned helpers due to elastic#164341 (call site)

Assigned home due to elastic#103192

Assigned index_lifecycle_management due to elastic#116207

Assigned ingest_node_pipelines due to elastic#113783

Assigned kibana_overview due to  https://github.com/elastic/kibana/blob/f00ac7a8a21463e6bb4a2784c3a3884f36c62900/x-pack/plugins/grokdebugger/kibana.jsonc#L4

Assigned management due to elastic#165705

Assigned painless_lab due to https://github.com/elastic/kibana/blob/main/x-pack/plugins/painless_lab/kibana.jsonc#L4

Assigned search_profiler due to elastic#195343

Assigned uptime due to https://github.com/elastic/kibana/blob/main/x-pack/plugins/observability_solution/uptime/kibana.jsonc#L4

Assigned lens due to elastic#175893

Assigned ml_anomaly_detection due to elastic#162126

Assigned canvas due to elastic#164376

Assigned cc replication due to elastic#149069

Assigned enterpise search due to elastic#79359

Assigned graph to due elastic#190797

Assigned license_management due to https://github.com/elastic/kibana/blob/main/x-pack/plugins/license_management/kibana.jsonc#L4

Assigned maps due to elastic#155161

Assigned observability due to https://github.com/elastic/kibana/blob/main/x-pack/plugins/observability_solution/observability/kibana.jsonc#L4

Assigned remote clusters due to elastic#96989

Assigned reporting due to elastic#121435

Assigned rollup_jobs due to https://github.com/elastic/kibana/blob/d57bc9a5d7d64f86b550eff7997605a3090aee9a/x-pack/plugins/rollup/kibana.jsonc#L4

Assigned watcher due to elastic#119717
wayneseymour added a commit that referenced this pull request Oct 22, 2024
## Summary

Assign files within `x-pack/test/accessibility/apps/group[1|2|3]`

### Why I assigned them:

Assigned advanced_settings due to
#175255

Assigned dashboard_controls due to
#190797

Assigned dashboard_links due to
#174772

Assigned dashboard_panel_options due to
#178596

Assigned grok_debugger due to
https://github.com/elastic/kibana/blob/main/x-pack/plugins/grokdebugger/kibana.jsonc#L4

Assigned helpers due to #164341
(call site)

Assigned home due to #103192

Assigned index_lifecycle_management due to
#116207

Assigned ingest_node_pipelines due to
#113783

Assigned kibana_overview due to
https://github.com/elastic/kibana/blob/f00ac7a8a21463e6bb4a2784c3a3884f36c62900/x-pack/plugins/grokdebugger/kibana.jsonc#L4

Assigned management due to #165705

Assigned painless_lab due to
https://github.com/elastic/kibana/blob/main/x-pack/plugins/painless_lab/kibana.jsonc#L4

Assigned search_profiler due to
#195343

Assigned uptime due to
https://github.com/elastic/kibana/blob/main/x-pack/plugins/observability_solution/uptime/kibana.jsonc#L4

Assigned lens due to #175893

Assigned ml_anomaly_detection due to
#162126

Assigned canvas due to #164376

Assigned cc replication due to
#149069

Assigned enterpise search due to
#79359

Assigned graph to due #190797

Assigned license_management due to
https://github.com/elastic/kibana/blob/main/x-pack/plugins/license_management/kibana.jsonc#L4

Assigned maps due to #155161

Assigned observability due to
https://github.com/elastic/kibana/blob/main/x-pack/plugins/observability_solution/observability/kibana.jsonc#L4

Assigned remote clusters due to
#96989

Assigned reporting due to #121435

Assigned rollup_jobs due to
https://github.com/elastic/kibana/blob/d57bc9a5d7d64f86b550eff7997605a3090aee9a/x-pack/plugins/rollup/kibana.jsonc#L4

Assigned watcher due to #119717


Contributes to: #194817

---------

Co-authored-by: kibanamachine <[email protected]>
Co-authored-by: Elastic Machine <[email protected]>
Co-authored-by: Rodney Norris <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a11yReviewNeeded Accessibility design or code review backport:skip This commit does not require backporting Feature:Embedding Embedding content via iFrame impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. loe:small Small Level of Effort Project:Accessibility Project:Dashboard Navigation Related to the Dashboard Navigation Project release_note:fix Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v8.13.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants