[A11y] Render `aria-label` as text in the icon testenv file #5709

yuliacech · 2022-03-10T17:30:27Z

Summary

Similar to #5702 , this PR updates the testenv file for EuiIcon that renders a mock version of EuiIcon. The current mock attaches aria-label as attribute to a span element that is rendered instead of an icon. This causes a11y violations in Kibana (see elastic/kibana#127185). I'm suggesting rendering aria-label as text if present.

Checklist

I don't think any of the items in the checklist are applicable because this PR only touches the mock file.

~~- [ ] Check against all themes for compatibility in both light and dark modes~~
- [ ] Checked in mobile
- [ ] Checked in Chrome, Safari, Edge, and Firefox
- [ ] Props have proper autodocs and playground toggles
- [ ] Added documentation
~~- [ ] Checked Code Sandbox works for any docs examples~~
- [ ] Added or updated jest and cypress tests
~~- [ ] Checked for breaking changes and labeled appropriately~~
~~- [ ] Checked for accessibility including keyboard-only and screenreader modes~~

A changelog entry exists and is marked appropriately

kibanamachine · 2022-03-10T18:03:21Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5709/

cee-chen

LGTM! Cool solution for this test mock (rendering the label as text)!

cee-chen · 2022-03-10T18:17:10Z

Re: CI failures - it looks like you'll want to run yarn test-unit -u to update all our various snapshots. It shouldn't take super long to run all our Jest tests, fingers crossed

yuliacech · 2022-03-14T13:28:07Z

Thanks a lot for the review, @constancecchen! And for the snaphots update tip 🎉

kibanamachine · 2022-03-14T13:53:59Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5709/

…ing for strict text equality to account for text coming from the EuiScreenReaderOnly component. Also updated tests to account for EuiIcon text that is now rendered when the icon is imported from .testenv (PR 5709 - elastic/eui#5709).

thompsongl · 2022-03-28T20:48:55Z

@yuliacech What's the axe violation this was solving?

cee-chen · 2022-03-28T21:31:12Z

This is the error:

aria-label attribute is not well supported on a span with no valid role attribute.

So one very easy alternative (if this is causing too much downstream Kibana snapshot churn) to this PR would be to add role="img" to our testenv mock instead of rendering the ariaLabel as text.

thompsongl · 2022-03-28T21:35:02Z

That seems like a better solution. The real component would render <img> or <svg> so that's closer to reality than a <span> with coerced text (probably should've been that way when the .testenv. file was first created)

yuliacech · 2022-03-29T08:52:17Z

@thompsongl @constancecchen Yes, the violation was about having an aria-label on a span element. An img element might be a better alternative, but the change would still cause Kibana snapshot updates 😓

thompsongl · 2022-03-29T18:41:07Z

Thanks, @yuliacech

This is less immediate now, but I'm going to plan on exploring the role="img" change. You're right about the snapshot churn; I was more concerned with the type of tests that inspect the rendered text and assert equality. Those are now finding "Info" and "External link" text that is misleading when compared to real environments.

* Updgraded EUI packages in package.json and src/dev/license_checker/config.js * Resolved Jest test failures for Jest test suites 1 and 2. Updated snapshots, and updated equality conditions for specific test cases * Resolve Jest test cases for Jest test suite 3. Updated snapshots for required tests * type fixes * Resolved failing Jest test cases in Jest suite 3. Updated tests checking for strict text equality to account for text coming from the EuiScreenReaderOnly component. Also updated tests to account for EuiIcon text that is now rendered when the icon is imported from .testenv (PR 5709 - elastic/eui#5709). * eui to 52.2.0 * Resolved test cases for Jest test suites 1 and 2. Updated required snapshots. Updated tests using getAllByLabelText and getByLabelText to getAllByText and getByText respectively as the former have been deprecated * Updated Jest tests for Jest test suites 5 and 6. Updated required snapshots. Updated instances of getByLabelText and getAllByLabelText to getByText and getAllByText as the former are now deprecated. * Updated Jest tests for Jest test suite 7. Updated required snapshots. * Completed test case revisions for Jest test suites 1, 3, 6, 7, and 8. Updated required snapshots. Updated various tests to account for text rendering of the EuiIcon text. * eui back to v52.2.0 * removed unused test utils * use .contains for euiicon content * storyshots updates * linting Co-authored-by: Greg Thompson <thompson.glowe@gmail.com>

…ing for strict text equality to account for text coming from the EuiScreenReaderOnly component. Also updated tests to account for EuiIcon text that is now rendered when the icon is imported from .testenv (PR 5709 - elastic/eui#5709).

cee-chen · 2022-03-29T23:08:40Z

Mentioning for recordkeeping, another example in Kibana of unintentional Jest churn based on tests looking for text content: elastic/kibana@d9fc2a1 - the test failed because it was looking for 1 instance of the passed text in the document and received 2 instead.

We should revert the above commit if we also decide to revert this and use role="img" instead

* Updgraded EUI packages in package.json and src/dev/license_checker/config.js * Resolved Jest test failures for Jest test suites 1 and 2. Updated snapshots, and updated equality conditions for specific test cases * Resolve Jest test cases for Jest test suite 3. Updated snapshots for required tests * Resolved failing Jest test cases in Jest suite 3. Updated tests checking for strict text equality to account for text coming from the EuiScreenReaderOnly component. Also updated tests to account for EuiIcon text that is now rendered when the icon is imported from .testenv (PR 5709 - elastic/eui#5709). * type fixes * eui to 52.2.0 * Resolved test cases for Jest test suites 1 and 2. Updated required snapshots. Updated tests using getAllByLabelText and getByLabelText to getAllByText and getByText respectively as the former have been deprecated * Updated Jest tests for Jest test suites 5 and 6. Updated required snapshots. Updated instances of getByLabelText and getAllByLabelText to getByText and getAllByText as the former are now deprecated. * Updated Jest tests for Jest test suite 7. Updated required snapshots. * Completed test case revisions for Jest test suites 1, 3, 6, 7, and 8. Updated required snapshots. Updated various tests to account for text rendering of the EuiIcon text. * removed unused test utils * use .contains for euiicon content * storyshots updates * linting * Fix failing a11y violations tests * Fix Jest failures caused by #eui/5709 - these changes should be reverted if we opt to revert the above PR Co-authored-by: Bree Hall <briannajdhall@gmail.com> Co-authored-by: Greg Thompson <thompson.glowe@gmail.com>

yuliacech added 3 commits March 10, 2022 16:07

[A11y] Removed aria-label from icon testenv file

dbf7f88

[A11y] Update icon testenv file to render aria-label as text

4c171b0

[A11y] Updated the changelog

755b14b

yuliacech marked this pull request as ready for review March 10, 2022 17:36

yuliacech changed the title ~~Testenv icon aria label~~ [A11y] Render aria-label as text in the icon testenv file Mar 10, 2022

yuliacech mentioned this pull request Mar 10, 2022

Jest axe a11y testing elastic/kibana#127185

Merged

cee-chen approved these changes Mar 10, 2022

View reviewed changes

yuliacech and others added 3 commits March 14, 2022 14:05

Merge branch 'main' into testenv_icon_aria_label

8bb9337

Fixed changelog file

a28352b

Updated test snapshots

a575e3d

yuliacech merged commit c7b8818 into elastic:main Mar 14, 2022

breehall mentioned this pull request Mar 22, 2022

Upgrade EUI to v52.2.0 elastic/kibana#128313

Merged

cee-chen mentioned this pull request Mar 29, 2022

Upgrade EUI to v52.2.0 elastic/kibana#128841

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[A11y] Render `aria-label` as text in the icon testenv file #5709

[A11y] Render `aria-label` as text in the icon testenv file #5709

yuliacech commented Mar 10, 2022 •

edited

Loading

kibanamachine commented Mar 10, 2022

cee-chen left a comment

cee-chen commented Mar 10, 2022

yuliacech commented Mar 14, 2022

kibanamachine commented Mar 14, 2022

thompsongl commented Mar 28, 2022

cee-chen commented Mar 28, 2022

thompsongl commented Mar 28, 2022 •

edited

Loading

yuliacech commented Mar 29, 2022

thompsongl commented Mar 29, 2022

cee-chen commented Mar 29, 2022 •

edited

Loading

[A11y] Render aria-label as text in the icon testenv file #5709

[A11y] Render aria-label as text in the icon testenv file #5709

Conversation

yuliacech commented Mar 10, 2022 • edited Loading

Summary

Checklist

kibanamachine commented Mar 10, 2022

cee-chen left a comment

Choose a reason for hiding this comment

cee-chen commented Mar 10, 2022

yuliacech commented Mar 14, 2022

kibanamachine commented Mar 14, 2022

thompsongl commented Mar 28, 2022

cee-chen commented Mar 28, 2022

thompsongl commented Mar 28, 2022 • edited Loading

yuliacech commented Mar 29, 2022

thompsongl commented Mar 29, 2022

cee-chen commented Mar 29, 2022 • edited Loading

[A11y] Render `aria-label` as text in the icon testenv file #5709

[A11y] Render `aria-label` as text in the icon testenv file #5709

yuliacech commented Mar 10, 2022 •

edited

Loading

thompsongl commented Mar 28, 2022 •

edited

Loading

cee-chen commented Mar 29, 2022 •

edited

Loading