-
Notifications
You must be signed in to change notification settings - Fork 72
Enhance screen reader support to announce column headers are interactable. #1836
Changes from 71 commits
4586b1b
61e616e
41d5697
f0c547b
d0e71e5
f83a0d8
d2c2959
0cc9d6e
6356750
c64ad23
0dad843
5568179
b727ebe
2a4ceeb
ff48e99
1b15e28
78b23ed
a4fd285
835c69d
bc74a4a
e2d3d3b
4de9bae
c949b32
82615a0
526698c
04a321e
010c3f9
ca10df4
9569340
52c788e
ce7f8ea
fb573e5
bc4901e
a3320ed
db90402
d5c6ea0
256a9f5
e1b6ddf
b28d215
7f5cf5d
c4b58f1
955a02e
698d130
b634939
48a60cc
e4be9b6
84f17e0
30e3985
e5fd211
6e66593
281cf72
74fb315
5868be5
cb29ed1
58ba111
aac35bf
9340a4c
b81bf75
0fc61be
7cd98b5
f74b948
00353ed
670288f
e149d55
88581fc
a2e12e1
96e9da9
7de1a76
0a74744
4fa2502
016ab65
ad6ad83
15bc0e1
fe7af39
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -189,6 +189,7 @@ const DataGrid = injectIntl((props) => { | |
const [checkResizable, setCheckResizable] = useState(false); | ||
const [focusedRow, setFocusedRow] = useState(0); | ||
const [focusedCol, setFocusedCol] = useState(0); | ||
const [gridHasFocus, setGridHasFocus] = useState(false); | ||
|
||
// Aria live region message management | ||
const [columnHeaderAriaLiveMessage, setColumnHeaderAriaLiveMessage] = useState(null); | ||
|
@@ -584,12 +585,19 @@ const DataGrid = injectIntl((props) => { | |
// Not triggered when swapping focus between children | ||
if (handleFocus.current) { | ||
setFocusedRowCol(focusedRow, focusedCol, true); | ||
setGridHasFocus(true); | ||
} | ||
} | ||
|
||
handleFocus.current = true; | ||
}; | ||
|
||
const onBlur = (event) => { | ||
if (!event.currentTarget.contains(event.relatedTarget)) { | ||
setGridHasFocus(false); | ||
} | ||
}; | ||
Comment on lines
+595
to
+599
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should not need to add new event handler logic to determine if the grid has focus. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When the grid loses focus, we want the columnHeaderCell that had focus to reflect the correct state. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
||
// ------------------------------------- | ||
|
||
return ( | ||
|
@@ -603,6 +611,7 @@ const DataGrid = injectIntl((props) => { | |
className={cx('data-grid', theme.className)} | ||
onKeyDown={handleKeyDown} | ||
onFocus={onFocus} | ||
onBlur={onBlur} | ||
onMouseDown={onMouseDown} | ||
tabIndex={0} | ||
{...(activeIndex != null && { onMouseUp, onMouseMove, onMouseLeave: onMouseUp })} | ||
|
@@ -614,7 +623,7 @@ const DataGrid = injectIntl((props) => { | |
columns={dataGridColumns} | ||
headerHeight={columnHeaderHeight} | ||
tableHeight={tableHeight} | ||
activeColumnIndex={focusedRow === 0 ? focusedCol : undefined} | ||
activeColumnIndex={(gridHasFocus && focusedRow === 0) ? focusedCol : undefined} | ||
activeColumnResizing={focusedRow === 0 && checkResizable} | ||
columnResizeIncrement={columnResizeIncrement} | ||
onColumnSelect={handleColumnSelect} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -159,16 +159,24 @@ const ColumnHeaderCell = (props) => { | |
|
||
const columnContext = useContext(ColumnContext); | ||
const columnHeaderCellRef = useRef(); | ||
const columnHeaderCellButtonRef = useRef(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we maybe just call this buttonRef? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The name was kept consistent with the existing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I that name was used because it was the name of the component There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I still think that columnHeaderCellButtonRef conveys more meaning than just buttonRef. |
||
|
||
const [isResizeHandleActive, setResizeHandleActive] = useState(false); | ||
|
||
const columnHeaderFocusArea = useCallback(() => (columnHeaderCellButtonRef.current ? columnHeaderCellButtonRef.current : columnHeaderCellRef.current), []); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why can't we just use a ref variable? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 ;-))? |
||
|
||
useEffect(() => { | ||
if (isActive && isResizeActive) { | ||
setResizeHandleActive(true); | ||
if (isActive) { | ||
if (isResizable && isResizeActive) { | ||
setResizeHandleActive(true); | ||
} else { | ||
columnHeaderFocusArea().focus(); | ||
setResizeHandleActive(false); | ||
} | ||
} else { | ||
setResizeHandleActive(false); | ||
} | ||
}, [isActive, isResizeActive]); | ||
}, [columnHeaderFocusArea, isActive, isResizable, isResizeActive]); | ||
|
||
const onResizeHandleMouseDown = useCallback((event) => { | ||
if (onResizeMouseDown) { | ||
|
@@ -178,9 +186,9 @@ const ColumnHeaderCell = (props) => { | |
|
||
// Restore focus to column header after resize action is completed. | ||
const onResizeHandleMouseUp = useCallback(() => { | ||
columnHeaderCellRef.current.focus(); | ||
columnHeaderFocusArea().focus(); | ||
setResizeHandleActive(false); | ||
}, []); | ||
}, [columnHeaderFocusArea]); | ||
|
||
// Handle column header selection via the mouse click. | ||
const handleMouseDown = (event) => { | ||
|
@@ -202,7 +210,7 @@ const ColumnHeaderCell = (props) => { | |
break; | ||
case KeyCode.KEY_LEFT: | ||
if (isResizable && isResizeHandleActive) { | ||
columnHeaderCellRef.current.focus(); | ||
columnHeaderFocusArea().focus(); | ||
setResizeHandleActive(false); | ||
event.stopPropagation(); | ||
event.preventDefault(); | ||
|
@@ -258,7 +266,10 @@ const ColumnHeaderCell = (props) => { | |
onKeyDown={(isSelectable || isResizable) ? handleKeyDown : null} | ||
style={{ width: `${width}px`, height: headerHeight, left: cellLeftEdge }} // eslint-disable-line react/forbid-dom-props | ||
> | ||
<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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are correct. The cell will not get focus when there is a button. |
||
> | ||
{errorIcon} | ||
<span>{displayName}</span> | ||
{sortIndicatorIcon} | ||
|
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 would think that we could determine this information without needing to alter the focus event handler logic.
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.
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.