[Select] Aria error flagged by WAVE and W3C

[mfsjr]

WAVE errors in tables, in TablePagination/TableFooter

• [ x] The issue is present in the latest release.
• [x ] I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

1. Go to https://material-ui.com/components/tables/
2. Run WAVE
3. Should not get errors from standard components

WAVE identifies 3 "Broken ARIA Reference Errors" associated with three demos

• Sorting & Selection
• Fixed Headers
• Editable Example - this is a material-table which seems to inherit the accessibility bug from TableFooter

Expected Behavior 🤔

No WAVE errors

No WAVE errors

Steps to Reproduce 🕹

Given above

Steps:

Context 🔦

Your Environment 🌎

OSX Mojave, Chrome v80, WAVE v3.0.4

Tech	Version
Material-UI	v4.9.6
React
Browser
TypeScript
etc.

[eps1lon]

Thanks for opening this issue.

Does WAVE provide more information beyond "Broken ARIA Reference Errors"?

[oliviertassinari]

Broken ARIA reference

What It Means

An aria-labelledby or aria-describedby reference exists, but the target for the reference does not exist.

Why It Matters

ARIA labels and descriptions will not be presented if the element referenced does not exist in the page.

How to Fix It

Ensure the element referenced in the aria-labelledby or aria-describedby attribute value is present within the page and presents a proper label or description.

The Algorithm... in English

An element has an aria-labelledby or aria-describedby value that does not match the id attribute value of another element in the page.

Standards and Guidelines

1.3.1 Info and Relationships (Level A)
4.1.2 Name, Role, Value (Level A)

[oliviertassinari]

@mfsjr Thanks for the report.

While this issue focuses on the reported errors. WAVE also reports less important alerts, I will take care of it in batch (e.g. broken anchor link).

[oliviertassinari]

@eps1lon If you would prefer working on it, I can leave it to you. Let me know, I wouldn't want to distract you from the prop-types -> TypeScript migration of the descriptions :D.

[eps1lon]

@oliviertassinari No perfectly fine. That's why I assigned it to you and unsubscribed. But you had to mention me again 😛

[mfsjr]

Thank you @oliviertassinari , @eps1lon for such quick response!

[oliviertassinari]

I could narrow the problem down to #17892 (comment).

@mfsjr What do you think of the following patch? Do you want to work on a pull request? :)

diff --git a/packages/material-ui/src/Select/Select.test.js b/packages/material-ui/src/Select/Select.test.js
index afe173849..bd9986b90 100644
--- a/packages/material-ui/src/Select/Select.test.js
+++ b/packages/material-ui/src/Select/Select.test.js
@@ -409,7 +409,7 @@ describe('<Select />', () => {
       const { getByRole } = render(<Select value="" />);

       // TODO what is the accessible name actually?
-      expect(getByRole('button')).to.have.attribute('aria-labelledby', ' ');
+      expect(getByRole('button')).to.not.have.attribute('aria-labelledby');
     });

     it('is labelled by itself when it has a name', () => {
@@ -417,7 +417,7 @@ describe('<Select />', () => {

       expect(getByRole('button')).to.have.attribute(
         'aria-labelledby',
-        ` ${getByRole('button').getAttribute('id')}`,
+        getByRole('button').getAttribute('id'),
       );
     });

diff --git a/packages/material-ui/src/Select/SelectInput.js b/packages/material-ui/src/Select/SelectInput.js
index 6ace9e2ba..9af95915e 100644
--- a/packages/material-ui/src/Select/SelectInput.js
+++ b/packages/material-ui/src/Select/SelectInput.js
@@ -320,7 +320,7 @@ const SelectInput = React.forwardRef(function SelectInput(props, ref) {
         tabIndex={tabIndex}
         role="button"
         aria-expanded={open ? 'true' : undefined}
-        aria-labelledby={`${labelId || ''} ${buttonId || ''}`}
+        aria-labelledby={[labelId, buttonId].filter(Boolean).join(' ') || undefined}
         aria-haspopup="listbox"
         onKeyDown={handleKeyDown}
         onMouseDown={disabled || readOnly ? null : handleMouseDown}

It solves:

1. WAVE

2. https://validator.w3.org/

[mfsjr]

Hi @oliviertassinari , looking...
yarn test is failing, probably doesn't make sense for me to do a pull request if I can't test it (homebrew yarn install v1.22.4 on OSX Catalina, yarn install succeeded following instructions from the material-ui contributing.md)

lerna ERR! yarn run typescript exited 1 in 'docs'
lerna ERR! yarn run typescript stdout:
$ tsc -p tsconfig.json
../node_modules/material-table/types/index.d.ts(231,3): error TS2300: Duplicate identifier 'searchText'.

[oliviertassinari]

@mfsjr Did you try to have the CI run the tests? Also, did you pull the latest changes and run yarn at the root of the repository?

[mfsjr]

@oliviertassinari The docs mention CI running tests upon submitting the PR only, so no, I did not, I was just trying to make sure my environment could run the tests on my fork of master.

I ran "yarn install" which took a few minutes and succeeded.
yarn install v1.22.4
[1/4] 🔍 Resolving packages...
success Already up-to-date.

Git log shows the most recent:
Author: Maksim Mamrikov [email protected]
Date: Mon Mar 30 00:26:05 2020 -0700

[Slide] Mark `direction` as optional in TypeScript (#20338)

[mfsjr]

@oliviertassinari Looks like the test failure is originating in material-table, not material-ui. I guess that is because material-table is in a material-ui demo, and somehow gets evaluated during testing. When I fix it there, 'yarn test' succeeds.
Known bug in material-table, see mbrn/material-table#1675

[oliviertassinari]

@mfsjr It's good to know, thanks for sharing. So you have an issue with your installation steps. We force an older version of material-table.

[mfsjr]

PR submitted: #20356

[mfsjr]

Thanks very much @oliviertassinari and @eps1lon !