-
-
Notifications
You must be signed in to change notification settings - Fork 31.6k
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
[TextareaAutosize] Fix missing form label #17931
Conversation
Details of bundle changes.Comparing: 702cde3...55a4f6c
|
I have created a support ticket on https://wave.webaim.org/feedback. |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have explored a couple of different approaches possible to avoid the need for a double textarea.
I can't find anything satisfactory. I have tried to inject a shadow textarea at each computation instead of permanent but it seems to trigger a layout of the whole page (even if we have position absolute :o).
Angular component caches the computation of the line-height.
What if instead we used aria-labelledby to indicate it shares labelling with the real input? The downside is we need an ID on the input. We could have the user provide one. |
What's the issue with the aria-label approach? Can a user see the second textarea? |
Nope, I was just suggesting it may be more logical to indicate "this doesn't need a label because it is part of this" rather than give it this meaningless "hidden input". In the end I don't think any of this matters to the user, we are just dealing with a standard that wasn't written for this edge case. |
@koshea Did you have a look at https://github.com/OcelotChatbot/material-ui/pull/1? |
Co-Authored-By: Olivier Tassinari <olivier.tassinari@gmail.com>
Co-Authored-By: Olivier Tassinari <olivier.tassinari@gmail.com>
@oliviertassinari I am good with your changes. The aria-hidden is unnecessary with the tabindex set. The text of the label does not really matter as it will never be read. I think this commit is worthwhile to avoid throwing the error in WAVE. 👍 I've tested in NVDA to confirm it is fine. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for testing it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A developer tool reporting an issue is no reason to change it here. We should fix this upstream because now:
- We have an additional element in the a11y tree that isn't interactive
- A non-localized label that can't be customized
Is there an actual existing issue with assistive technology?
aria-hidden is an invalid attribute on navigable elements such as form controls.
does not apply here since the element isn't navigable because it has a negative tabindex.
Yeah, I think that these are pur DX changes. I find the label description on the shadow textarea interesting because it yields some information on why it's needed for the developers. I find the fix of the errors reported by Wave interesting because remove the need for a developer to go check what's wrong with the element. @eps1lon are you worried about the changes because it creates a precedent to work around accessibility tools, that it removes an incentive for them to double-check their logic and fix upstream? |
@eps1lon my two cents is that WAVE is somewhere between a developer tool and a browser. It is almost like putting in a hack to support IE11, except with the slightest chance upstream will change their mind. Non-technical users in my industry use this tool to evaluate products for compliance (whether that is a good idea or not, it is reality), and basically if it is showing an error, they are unable to deploy the software. At the end of the day I am just going to have to patch the library with this change which is a small nuisance. If you don't want to introduce this label, would you consider adding a shadowInputProps or something to be able to alter that component without patching? |
My problem is that we don't have sufficient test coverage for assistive technology and the rationale provided here does not apply. So from my viewpoint this change is neither tested sufficiently nor justified. |
@koshea If I understand your business case, your product is a widget integrated into your customer websites (schools). You fear to not convert potential customers because they use Wave and notice an error? I have no objection for a |
@oliviertassinari yep, you got it. If @eps1lon is also open to the new prop, I will get the commit in. |
@koshea For the time being, you can perform DOM operations on the shallow element once the page mount. Thank you for raising this problem. I think that we will wait to have more user's feedback before moving forward. |
Fair enough, thanks. |
When using a multiline TextField, the popular WAVE Accessibility tool is throwing an error about the hidden "Shadow input" missing a form label. This doesn't really matter as it is aria-hidden, but I suppose the logic could be that it should be there in case that visibility were to change. It would be great to clear the error, as the tool is popular with clients testing blindly for compliance.
You can see the error here: https://hlk21.csb.app/
https://material-ui.com/components/textarea-autosize/
I was not quick to determine the best practice for how to label this invisible input, so I am hardly going to argue if you change my proposed value.