Enhance screen reader support to announce column headers are interactable. #1836
Conversation
sdadn
force-pushed
the
wdg_column_resizing
branch
from
0178357 to
a4d7f99
Compare
|
@KoushikKalli @brithom Tested for accessibility. Looks good, great job! Updating the label to a11y reviewed. |
chrismichalewicz
added
⭐ Accessibility Reviewed
and removed
Accessibility Review Required
labels
| const onBlur = (event) => { | ||
| if (!event.currentTarget.contains(event.relatedTarget)) { | ||
| setGridHasFocus(false); | ||
| } | ||
| }; |
There was a problem hiding this comment.
We should not need to add new event handler logic to determine if the grid has focus.
There was a problem hiding this comment.
When the grid loses focus, we want the columnHeaderCell that had focus to reflect the correct state.
There was a problem hiding this comment.
But the column header cell would lose focus itself. We should not need a state update because the CSS is based on focus, right?
There was a problem hiding this comment.
The columnHeaderCell's active state is set via a prop so if we do not do this here, the columnHeaderCell would have lost focus but its active state would remain true. The result is that when focus comes back into the grid, the columnHeaderCell will not get re-rendered thus causing incorrect screen reader behavior because focus is not correctly set to the button.
| @@ -584,12 +585,19 @@ const DataGrid = injectIntl((props) => { | |||
| // Not triggered when swapping focus between children | |||
| if (handleFocus.current) { | |||
| setFocusedRowCol(focusedRow, focusedCol, true); | |||
| setGridHasFocus(true); | |||
There was a problem hiding this comment.
I would think that we could determine this information without needing to alter the focus event handler logic.
There was a problem hiding this comment.
This is where this logic should live. When the grid is initially rendered, nothing on the grid has focus. All the columnHeaderCells would be rendered with isActive=false. When the grid gets focus, since nothing is getting re-rendered, the state of the columnHeaderCell that got focus is incorrect. This ensures that when the grid gets focus the columnHeaderCell with the focus gets updated correctly.
|
|
||
| const [isResizeHandleActive, setResizeHandleActive] = useState(false); | ||
|
|
||
| const columnHeaderFocusArea = useCallback(() => (columnHeaderCellButtonRef.current ? columnHeaderCellButtonRef.current : columnHeaderCellRef.current), []); |
There was a problem hiding this comment.
Do we really need a useCallback here? This should really just be a simple or statement to obtain this value, when needed. I could maybe see it being a ref variable. It just needs to be set at the proper time.
There was a problem hiding this comment.
Based on where it is called from, the useCallback is required. We did not want to do the same or statement in multiple places.
There was a problem hiding this comment.
Why can't we just use a ref variable?
There was a problem hiding this comment.
So is your concern about the use of a function vs a variable. There is no reason we could not have used a ref but we used a function. Does it really matter (asking for a friend ;-))?
| <div className={cx('header-container')} role={displayName && 'button'}> | ||
| <div | ||
| className={cx('header-container')} | ||
| {...isSelectable && displayName && { ref: columnHeaderCellButtonRef, tabIndex: '-1', role: 'button' }} |
There was a problem hiding this comment.
This won't work properly for the Terra Table. In that scenario, the tabIndex would be 0. Also, should the cell tabIndex be updated when this one is set? Would the cell ever have focus in that use case?
There was a problem hiding this comment.
I think that if refactoring is needed for table then it should be done at that level. We have added the requisite tests to ensure that the grid functionality is preserved if such refactoring becomes necessary. The cell has a tabIndex of -1 and I do not think that needs to change.
There was a problem hiding this comment.
We might be able to get away with it since the button is taking up most of the area. Ideally, the cell itself would not get focus when there is a button.
There was a problem hiding this comment.
You are correct. The cell will not get focus when there is a button.
sdadn
changed the title
Summary
Added additional screen reader support to announce that column headers are interactable upon selection by giving focus to the button control that allows column headers to be selected.
What was changed:
Focus of a column header cell is changed from the cell to the button control inside the cell that allows selection.
Why it was changed:
To ensure screen readers announce that column headers are interactable upon selection.
Testing
This change was tested using:
Reviews
In addition to engineering reviews, this PR needs:
This PR resolves: UXPLATFORM-9611