-
-
Notifications
You must be signed in to change notification settings - Fork 32.5k
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
[Table] Add aria-label & caption in table demos #17696
[Table] Add aria-label & caption in table demos #17696
Conversation
Are aria- and caption meant to be used at the same time? Which one should be preferred? cc @ahayes91 |
The difference that I was able to find between
Reference: https://fae.disability.illinois.edu/rulesets/TABLE_2/ So i ended up using both, which I am now thinking is not the right approach maybe. What do you suggest? @oliviertassinari |
Details of bundle changes.Comparing: daadbca...4a2532a
|
If we want to include this we should provide explicit examples not just repeating the words. |
So just |
Can't give you a clear prescription here but at least something more specific than |
I'm definitely not an expert or regular user of these to be honest. I've done some testing on both options both separately and together ( I've applied CSS to visually hide the
Screenreader testing:
If you have any doc examples ready I can do some testing on those too - the tests above were performed on my own implementation of an MUI table. |
I have added an |
@ahayes91 I have not worked with accessibility that much, but does |
Quite the opposite 😄 |
What do you guys think of using, and only, using caption?
|
According to this if we are using
<p id="tblDesc">Column one has the location and size of accommodation, other columns show the type and number of properties available.</p>
<table aria-describedby="tblDesc"> So would a simple table with caption work like below; <table>
<caption>lorem ipsum lorem ipsum lorem ipsum</caption>
<!-- TABLE CONTENT -->
</table> |
caption should be visible for sighted users.
|
I have made the changes, can you kindly review again. 😄 😅 |
This comment has been minimized.
This comment has been minimized.
@adeelibr Great work so far. Sorry, I have commented too early. After more thoughts, I come to the following conclusion:
|
I honestly love it, how 4 lines of CSS can make it so pretty (still amazes me) 😄 😺 But if you don't find it elegant, I can remove the caption tag from all of the tables & only keep aria-label & add a new section for `Accessibility with 2 demos, 1 with caption & the other with aria-describedby, if that's okay? |
@ahayes91 this bit might bite us in the behind later...
This is perfect for desktop but on mobile you might end up with unexpected focusing behaviour. It might very well work fine, but worth testing before merging. |
The styling looks fine to me. What is the problem currently? Quick summary as to label, caption and description:
|
1b96120
to
f900cfd
Compare
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 updated the pull request with a new commit.
- I have added the body2 typography to the caption
- I have added a small accessibility and caption section in the documentation
- I have removed the caption from the other table demos for simplicity. I could only find a design system that shows the caption support: Bootstrap and even then, it's scoped to a single demo. I don't think that it's something developers want in the majority of the cases (outside of the morality of it). I would expect the majority to use the component to build large interactive tables.
Do you guys any concerns left to be addressed?
f900cfd
to
4a2532a
Compare
I'd argue that Captions are a fine thing either way. I hope more people would to add them to their tables since that's what you're supposed to do with your tables. Just like you label your diagrams. Or eat your vitamins 😄 |
Dead right, VoiceOver on iPad focus was going off screen when navigating to the table. Changed the CSS to use
|
@adeelibr Thank you! |
Closes #17679