Repository navigation
feat: structural accessibility for data table - #5082
oleksandrzavarzin-callstack wants to merge 14 commits into
Conversation
| const publish = useLatestCallback(() => setHeaderLabels?.(labels)); | ||
| const signature = labels.join(' '); | ||
|
|
||
| React.useEffect(publish, [publish, signature]); |
There was a problem hiding this comment.
Publishing the column names through an effect makes the table's ARIA indices a second-render value. Server-rendered output (the docs site is one, rspress.config.ts:122 aliases to react-native-web) gets aria-rowcount="6" instead of 7, and the header row and the first data row both get aria-rowindex="1" - duplicate row indices. DataTable already reads column position from ColumnIndexContext without an effect; can the header names go the same way?
| const label = | ||
| ariaLabel ?? (isWeb ? undefined : composeCellLabel({ columnLabel, value })); |
There was a problem hiding this comment.
Try const label = ariaLabel ?? (cellIsFocusUnit ? composeCellLabel({ columnLabel, value }) : undefined) - a cell wrapping a Checkbox currently gets aria-label="Pick", which buries the checkbox's own role and state behind the column name.
| 'aria-rowindex': | ||
| index == null ? undefined : index + 1 + (table?.hasHeader ? 1 : 0), | ||
| }), | ||
| accessible: accessible ?? (rowIsFocusUnit || undefined), |
There was a problem hiding this comment.
Try accessible: accessible ?? rowIsFocusUnit - the || undefined leaves the prop unset, which is exactly when Pressable applies its own default of true, so a pressable row swallows the Checkbox inside it on iOS.
| return child.props.index === undefined | ||
| ? React.cloneElement(child, { index: firstRowIndex + offset }) | ||
| : child; | ||
| }); |
There was a problem hiding this comment.
Try the provider pattern you already wrote for columns (DataTableColumnsContext.tsx:36-44) - cloneElement means a consumer's <NameRow /> wrapper gets no index and silently loses its aria-rowindex. It is also the React.Children problem #4954 names.
|
@JKobrynski Thank you for the review! |
| ): child is React.ReactElement<P> => | ||
| React.isValidElement(child) && | ||
| typeof child.type !== 'string' && | ||
| 'displayName' in child.type && |
There was a problem hiding this comment.
A fragment child crashes the table - <DataTable><>{rows}</></DataTable> throws
Cannot use 'in' operator to search for 'displayName' in Symbol(react.fragment) on this line, and so does a fragment inside DataTable.Row or DataTable.Header. It renders fine on main. Same family as #4710.
| // Anything else is taken for a row. Virtualized rows have no children | ||
| // here to number, so those pass `index` themselves. | ||
| return ( | ||
| <RowIndexContext.Provider value={firstRowIndex + rendered++}> |
There was a problem hiding this comment.
Wrap the rows in a View and they all get the same aria-rowindex, because every child that isn't the header or the pagination counts as one row here. A <Text> empty state inflates aria-rowcount the same way, and the spoken "row 3 of 6" with it. Could rows register themselves instead, so only real rows are counted?
There was a problem hiding this comment.
Thank you! I fixed your both moments.
The child map is now a recursive walk: rows are numbered wherever they sit, and
anything the table can see into - a fragment, a grouping View, a <Text>
empty state - is descended into rather than counted.
Not registration because a row can only claim an index by mutating shared state
during render, which double-counts under StrictMode, or in an effect - which
your DataTableHeader comment rightly ruled out for the server-rendered docs.
There was a problem hiding this comment.
Have you tried a Divider between the rows with TalkBack? I'm afraid it counts as a row, so everything after it is announced one place too far.
There was a problem hiding this comment.
Ok, I've added a divider to a skip list, the list of elements that won't be counted as a row.
| // Native has no column-header semantics, so a title that is not already a | ||
| // pressable has to opt in, or its text is absorbed by whichever ancestor | ||
| // happens to be focusable and the columns read as one run-on stop. | ||
| accessible: Platform.OS === 'web' ? undefined : true, |
There was a problem hiding this comment.
A header cell holding a select-all checkbox becomes one accessibility element on native, so the checkbox can't be reached - accessible is true here whatever the title contains. The row has a describable check for exactly this case; the title has none.
| ...rest | ||
| }: Props) => { | ||
| if ( | ||
| __DEV__ && |
There was a problem hiding this comment.
This warning throws a ReferenceError in any web bundle that doesn't define __DEV__, and it is the first use of it in src/. splitStyles.ts:26 guards with process.env.NODE_ENV !== 'production' - worth matching that?
|
@JKobrynski, your comments should be resolved now. Please check if that's true, and let me know if you have any additional issues here. |
|
|
||
| // Anything the table can see into is descended into, not counted: | ||
| // a wrapper must not make its rows one row, nor an empty state a row. | ||
| if (nested !== undefined) { |
There was a problem hiding this comment.
A row component that takes children is now looked through instead of counted, so
<MyRow>{item.name}</MyRow> loses its aria-rowindex and the table under-counts. One using React.Children.only throws outright, where it renders fine on main. Both worked before this commit - since registration is out, is there another way to tell a grouping wrapper from a row component?
There was a problem hiding this comment.
Thank you for pointing on it. The check is now the element's type plus what the table can see inside it.
The walk descends only into a type it sees through (Fragment, View, ScrollView, Text) or an element with a Header, Row or Pagination somewhere inside. Anything else counts as one row, as on main, so <MyRow> keeps its aria-rowindex.
Still one case, like on main: a wrapper whose rows the table can't see counts as one row. In this case index on the row is the way out. I've noted it in the migration doc.
| (typeof type === 'function' || | ||
| (typeof type === 'object' && type !== null)) && | ||
| 'displayName' in type && | ||
| (type as { displayName?: unknown }).displayName === displayName |
There was a problem hiding this comment.
Do we really need to use as here?
There was a problem hiding this comment.
No, I've removed the cast.
| if (isTransparentContainer(child) || containsTableStructure(nested)) { | ||
| return nested === undefined | ||
| ? child | ||
| : React.cloneElement( |
There was a problem hiding this comment.
Any reason to rebuild every consumer's element here .github/instructions/code.instructions.md asks to avoid cloneElement and React.Children.
There was a problem hiding this comment.
Oh, sorry. I haven't seen these instructions. I removed the cloning alltogether.
And now:
Fragmentis flatten.Textis left alone.- A
Viewor a consumer wrapper is now rendered as written. A row that ends up with no position warns in dev. The migration guide notes the change.
There was a problem hiding this comment.
Have you tried rows inside a View with VoiceOver now? I'm afraid they're still counted in the total but announce no position at all.
There was a problem hiding this comment.
Unfortunately, I don't see a way to automatically insert an index without injecting it via cloneElement or similar implementations. That's why I've added a note to the migration guide and a warning about manual position inclusion (in dev mode). So the result code could look something like that:
<DataTable>
<DataTable.Header>…</DataTable.Header>
{pinned.map((item) => (
<DataTable.Row key={item.key}>…</DataTable.Row>
))}
<ScrollView>
{/* the consumer adds the offset */}
{items.map((item, i) => (
<DataTable.Row key={item.key} index={pinned.length + i}>…</DataTable.Row>
))}
</ScrollView>
</DataTable>…essibility-for-DataTable
…essibility-for-DataTable
satya164
left a comment
There was a problem hiding this comment.
Review by Claude
See the inline comments. (via Claude, verify if it makes sense)
| React.Children.map(nodes, (child) => { | ||
| if (!React.isValidElement<{ children?: React.ReactNode }>(child)) { | ||
| return child; | ||
| } | ||
|
|
||
| // Read before the guards below narrow `child` to `never`. | ||
| const { | ||
| type, | ||
| props: { children: nested }, | ||
| } = child; | ||
|
|
||
| if ( | ||
| isDataTableElement<DataTableHeaderProps>(child, 'DataTable.Header') | ||
| ) { | ||
| hasHeader = true; | ||
| columnLabels = readColumnLabels(nested); | ||
| return child; | ||
| } | ||
|
|
||
| if (isDataTableElement(child, 'DataTable.Pagination')) { | ||
| return child; | ||
| } | ||
|
|
||
| if (isDataTableElement(child, 'DataTable.Row')) { | ||
| return number(child); | ||
| } | ||
|
|
||
| if (isNeverRow(child)) { | ||
| return child; | ||
| } | ||
|
|
||
| if (type === React.Fragment) { | ||
| return walk(nested); | ||
| } | ||
|
|
||
| if (isGroupingContainer(child) || containsTableStructure(nested)) { | ||
| void walk(nested); | ||
| return child; | ||
| } | ||
|
|
||
| // What is left renders out of sight - a row component of the | ||
| // consumer's own, most often - so it counts as one row. A row the | ||
| // table never sees at all, as a virtualized list renders, passes | ||
| // `index` itself. | ||
| return number(child); |
There was a problem hiding this comment.
The cloning is gone, but the table still walks its children with React.Children.map and decides what each element is from its type and displayName. The project rules ask to avoid this, and it breaks as soon as a consumer wraps a part of the table in a component of their own:
const MyHeader = () => <DataTable.Header>...</DataTable.Header>is counted as a data row. On iOS the only row reads "Frozen yogurt, 159, row 2 of 2" and has no column names, because the header is never found.const MyPagination = () => <DataTable.Pagination ... />is counted as a row too. One data row reads "row 1 of 2", andaria-rowcounton web is 3.
Both render fine and the dev warning does not fire, so nothing tells the consumer. Could the table take this from explicit props instead? rowCount on the table and index on the row already exist, and a hasHeader-style prop or a label on each columns entry can cover the rest through context. (via Claude, verify if it makes sense)
| export const readColumnLabels = ( | ||
| children: React.ReactNode | ||
| ): Array<string | undefined> => { | ||
| const labels: Array<string | undefined> = []; | ||
|
|
||
| React.Children.forEach(children, (child, position) => { | ||
| labels[position] = isDataTableElement<LabelledProps>( | ||
| child, | ||
| 'DataTable.Title' | ||
| ) | ||
| ? getElementLabel(child.props) | ||
| : undefined; | ||
| }); | ||
|
|
||
| return labels; | ||
| }; |
There was a problem hiding this comment.
Column names come from reading DataTable.Title elements by displayName, so a title in a wrapper loses its name. With const SortTitle = (props) => <DataTable.Title {...props} /> as the first column, the row reads "Frozen yogurt, Calories, 159, row 1 of 1" and the "Dessert" column name is gone. DataTableRow has the same problem at line 145: it reads child.props of each cell, so a wrapped DataTable.Cell makes the row fall back to per-cell focus.
Since columns is already the single place for column data, a label field on DataTableColumn, read through ColumnsContext, would give cells their column name without reading elements. (via Claude, verify if it makes sense)
| export const withColumnIndices = (children: React.ReactNode) => | ||
| React.Children.map(children, (child, index) => | ||
| child == null ? ( | ||
| child | ||
| ) : ( | ||
| <ColumnIndexContext.Provider value={index}> | ||
| {child} | ||
| </ColumnIndexContext.Provider> | ||
| ) | ||
| ); |
There was a problem hiding this comment.
React.Children.map does not look inside fragments, so cells given in a fragment all get index 0:
<DataTable.Row>
<>
<DataTable.Cell>Frozen yogurt</DataTable.Cell>
<DataTable.Cell>159</DataTable.Cell>
</>
</DataTable.Row>With the example's columns, the second cell gets flex: 2 and start alignment from column 0. Both cells get aria-colindex="1" on web, and on iOS both are named after the first column ("Dessert, 159"). The cell's column key avoids this, so resolving cells by that key when columns is given would avoid both the positional guess and React.Children. (via Claude, verify if it makes sense)
| textAlign: | ||
| align === 'center' ? 'center' : isEnd === isRTL ? 'left' : 'right', |
There was a problem hiding this comment.
This flips the alignment twice on iOS and Android. React Native already mirrors left and right in RTL layouts there. Wrapped start-aligned text ends up on the left in RTL, and wrapped end-aligned text on the right. Only the web needs the mapping:
// iOS and Android mirror left and right in RTL layouts. The web does not.
const mirror = Platform.OS === 'web' && isRTL;
textAlign: align === 'center' ? 'center' : isEnd !== mirror ? 'right' : 'left',The test "mirrors text alignment in right-to-left layouts" runs as iOS and currently asserts the double flip, so it needs to change with this. (via Claude, verify if it makes sense)
| const rotation = useSharedValue(sortDirection === 'ascending' ? 0 : 180); | ||
| const isFirstRender = React.useRef(true); | ||
|
|
||
| React.useEffect(() => { | ||
| spin.value = withTiming(sortDirection === 'ascending' ? 0 : 180, { | ||
| duration: 150, | ||
| easing: Easing.inOut(Easing.ease), | ||
| const { duration, easing } = theme.motion; | ||
|
|
||
| const timingConfig = React.useMemo( | ||
| () => ({ | ||
| duration: duration.short3, | ||
| easing: Easing.bezier(...easing.standard), | ||
| reduceMotion: reduceMotion ? ReduceMotion.Always : ReduceMotion.Never, | ||
| }); | ||
| }, [reduceMotion, sortDirection, spin]); | ||
| }), | ||
| [duration.short3, easing.standard, reduceMotion] | ||
| ); | ||
|
|
||
| const animatedStyle = useAnimatedStyle(() => ({ | ||
| transform: [{ rotate: `${spin.value}deg` }], | ||
| })); | ||
| React.useEffect(() => { | ||
| if (isFirstRender.current) { | ||
| isFirstRender.current = false; | ||
| return; | ||
| } | ||
|
|
||
| rotation.value = withTiming( | ||
| sortDirection === 'ascending' ? 0 : 180, | ||
| timingConfig | ||
| ); | ||
| }, [sortDirection, rotation, timingConfig]); |
There was a problem hiding this comment.
The arrow rotation is a transition between two states driven by sortDirection, so it fits Reanimated CSS transitions, as in Badge and Checkbox. That also removes the shared value, the effect and the isFirstRender ref:
<Animated.View
style={[
styles.icon,
{
transform: [{ rotate: sortDirection === 'ascending' ? '0deg' : '180deg' }],
transitionProperty: 'transform',
transitionDuration: reduceMotion ? 0 : duration.short3,
transitionTimingFunction: cubicBezier(...easing.standard),
},
]}
>
``` (via Claude, verify if it makes sense)| const role = | ||
| Platform.OS === 'web' | ||
| ? ('columnheader' as const) | ||
| : onPress | ||
| ? ('button' as const) | ||
| : undefined; |
There was a problem hiding this comment.
On web a sortable title is a focusable div with role="columnheader". Screen readers announce it as a header, not as something to activate, and Space does nothing. Only Enter toggles the sort, because React Native Web activates on Space only for button roles. The ARIA sortable table pattern keeps columnheader on the container and puts a button inside it. Rendering a View with role="columnheader" around a Pressable with role="button" on web would fix both. (via Claude, verify if it makes sense)
| role: | ||
| Platform.OS === 'web' || !interactive | ||
| ? ('row' as const) | ||
| : ('button' as const), |
There was a problem hiding this comment.
A pressable row on web is a focusable role="row", so it has no activatable role and Space does not press it (only Enter does). A row in a table cannot also be a button. Interactive rows need either the grid pattern or a control inside a cell. (via Claude, verify if it makes sense)
| <DataTable columns={columns}> | ||
| <DataTable.Title>Dessert</DataTable.Title> | ||
| <DataTable.Cell>{item.name}</DataTable.Cell> | ||
| </DataTable> |
There was a problem hiding this comment.
This example puts DataTable.Title and DataTable.Cell directly in DataTable. With this PR the title is then counted as a row. A complete structure shows the change better:
| <DataTable columns={columns}> | |
| <DataTable.Title>Dessert</DataTable.Title> | |
| <DataTable.Cell>{item.name}</DataTable.Cell> | |
| </DataTable> | |
| <DataTable columns={columns}> | |
| <DataTable.Header> | |
| <DataTable.Title>Dessert</DataTable.Title> | |
| </DataTable.Header> | |
| <DataTable.Row> | |
| <DataTable.Cell>{item.name}</DataTable.Cell> | |
| </DataTable.Row> | |
| </DataTable> | |
| ``` (via Claude, verify if it makes sense) |



Motivation
DataTable's structure was purely visual: bareViews andTouchableRipples with no roles, indices or header relationships, so a screen reader conveyed no table context at all. A user heard"159"with no way to know it was the Calories column of row 3.Changes
role="table"witharia-rowcount/aria-colcount,role="row"witharia-rowindex,role="columnheader"witharia-sort,role="cell"witharia-colindex."Dessert, Frozen yogurt, Calories, 159, row 3 of 6". It falls back to per-cell focus when a row holds interactive or non-text content, so nothing becomes unreachable.nativeFocusModeoverrides the choice.View.aria-sorton web, folded into the accessible name on native, with an announcement on change since focus stays on the header. NewsortAccessibilityLabelslocalizes the wording.Animated, matchingSwitchandCheckbox. Reduced motion is suppressed viaReduceMotion.Alwaysinstead of a manual snap, and150msbecomestheme.motion.duration.short3(same value).labelsprop. Data-dependent values are functions, so pluralization stays with the app's own i18n.columnsdescribes width and alignment once instead of repeatingstyleon a title and every cell. Newaligncomposes withnumeric, which is unchanged and not deprecated.numberOfLinesis honoured exactly at every font scale; the default is one line at the default scale and unclamped above it.surfaceVarianttooutlineVariant, matchingDivider.Buttongainsaria-expanded, needed by the rows-per-page anchor which never reported that it opens a menu.Breaking changes
Rows, cells and titles without a touch handler render a plain
View. They lose the ripple, the web hover background and keyboard focus, and no longer report a disabled state.Other important changes:
rowCountandfirstRowIndexfor correct positions when paginating.numericnow applies tabular figures, the one visual change to existing tables.Related issue
Child of the "Modernize non-standard components" effort; no issue exists yet.
Test plan
yarn typescript,yarn lintcleanyarn test806 passed, 167 snapshots. The 2828-line snapshot is replaced with 8 small per-component snapshots plus named contract assertions, since the old blob is where thearia-disabledregression sat unnoticed.aria-sortcycling, and noaria-disabledon any row, cell or title.adb shell uiautomator dump. Tapping a row focuses that row, each header is its own stop, and a row containing an interactive element (e.g. Checkbox) falls back to per-cell focus.Screenshots: