[Select] Add labelId to implement proper labelling #17892
Conversation
eps1lon
added
accessibility
|
@material-ui/core: parsed: +0.08% , gzip: +0.11% Details of bundle changes.Comparing: 1e4d2db...c5b1f66
|
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit f88a550:
|
There was a problem hiding this comment.
Great to follow the WAI-ARIA.
It seems that selects/SimpleSelect.tsx and selects/NativeSelects.tsx demos are no longer "in sync". Should we care? Should we demonstrate the same cases? Should we break the demo into smaller ones?
Historically, having almost identical demos helped to make sure the behavior where almost native prop invariant.
We have a couple of errors/warnings in https://validator.w3.org/nu/?doc=https%3A%2F%2Fdeploy-preview-17892--material-ui.netlify.com%2Fcomponents%2Fselects%2F
| @@ -282,15 +285,15 @@ const SelectInput = React.forwardRef(function SelectInput(props, ref) { | |||
| tabIndex={tabIndex} | |||
| role="button" | |||
| aria-expanded={open ? 'true' : undefined} | |||
There was a problem hiding this comment.
Side question, would it work with?
| aria-expanded={open ? 'true' : undefined} | |
| aria-expanded={open} |
(smaller)
There was a problem hiding this comment.
It's explicitly defined that way in the apg:
Set by the JavaScript when the listbox is displayed.
Otherwise, is not present.
Though it is quite at odds with how aria-expanded is defined
undefined (default) | The element, or another grouping element it controls, is neither expandable nor collapsible; all its child elements are shown or there are no child elements.
-- | --
I would wait for w3c/aria-practices#618 to be resolved.
There was a problem hiding this comment.
Benchmarking for the combobox, I have found most implementation to set false. For instance: https://www.w3.org/TR/wai-aria-practices/examples/combobox/aria1.1pattern/listbox-combo.html or https://demos.telerik.com/kendo-ui/dropdownlist/index.
Maybe more relevant:
https://www.w3.org/TR/wai-aria-1.1/#valuetype_true-false-undefined
There was a problem hiding this comment.
As I said: The original APG section unsets it and the regarding issue is not resolved. Same goes for the issue what a select size="1" should be mapped to (listbox vs combobox).
| <Select aria-describedby={helperTextId} value={value} input={InputElement} {...SelectProps}> | ||
| <Select | ||
| aria-describedby={helperTextId} | ||
| id={id} |
There was a problem hiding this comment.
Is this needed? I expect it to already come from the InputElement element.
There was a problem hiding this comment.
I'll add a test/try without it. I think I considered this but it might have been in another context.
There was a problem hiding this comment.
It is required. But it helped uncover another bug where the input[type="hidden"] had a (colliding) id and aria-describedby. Neither of which are required anymore.
I'll check again. I was under the assumption that this doesn't matter i.e. if the attribute is empty nothing bad happens. Conditional string concatenation is always so unwieldy so I tried to take as much shortcuts as possible |
eps1lon
force-pushed
the
fix/Select/label
branch
from
f88a550
to
12c4848
Compare
|
@eps1lon The wave chrome browser extension reports 4 issues:
Do they signal the same issue than the W3 org HTML validator? |
|
I don't know these tools. What issues are they reporting and what is the rationale for these rules? |
|
The tool is https://wave.webaim.org/, it's the one used in #17931. |
Ok I looked at the rationale and it does not apply
This was/is the point of the demo (which explicitly reads "without a label"). The issue is expected. Same for the placeholder. For accessible name computation empty attributes or incorrect references will not block other alternatives. Only valid IDREFs are considered. I can add special logic but then a legitimate issue is less obvious. One issue was a true positive though. |
|
@eps1lon What do you think of the following incentive to fix false positive? Developers use accessibility automation tools to assist them in authoring accessible interfaces. These tools have flaws and can report false positives. However, each false positive adds noise and needs to be verified. If we know that it's a false positive, we have an opportunity to solve it and void it to bug a developer that uses the component. |
I just don't have the time and fix every issue upstream, sorry. This demo was there before and it never bothered you which is why I guess this is still a low priority. |
|
@eps1lon Sounds good 👍 . |
|
@eps1lon I have recently started seeing the below issue in my code, and am trying to track down the cause. Could it be related to this patch? A Select field written as follows:
Previously when the user interacts with this field, a menu dropdown was rendered with After updating material-ui, this menu is no longer attributed with the correct id: As we use the id in several of our tests, this change has caused some breakages. Grateful for guidance on these questions:
Thanks! |
If you need an id for testing I recommend using some |
How to properly label a
Select:will create a proper
Age 20accessible name followingor a complete solution
Closes #16409
Follows https://www.w3.org/TR/wai-aria-practices/examples/listbox/listbox-collapsible.html
The previous solution did properly label the hidden input which doesn't make much sense and was partially motivated by lighthouse complaining. Either it removed this behavior or now properly recognizes the combobox.
I also simplified the demos to use a single value type. This gets rid of structures like
{ ...rest, [key]: value }or curried functions which is pretty advanced JS syntax and not appropriate for demos.Argos failure is expected. A reviewer should accept these changes.