va-table: add caption summary and focus props#1907
Conversation
| outline: 2px solid var(--vads-color-action-focus-on-light); | ||
| outline-offset: 2px; | ||
| z-index: 2; | ||
| } No newline at end of file |
There was a problem hiding this comment.
note: I created this Sass mixin because the single stylesheet approach for focus style has become unwieldy. I am going to create a follow-up ticket for us to look at switching to using a direct mixin in the other places the original stylesheet is referencing.
| margin-bottom: 0.75rem; | ||
| margin-top: 0.75rem; | ||
| margin-left: 0.25rem; | ||
| margin-right: 0.25rem; |
There was a problem hiding this comment.
| margin-right: 0.25rem; | ||
| span { | ||
| display: block; | ||
| font-weight: normal; |
There was a problem hiding this comment.
@jamigibbs I think the styling is good. I'm not sure if that's the best placement for the results text though? Should it be closer to the pagination controls? 🤔 That might complicat focus order though. This is just a hunch, but I can do some research if necessary. Before doing that, I wonder if this be more of an accessibility question though. @amyleadem @jeana-adhoc Your thoughts?
Here's a quick mock up moving the results total to be near the pagination controls:
(It will need a little more space if we go this direction.)
There was a problem hiding this comment.
I think @jamigibbs initial placement makes sense. It's similar to how we manage search results.
After triggering a pagination change, we need to announce that change and send focus somewhere, and we wouldn't want to focus directly above the pagination.
@amyleadem Would be curious if you have other thoughts about this.
| setTimeout(() => { | ||
| const main = tableRef.current; | ||
| const vaTable = main?.querySelector('va-table'); | ||
| vaTable?.setAttribute('set-caption-focus', 'true'); |
There was a problem hiding this comment.
Howdy @department-of-veterans-affairs/platform-design-system-a11y reviewer! 👋🏼 I added a prop for focusing the caption element as-needed and it appears to be functioning as intended but my VO testing resulted in some duplicate readouts when I integrated it with this pagination example in Storybook.
Is it possible that this isn't the correct a11y approach for using pagination with va-table?
There was a problem hiding this comment.
I'll take a look - I heard it duplicated in VO too (but sometimes that just.. happens). I'm going to listen in NVDA & JAWS too, then come back and see if this might just be a VO quirk.
There was a problem hiding this comment.
Thanks @jeana-adhoc! Based on our convo, I've updated the description of the PR with a11y notes to explain our approach. Feel free to edit or add if you'd like!
| {tableTitle && <caption>{tableTitle}</caption>} | ||
| {tableTitle && <caption ref={(el) => this.captionRef = el}> | ||
| {tableTitle} | ||
| {tableTitleSummary && <span>{tableTitleSummary}</span>} |
There was a problem hiding this comment.
The dev looks good here in general but it does look like the nested span element in this caption element leads to some browsers not announcing the text content of the span on screen readers. Note that this behavior has been observed elsewhere (i.e. department-of-veterans-affairs/vets-design-system-documentation#5101).
I tested on MacOS VoiceOver on Chrome, Safari, and Edge and found that the announcement was working as expected on all but Firefox, where only the immediate child text of the caption is being read (the tableTitle value). I pulled the branch down locally and removed the nested span so that the tableTitleSummary value was a direct child of the caption element and it was being announced as expected.
I'll let the @department-of-veterans-affairs/platform-design-system-a11y reviewer advise on how to move forward with this, but just wanted to get it on the radar here.
There was a problem hiding this comment.
@RyanMunsch Thanks for catching this! I'm going to update it with sr-only text and combine it with the nested elements using aria-hidden so we don't have a double readout. Like this:
<caption ref={(el) => this.captionRef = el}>
<span class="usa-sr-only">{tableTitle}{tableTitleSummary ? ` ${tableTitleSummary}` : ''}</span>
<span aria-hidden="true">
{tableTitle}
{tableTitleSummary && <span id="summary">{tableTitleSummary}</span>}
</span>
</caption>That seems to fix it in Firefox, etc. but curious to hear your thoughts from an engineering perspective. The only other solution that I think might work is adding aria-label on the caption element but I feel like we generally try to avoid using that if possible.
There was a problem hiding this comment.
No problem, @jamigibbs. That solution looks like it should work! That's the same approach that I used in this PR and that behaved as expected across browsers, operating systems, and different screen readers according to Jeana's testing.
danbrady
left a comment
There was a problem hiding this comment.
@jamigibbs I noticed that there's a bit more top margin with this update. Is that intentional?
Original
Updated
|
Hello participants of this PR! After some discussion with @jeana-adhoc, I'm going to push an update that changes focus from the |
| text-align: left; | ||
| padding: 0 0 0.313rem; | ||
| font-weight: 700; | ||
| font-size: 1.25rem; |
@danbrady Thanks for noticing that! I cleaned up some old |
|
We are going to split out these two features into separate PRs (title summary & title focus). I'm going to convert this PR to a draft for now and here is the new PR for just the title summary prop: |






Chromatic
https://5099-table-focus--65a6e2ed2314f7b8f98609d8.chromatic.com
Description
This PR will add two new props to
va-table:tableTitleSummaryprop for adding additional context/summary to thecaption.captionelement for more complex tables and this seems like a good place to put something like pagination summary information.captionelement should only contain flow content (ie. no div, p, etc). By using a prop, we can enforce that requirement and also provide consist styling for the summary.setCaptionFocusprop for programmatically focusing thecaptionas needed.captionelement in the shadow dom along with the nature of the table component re-rending with pagination, we are hoping this will make it easier for teams to add focus to this element if it's needed.Accessibility note
After discussion with @jeana-adhoc, we tested focusing on the
tableelement instead of thecaptionelement but VO + Firefox and JAWS + Edge were not reading out the title & summary at all.Adding sr-only text to the
captionwith focus was the only way that we could reliably get the screenreaders to read that text across all browsers. The downside of this is that in some screenreader + browser combinations, the caption will be read twice but we thought that would be better than not reading it at all.Related tickets and links
related department-of-veterans-affairs/vets-design-system-documentation#5099
Screenshots
Caption with summary text and focused
Testing and review
Approvals
See the QA Checklists section below for suggested approvals. Use your best judgment if additional reviews are needed. When in doubt, request a review.
Approval groups
Add approval groups to the PR as needed:
QA checklists
Use the QA checklists below as guides, not rules. Not all checklists will apply to every PR but there could be some overlap.
In all scenarios, changes should be fully tested by the author and verified by the reviewer(s); functionality, responsiveness, etc.
✨ New Component Added
minorlabel🌱 New Component Variation Added
minorlabel🐞 Component Fix
patchlabel♿️ Component Fix - Accessibility
patchlabel🚨 Component Fix - Breaking API Change
majorlabel🔧 Component Update - Non-Breaking API Change
minorlabel📖 Storybook Update
ignore-for-releaselabel🎨 CSS-Library Update
css-librarylabel