[EuiBasicTable & EuiInMemoryTable] Fix multiple accessibility/axe errors on EuiBasicTable and EuiInMemoryTable pages #5241

cee-chen · 2021-10-04T23:35:44Z

Summary

a11y/axe tests would have caught the duplicate ID issue in #5237, but we had them skipped in our a11y-testing.js testing due to multiple failures that I'm assuming we didn't have bandwidth to fix at the time.

This PR attempts to put us back on the straight and narrow by re-enabling a11y testing on our table docs pages and fixing the various axe issues that were present on the 2 pages.

I'll post a couple before/after screenshots in individual comments below in a bit, but I also strongly recommend following along by-commit.

Checklist

~~- [ ] 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 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

…s on the page - This is possibly only an issue with our docs and the fact that we use the same dataset repeatedly across multiple examples, but solves the error of duplicated IDs and shouldn't negatively affect production users

… the page - there's no real need for this popover to have a custom ID instead of a randomized one - remove it - This is possibly only an issue with our docs and the fact that we use the same dataset repeatedly across examples, but solves the error of duplicated IDs and shouldn't negatively affect production users

- An `aria-label` was being passed to the `PaginationBar` component, but it wasn't actually being correctly used: - `PaginationBar` was never passing an `aria-label` prop down to `EuiTablePagination` or `EuiPagination` - The i18n typing was passing a react element instead of a string via render prop

…able docs - While we added an aria-label for each table's paginatoin nav, they still need to be unique for multiple tables on the page, which means adding a tableCaption for each demo

kibanamachine · 2021-10-05T00:17:01Z

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

- empty `td`s within a `thead` is valid per https://webaim.org/techniques/tables/data's examples, but should not have the `scope` attr set

- I opted to use visually empty headings, populated with EuiScreenReaderOnly text as an example for users looking to implement their own columns + fix odd data-test-subjs caused by either undefined or node column names

- missing labels on checkbox elements, duplicate checkbox ID (solved in elastic#5237) - These are all a11y solutions that come OOTB in EuiBasicTable, but need to be added for the custom example

- these should very likely just be paragraphs, not headings

cee-chen

Quick rundown of the source code changes with before/after screenshots - I didn't do those for docs-only fixes, as those felt pretty straightforward and should be easy enough to follow along in the commit messages

cee-chen · 2021-10-04T23:56:22Z

scripts/a11y-testing.js

-    `${root}#/tabular-content/tables`,
-    `${root}#/tabular-content/in-memory-tables`,


Feel free to test locally yourself with yarn test-a11y!

cee-chen · 2021-10-05T00:03:57Z

src/components/basic_table/basic_table.tsx

@@ -1166,7 +1166,7 @@ export class EuiBasicTable<T = any> extends Component<
        <EuiI18n token="euiBasicTable.selectThisRow" default="Select this row">
          {(selectThisRow: string) => (
            <EuiCheckbox
-              id={`${key}-checkbox`}
+              id={`${this.tableId}${key}-checkbox`}


Axe error: https://dequeuniversity.com/rules/axe/4.3/duplicate-id-active
Commit: 06fbd23

Before:

After:

NB: This is really only an issue with our docs because we use the same dataset repeatedly across multiple examples - but it's definitely possible other end-users may have multiple isSelectable tables on the same page. Generally, it shouldn't negatively affect production users (who should likely prefer the data-test-subj attr over id to hook into the checkbox if needed)

cee-chen · 2021-10-05T00:13:32Z

src/components/search_bar/filters/field_value_selection_filter.tsx

@@ -368,7 +368,6 @@ export class FieldValueSelectionFilter extends Component<

    return (
      <EuiPopover
-        id={`${config.type}_${index}`}


Axe error: https://dequeuniversity.com/rules/axe/4.3/duplicate-id-active
Commit: 08ca142

Before:

After:

NB: This is really only an issue with our docs because we use the same dataset repeatedly across multiple examples. This shouldn't have any impact on production users that I can reasonably think of - honestly, I'm not totally sure why this ID was necessary in the first place, as EuiPopover generates its own internal ID for screen reader text.

cee-chen · 2021-10-05T00:17:42Z

src/components/basic_table/pagination_bar.tsx

@@ -80,6 +82,7 @@ export const PaginationBar = ({
        onChangeItemsPerPage={onPageSizeChange}
        onChangePage={onPageChange}
        aria-controls={ariaControls}
+        aria-label={ariaLabel}


A bit of a d'oh moment: while we were passing an aria-label prop from above/EuiBasicTable, we were never actually passing it from the PaginationBar component to the underlying EuiTablePagination->EuiPagination component. This fixes that and allows the pagination nav element to correctly have a unique label.

Before:

After:

cee-chen · 2021-10-05T00:21:59Z

src/components/basic_table/basic_table.tsx

+        <EuiI18n
+          token="euiBasicTable.tablePagination"
+          default="Pagination for preceding table: {tableCaption}"
+          values={{ tableCaption }}
+        >
+          {(tablePagination: string) => (
+            <PaginationBar
+              pagination={pagination}
+              onPageSizeChange={this.onPageSizeChange.bind(this)}
+              onPageChange={this.onPageChange.bind(this)}
+              aria-controls={this.tableId}
+              aria-label={tablePagination}
+            />
+          )}
+        </EuiI18n>


Axe error: https://dequeuniversity.com/rules/axe/4.3/landmark-unique
Commits: 20afa32 and 51c053b

What this does:

Correctly uses EuiI18n's render prop functionality to pass a string to aria-label instead of a React element (type error)

Passes an aria-label to all table pagination nav elements always, regardless of whether the tableCaption prop is set

For users who have just one table on a page, this passes the unique label on the nav axe check, and screen readers will read out just "pagination for preceding table" without a title, which sounds fairly natural to me.

If users have multiple tables on the same page, they should use tableCaption to pass axe/to differentiate between their tables.

@1Copenut - I played around with aria-labelledby and pointing this at our hidden table caption SR text (picture below), but I ended up not being a huge fan of the behavior. It also doesn't solve the duplicate axe issue when users have multiple tables that happen to have the same row & page count - I think we should consider encouraging tableCaption usage instead. Any thoughts or objections?

@constancecchen I forgot to comment earlier. No objection to this approach. I agree with you to provide good defaults and guidance on using tableCaption to create meaningful labels.

cee-chen · 2021-10-05T01:46:38Z

src/components/table/table_header_cell.tsx

@@ -155,6 +155,7 @@ export const EuiTableHeaderCell: FunctionComponent<EuiTableHeaderCellProps> = ({
  const styleObj = resolveWidthAsStyle(style, width);

  const CellComponent = children ? 'th' : 'td';
+  const cellScope = CellComponent === 'th' ? scope ?? 'col' : undefined; // `scope` is only valid on `th` elements


axe error: https://dequeuniversity.com/rules/axe/4.3/scope-attr-valid
commit: b517a77

I've personally run into/seen this error before when running axe-devtools on my previous Kibana plugin so I'm excited to be able to fix it in EUI!

I like this a lot. It gives us a default scope="col" but allows for passing scope="row" in cases where we might use TH as a row header.

cee-chen · 2021-10-05T01:49:53Z

src-docs/src/views/tables/expanding_rows/expanding_rows.js

+      name: (
+        <EuiScreenReaderOnly>
+          <span>Expand rows</span>
+        </EuiScreenReaderOnly>
+      ),


This addition was optional with b517a77, but I added it because:

It works really well for screen readers, and we should always try to provide more context where possible

It provides an example for consuming users/devs to use

It addresses a 'needs review' issue for https://dequeuniversity.com/rules/axe/4.3/empty-table-header

Demo of visually hidden table column heading with SR text:

sr-table-heading.mp4

cee-chen · 2021-10-05T01:54:57Z

src/components/basic_table/basic_table.tsx

+            data-test-subj={`tableHeaderCell_${
+              typeof name === 'string' ? name : ''
+            }_${index}`}


Before:

Bonus: if you passed a react node it rendered as tableHeaderCell_[object Object]_4 lol

After:

kibanamachine · 2021-10-05T02:07:05Z

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

1Copenut

Thank you @constancecchen! One change request, one discussion item and we should be good to go.

1Copenut · 2021-10-05T19:25:20Z

src/components/table/table_header_cell.tsx

@@ -155,6 +155,7 @@ export const EuiTableHeaderCell: FunctionComponent<EuiTableHeaderCellProps> = ({
  const styleObj = resolveWidthAsStyle(style, width);

  const CellComponent = children ? 'th' : 'td';
+  const cellScope = CellComponent === 'th' ? scope ?? 'col' : undefined; // `scope` is only valid on `th` elements


I like this a lot. It gives us a default scope="col" but allows for passing scope="row" in cases where we might use TH as a row header.

src/components/basic_table/basic_table.tsx

1Copenut · 2021-10-05T19:37:41Z

src/components/basic_table/basic_table.tsx

+              pagination={pagination}
+              onPageSizeChange={this.onPageSizeChange.bind(this)}
+              onPageChange={this.onPageChange.bind(this)}
+              aria-controls={this.tableId}


I'm on the fence about the value of aria-controls. It's supported in JAWS, but not NVDA or VoiceOver at this time. If we have just one pagination table on the page, the UX is fairly straight forward. If there are more than one, it could create unexpected behaviors. If there are accidental duplicate IDs, users may end up in a different table.

I'm okay with this staying in if it's difficult to remove, out of scope, or warrants more discussion.

TBH I'm not a super huge expert / haven't used JAWS, so I'm not familiar with how aria-controls is supposed to work 🙈 I'm kind of tempted to leave it in in case aria-controls gets more support in the future. Can we leave it for now and revisit it down the road maybe?

FWIW,

If there are accidental duplicate IDs, users may end up in a different table

isn't possible because the table ID is randomly generated between tables, and we don't allow users to pass in a custom ID for tables.

Sounds good, let's leave it for now and keep an ear out for user feedback.

Awesome, thanks a million Trevor! Mind giving this a ✅ if all your change requests have been addressed?

kibanamachine · 2021-10-05T20:17:09Z

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

cee-chen · 2021-10-05T21:09:28Z

jenkins test this

kibanamachine · 2021-10-05T21:35:43Z

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

1Copenut

👍 LGTM! Thank you @constancecchen

cchaos · 2021-10-18T22:09:24Z

src-docs/src/views/tables/custom/custom.js

@@ -741,6 +756,7 @@ export default class extends Component {
        <EuiSpacer size="m" />

        <EuiTablePagination
+          tableCaption="Custom EuiTable demo"


@constancecchen Question here, is this supposed to be right? In master I'm getting a dev console error that tableCaption is just being passed all the way to the DOM element.

tableCaption is a valid prop that we document and use for accessibility - it looks like we should be pulling it out via ...rest or other spread operator so it doesn't fall through to the DOM. That's not caused by this PR specifically though, it's something that would have already been an issue when tableCaption was first implemented.

I'd offer to open a PR but I'm a little knee-deep in last minute data grid fires ATM, feel free to open an issue and I can hopefully get to it later!

…ors on EuiBasicTable and EuiInMemoryTable pages (elastic#5241) * Fix duplicate checkbox `id`s with multiple isSelectable EuiBasicTables on the page - This is possibly only an issue with our docs and the fact that we use the same dataset repeatedly across multiple examples, but solves the error of duplicated IDs and shouldn't negatively affect production users * Fix duplicate popover `id`s with multiple EuiInMemoryTable filters on the page - there's no real need for this popover to have a custom ID instead of a randomized one - remove it - This is possibly only an issue with our docs and the fact that we use the same dataset repeatedly across examples, but solves the error of duplicated IDs and shouldn't negatively affect production users * Fix missing `aria-label` on table pagination - An `aria-label` was being passed to the `PaginationBar` component, but it wasn't actually being correctly used: - `PaginationBar` was never passing an `aria-label` prop down to `EuiTablePagination` or `EuiPagination` - The i18n typing was passing a react element instead of a string via render prop * Fix multiple `landmark-unique` issues on EuiBasicTable & EuiInMemoryTable docs - While we added an aria-label for each table's paginatoin nav, they still need to be unique for multiple tables on the page, which means adding a tableCaption for each demo * Fix `scope-attr-valid` errors on empty table column headings - empty `td`s within a `thead` is valid per https://webaim.org/techniques/tables/data's examples, but should not have the `scope` attr set * Improve demos with empty table headings - I opted to use visually empty headings, populated with EuiScreenReaderOnly text as an example for users looking to implement their own columns + fix odd data-test-subjs caused by either undefined or node column names * Fix various axe failures on custom EuiTable example - missing labels on checkbox elements, duplicate checkbox ID (solved in elastic#5237) - These are all a11y solutions that come OOTB in EuiBasicTable, but need to be added for the custom example * Fix unsemantic headings in EuiBasicTable responsive documentation - these should very likely just be paragraphs, not headings * Re-enable basic table & in-memory table documentations in a11y tests * Add changelog entry * PR feedback: screen reader landmark copy

cee-chen added 4 commits October 4, 2021 16:13

Fix multiple landmark-unique issues on EuiBasicTable & EuiInMemoryT…

51c053b

…able docs - While we added an aria-label for each table's paginatoin nav, they still need to be unique for multiple tables on the page, which means adding a tableCaption for each demo

cee-chen added the accessibility label Oct 4, 2021

cee-chen requested a review from 1Copenut October 4, 2021 23:35

cee-chen force-pushed the table-axe-fixes branch from 424cab8 to d66cec0 Compare October 4, 2021 23:49

cee-chen added 6 commits October 4, 2021 18:38

Fix scope-attr-valid errors on empty table column headings

b517a77

- empty `td`s within a `thead` is valid per https://webaim.org/techniques/tables/data's examples, but should not have the `scope` attr set

Improve demos with empty table headings

7d4e735

- I opted to use visually empty headings, populated with EuiScreenReaderOnly text as an example for users looking to implement their own columns + fix odd data-test-subjs caused by either undefined or node column names

Fix various axe failures on custom EuiTable example

b42992b

- missing labels on checkbox elements, duplicate checkbox ID (solved in elastic#5237) - These are all a11y solutions that come OOTB in EuiBasicTable, but need to be added for the custom example

Fix unsemantic headings in EuiBasicTable responsive documentation

6477253

- these should very likely just be paragraphs, not headings

Re-enable basic table & in-memory table documentations in a11y tests

c4f6773

Add changelog entry

ff2d070

cee-chen force-pushed the table-axe-fixes branch from d66cec0 to ff2d070 Compare October 5, 2021 01:41

cee-chen commented Oct 5, 2021

View reviewed changes

1Copenut suggested changes Oct 5, 2021

View reviewed changes

cee-chen added 2 commits October 5, 2021 12:53

PR feedback: screen reader landmark copy

68b48a6

Merge branch 'master' into table-axe-fixes

cc50123

1Copenut approved these changes Oct 5, 2021

View reviewed changes

cee-chen merged commit cbf7c3a into elastic:master Oct 5, 2021

cee-chen deleted the table-axe-fixes branch October 5, 2021 22:22

breehall mentioned this pull request Oct 6, 2021

Bumping EUI to 39.0.0 elastic/kibana#113633

Merged

cee-chen mentioned this pull request Oct 13, 2021

Upgrade EUI to v39.1.1 elastic/kibana#114732

Merged

cchaos reviewed Oct 18, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EuiBasicTable & EuiInMemoryTable] Fix multiple accessibility/axe errors on EuiBasicTable and EuiInMemoryTable pages #5241

[EuiBasicTable & EuiInMemoryTable] Fix multiple accessibility/axe errors on EuiBasicTable and EuiInMemoryTable pages #5241

cee-chen commented Oct 4, 2021 •

edited

Loading

kibanamachine commented Oct 5, 2021

cee-chen left a comment

cee-chen Oct 4, 2021

cee-chen Oct 5, 2021

cee-chen Oct 5, 2021

cee-chen Oct 5, 2021

cee-chen Oct 5, 2021

1Copenut Oct 5, 2021

cee-chen Oct 5, 2021 •

edited

Loading

1Copenut Oct 5, 2021

cee-chen Oct 5, 2021 •

edited

Loading

cee-chen Oct 5, 2021

kibanamachine commented Oct 5, 2021

1Copenut left a comment

1Copenut Oct 5, 2021

1Copenut Oct 5, 2021

cee-chen Oct 5, 2021

1Copenut Oct 5, 2021

cee-chen Oct 5, 2021

kibanamachine commented Oct 5, 2021

cee-chen commented Oct 5, 2021

kibanamachine commented Oct 5, 2021

1Copenut left a comment

cchaos Oct 18, 2021

cee-chen Oct 18, 2021

		`${root}#/tabular-content/tables`,
		`${root}#/tabular-content/in-memory-tables`,

[EuiBasicTable & EuiInMemoryTable] Fix multiple accessibility/axe errors on EuiBasicTable and EuiInMemoryTable pages #5241

[EuiBasicTable & EuiInMemoryTable] Fix multiple accessibility/axe errors on EuiBasicTable and EuiInMemoryTable pages #5241

Conversation

cee-chen commented Oct 4, 2021 • edited Loading

Summary

Checklist

kibanamachine commented Oct 5, 2021

cee-chen 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

cee-chen Oct 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cee-chen Oct 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibanamachine commented Oct 5, 2021

1Copenut 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

kibanamachine commented Oct 5, 2021

cee-chen commented Oct 5, 2021

kibanamachine commented Oct 5, 2021

1Copenut left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cee-chen commented Oct 4, 2021 •

edited

Loading

cee-chen Oct 5, 2021 •

edited

Loading

cee-chen Oct 5, 2021 •

edited

Loading