-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
fix(DataTable): announce table batch action bar #5631
Conversation
This change uses a combination of `aria-expanded`/`aria-controls` so we give the table selection checkbox a mean to show/hide batch action bar. Also does the following as suggested in carbon-design-system#5242: * Hide the batch action bar from screen reader when we are visiblly hiding it * Remove `aria-label` from the batch action bar * Remove `aria-live="polite"` from `<tbody>` Fixes carbon-design-system#5242.
Deploy preview for carbon-elements ready! Built with commit 171741f |
Deploy preview for carbon-components-react ready! Built with commit 171741f https://deploy-preview-5631--carbon-components-react.netlify.com |
4d89d82
to
e10025d
Compare
Hi @asudoh! 👋 Make sure to use the auto-assigner for PR Reviews 👍 Direct reviews can always be requested after the two are assigned. |
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.
looks good to me, the batch action bar is announced only while it is visible as expected
}) => { | ||
const selectionInputProps = { | ||
id, | ||
name, | ||
onClick: onSelect, | ||
checked, | ||
disabled, | ||
'aria-expanded': checked, |
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.
Just wondering if this should be aria-checked
, rather than aria-expanded
since this seems to be on the checkbox in DataTable with Selection
?
Looking into it further, I see that this is also used in DataTable with Batch Actions
to convey the fact that the data table toolbar is now "expanded". Seems like this could be confusing, since the row itself isn't expanding, it's the table toolbar that is appearing.
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.
@tw15egan Thank you for asking - I can see that aria-expanded
for a checkbox may be confusing. This approach does below trick discussed in #5242 wrt how to tell screen readers that the checkbox expands/collapses batch action bar:
From @asudoh:
Does ARIA have a notion of "contextual action bar", where an action bar appears only in certain context?
From @carmacleod
ARIA can be used to indicate that an input like a button or checkbox controls the visibility of a container element. See the spec for aria-expanded. So if each checkbox has aria-expanded="true/false" and uses aria-controls to point to the id of the section element, then that should be helpful.
Does this make sense? Please don't hesitate to ping me if you have any questions on this. Thanks!
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.
Makes sense, thanks for the clarification 👍
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.
Is there a link where I can test this? Preferably one where there's more than one row? Neither of the above preview links seem to go to a table component page. I just want to make sure the aria-expanded markup isn't confusing with multiple checkboxes, because even though it follows the spec, it is a bit unorthodox.
Does the toolbar show when any one checkbox is checked, and hide when all checkboxes are unchecked? If so, the logic for aria-expanded might be a bit tricky.
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.
@carmacleod should be able to view it here: https://deploy-preview-5631--carbon-components-react.netlify.com/?path=/story/datatable--with-selection
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.
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.
👍 ✅
Please wait on merging this. Now that I am testing this with a multi-row table, I do think it could be pretty confusing, and I want to try to find a better way. Interesting that there are actually 2 toolbars - the initial example only had the batch toolbar. ARIA may not be able to help a lot here, because I think the correct semantics are for the table to say that it has a popup non-modal dialog, but that particular pattern is not robustly defined in the spec, and it's not consistently implemented in screen readers. So we may have to fall back on aria-live, but not quite the way it's currently being used. I will get back to you tomorrow. |
Hey there! Going through the PR backlog today and am clearing out PRs that have been up for a while 👀 |
@joshblack @asudoh I think the simplest thing at this point would be to remove the aria-expanded and aria-controls from the checkboxes, and instead, have an offscreen live region that simply says "Toolbar visible" as soon as the toolbar is visible, and "Toolbar hidden" when it is hidden again. The offscreen live region could look something like this:
Note that you need to have the status region in the DOM well before you add the span containing the text. The easiest is probably to have it always available, but offscreen. When you switch status messages, remove the whole span and then add the new one (instead of just changing the text content of the span). Let me know if this makes sense, and when a preview is ready I can try it out with a few screen readers. They may not all work perfectly, but I think it's the best we can do with this "hide the toolbar when no items are checked/show the toolbar when items are checked" design. |
This change uses a combination of
aria-expanded
/aria-controls
so we give the table selection checkbox a mean to show/hide batch action bar.Also does the following as suggested in #5242:
hiding it
aria-label
from the batch action bararia-live="polite"
from<tbody>
Fixes #5242.
Changelog
New
aria-expand
/aria-controls
combination that connects table selection checkbox and table batch action bar.Changed
Changes to:
aria-label
from the batch action bararia-live="polite"
from<tbody>
Testing / Reviewing
Testing should make sure our data table is not broken.