-
Notifications
You must be signed in to change notification settings - Fork 1.3k
fix: GridList section accessibility updates #8790
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
base: main
Are you sure you want to change the base?
Changes from 8 commits
1269d85
56ddffa
84a3a20
dd71e54
ff9e891
8c8841e
ea5b2f7
ca4369d
75c3c4f
22c3785
50b7322
2c5afba
f3d5ef2
d0be14d
63097a6
1d0adef
e6e76d3
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 | ||
---|---|---|---|---|
|
@@ -11,6 +11,7 @@ | |||
*/ | ||||
|
||||
import {chain, getScrollParent, mergeProps, scrollIntoViewport, useSlotId, useSyntheticLinkProps} from '@react-aria/utils'; | ||||
import {CollectionNode} from '@react-aria/collections'; | ||||
import {DOMAttributes, FocusableElement, Key, RefObject, Node as RSNode} from '@react-types/shared'; | ||||
import {focusSafely, getFocusableTreeWalker} from '@react-aria/focus'; | ||||
import {getRowId, listMap} from './utils'; | ||||
|
@@ -277,6 +278,35 @@ export function useGridListItem<T>(props: AriaGridListItemOptions, state: ListSt | |||
// }); | ||||
// } | ||||
|
||||
let sumOfNodes = (node: CollectionNode<T>): number => { | ||||
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. can you add a code comment to this function? It seems to be summing from a few different places, so I'm not entirely following the usage 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. yep sure, i can add some comments to clarify how it works it's not recursive so that it can handle things like nesting of sections inside sections (tbh, im not even sure if that's supported/would work) but it just allows you to jump around to nodes more easily so that we don't have to go through each individual item, header, and section node. for useGridListItem, if we start inside of a section, we jump up to the parent node (aka the section the item is contained in), and then go through each section node or individual item node that are outside of sections. it's the same for useGridListSection, except that the node won't ever be inside a section because it is the section node itself. and then again, similar logic, we go through each section node or individual item nodes that are outside of sections. it might be more helpful to draw a diagram to explain how it works so i'll see if i can draw one up... |
||||
if (node.prevKey === null) { | ||||
if (node.type === 'section') { | ||||
let lastChild = node.lastChildKey ? state.collection.getItem(node.lastChildKey) : null | ||||
return lastChild ? lastChild.index + 1 : 0; | ||||
} else if (node.type === 'item') { | ||||
return 1; | ||||
} | ||||
return 0; | ||||
} | ||||
|
||||
let parentNode = node.parentKey ? state.collection.getItem(node.parentKey) as CollectionNode<T> : null; | ||||
if (parentNode && parentNode.type === 'section') { | ||||
return sumOfNodes(parentNode); | ||||
} | ||||
|
||||
let prevNode = state.collection.getItem(node.prevKey!) as CollectionNode<T>; | ||||
if (prevNode) { | ||||
if (node.type === 'section') { | ||||
let lastChild = node.lastChildKey ? state.collection.getItem(node.lastChildKey) : null | ||||
return lastChild ? sumOfNodes(prevNode) + lastChild.index + 1 : 0; | ||||
} | ||||
|
||||
return sumOfNodes(prevNode) + 1; | ||||
} | ||||
|
||||
return 0; | ||||
}; | ||||
|
||||
let rowProps: DOMAttributes = mergeProps(itemProps, linkProps, { | ||||
role: 'row', | ||||
onKeyDownCapture, | ||||
|
@@ -293,8 +323,25 @@ export function useGridListItem<T>(props: AriaGridListItemOptions, state: ListSt | |||
if (isVirtualized) { | ||||
let {collection} = state; | ||||
let nodes = [...collection]; | ||||
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 means every grid item has to make a copy of the collection (O)n^2, can we do this at the useGrid level and send it ( 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. yeah we could add a new prop to 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 check here isn't if the node is inside a section, we do that below without needing to make a copy of the collection:
This line here:
just checks if there are any sections in the collection, and it does it for every item, and it copies the collection every time as well. Which, for the worst case (no sections) would actually be 2 complete iterations. I propose simplifying by doing this check for
We could also instead add it as a tracked property here https://github.com/adobe/react-spectrum/blob/main/packages/%40react-stately/list/src/ListCollection.ts and update it when the collection builds, then we could just ask the collection if it has sections and we could skip copying and iterating over the entire collection. 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. ohhhh right right, sorry i see what you mean |
||||
// TODO: refactor ListCollection to store an absolute index of a node's position? | ||||
rowProps['aria-rowindex'] = nodes.find(node => node.type === 'section') ? [...collection.getKeys()].filter((key) => collection.getItem(key)?.type !== 'section').findIndex((key) => key === node.key) + 1 : node.index + 1; | ||||
// TODO: refactor BaseCollection to store an absolute index of a node's position? | ||||
if (nodes.find(node => node.type === 'section')) { | ||||
yihuiliao marked this conversation as resolved.
Show resolved
Hide resolved
|
||||
let parentNode = node.parentKey ? state.collection.getItem(node.parentKey) as CollectionNode<T> : null; | ||||
let isInSection = parentNode && parentNode.type === 'section'; | ||||
let lastChildKey = parentNode?.lastChildKey; | ||||
if (isInSection && lastChildKey) { | ||||
let lastChild = state.collection.getItem(lastChildKey); | ||||
let diff = lastChild ? lastChild.index - node.index : 0; | ||||
yihuiliao marked this conversation as resolved.
Show resolved
Hide resolved
|
||||
if (parentNode!.prevKey) { | ||||
rowProps['aria-rowindex'] = sumOfNodes(parentNode!) - diff; | ||||
yihuiliao marked this conversation as resolved.
Show resolved
Hide resolved
|
||||
} else { | ||||
rowProps['aria-rowindex'] = lastChild ? lastChild.index - diff + 1 : 0; | ||||
} | ||||
} else { | ||||
rowProps['aria-rowindex'] = sumOfNodes(node as CollectionNode<T>); | ||||
} | ||||
} else { | ||||
rowProps['aria-rowindex'] = node.index + 1; | ||||
} | ||||
} | ||||
|
||||
let gridCellProps = { | ||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,13 +10,18 @@ | |
* governing permissions and limitations under the License. | ||
*/ | ||
|
||
import {DOMAttributes, RefObject} from '@react-types/shared'; | ||
import {CollectionNode} from '@react-aria/collections'; | ||
import {DOMAttributes, RefObject, Node as RSNode} from '@react-types/shared'; | ||
import type {ListState} from '@react-stately/list'; | ||
import {useLabels, useSlotId} from '@react-aria/utils'; | ||
|
||
export interface AriaGridListSectionProps { | ||
/** An accessibility label for the section. Required if `heading` is not present. */ | ||
'aria-label'?: string | ||
'aria-label'?: string, | ||
/** An object representing the section. */ | ||
node: RSNode<unknown>, | ||
/** Whether the list row is contained in a virtual scroller. */ | ||
isVirtualized?: boolean | ||
} | ||
|
||
export interface GridListSectionAria { | ||
|
@@ -37,20 +42,62 @@ export interface GridListSectionAria { | |
*/ | ||
// eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
export function useGridListSection<T>(props: AriaGridListSectionProps, state: ListState<T>, ref: RefObject<HTMLElement | null>): GridListSectionAria { | ||
let {'aria-label': ariaLabel} = props; | ||
let {'aria-label': ariaLabel, node, isVirtualized} = props; | ||
let headingId = useSlotId(); | ||
let labelProps = useLabels({ | ||
'aria-label': ariaLabel, | ||
'aria-labelledby': headingId | ||
}); | ||
let rowIndex; | ||
|
||
let sumOfNodes = (node: RSNode<unknown>): number => { | ||
snowystinger marked this conversation as resolved.
Show resolved
Hide resolved
yihuiliao marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if (node.prevKey === null) { | ||
let lastChildKey = (node as CollectionNode<T>).lastChildKey; | ||
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. if we want to use |
||
if (node.type === 'section' && lastChildKey) { | ||
let lastChild = state.collection.getItem(lastChildKey); | ||
return lastChild ? lastChild.index + 1 : 0; | ||
} else if (node.type === 'item') { | ||
return 1; | ||
} | ||
return 0; | ||
} | ||
|
||
let prevNode = state.collection.getItem(node.prevKey!); | ||
if (prevNode) { | ||
if (node.type === 'item') { | ||
return sumOfNodes(prevNode) + 1; | ||
} | ||
|
||
let lastChildKey = (node as CollectionNode<T>).lastChildKey; | ||
if (lastChildKey) { | ||
let lastChild = state.collection.getItem(lastChildKey); | ||
return lastChild ? sumOfNodes(prevNode) + lastChild.index + 1 : sumOfNodes(prevNode); | ||
} | ||
} | ||
|
||
return 0; | ||
}; | ||
|
||
if (isVirtualized) { | ||
if (node.prevKey) { | ||
let prevNode = state.collection.getItem(node.prevKey); | ||
if (prevNode) { | ||
rowIndex = sumOfNodes(prevNode) + 1; | ||
} | ||
} else { | ||
rowIndex = 1; | ||
} | ||
} | ||
|
||
return { | ||
rowProps: { | ||
role: 'row' | ||
role: 'row', | ||
'aria-rowindex': rowIndex | ||
}, | ||
rowHeaderProps: { | ||
id: headingId, | ||
role: 'rowheader' | ||
role: 'rowheader', | ||
'aria-colindex': 1 | ||
}, | ||
rowGroupProps: { | ||
role: 'rowgroup', | ||
|
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 Accessibility checker within Storybook is throwing errors for an invalid aria role on the See: https://w3c.github.io/html-aria/#el-header and https://w3c.github.io/html-aria/#el-section regarding the roles permitted on |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,7 +21,6 @@ import {DraggableCollectionState, DroppableCollectionState, Collection as IColle | |
import {FieldInputContext, SelectableCollectionContext} from './context'; | ||
import {filterDOMProps, inertValue, LoadMoreSentinelProps, useLoadMoreSentinel, useObjectRef} from '@react-aria/utils'; | ||
import {forwardRefType, GlobalDOMAttributes, HoverEvents, Key, LinkDOMProps, PressEvents, RefObject} from '@react-types/shared'; | ||
import {HeaderContext} from './Header'; | ||
import {ListStateContext} from './ListBox'; | ||
import React, {createContext, ForwardedRef, forwardRef, HTMLAttributes, JSX, ReactNode, useContext, useEffect, useMemo, useRef} from 'react'; | ||
import {TextContext} from './Text'; | ||
|
@@ -580,13 +579,15 @@ export interface GridListSectionProps<T> extends SectionProps<T> {} | |
/** | ||
* A GridListSection represents a section within a GridList. | ||
*/ | ||
export const GridListSection = /*#__PURE__*/ createBranchComponent(SectionNode, <T extends object>(props: GridListSectionProps<T>, ref: ForwardedRef<HTMLElement>, item: Node<T>) => { | ||
export const GridListSection = /*#__PURE__*/ createBranchComponent(SectionNode, <T extends object>(props: GridListSectionProps<T>, ref: ForwardedRef<HTMLDivElement>, item: Node<T>) => { | ||
let state = useContext(ListStateContext)!; | ||
let {CollectionBranch} = useContext(CollectionRendererContext); | ||
let {CollectionBranch, isVirtualized} = useContext(CollectionRendererContext); | ||
let headingRef = useRef(null); | ||
ref = useObjectRef<HTMLElement>(ref); | ||
ref = useObjectRef<HTMLDivElement>(ref); | ||
let {rowHeaderProps, rowProps, rowGroupProps} = useGridListSection({ | ||
'aria-label': props['aria-label'] ?? undefined | ||
'aria-label': props['aria-label'] ?? undefined, | ||
node: item, | ||
isVirtualized | ||
}, state, ref); | ||
let renderProps = useRenderProps({ | ||
defaultClassName: 'react-aria-GridListSection', | ||
|
@@ -599,33 +600,35 @@ export const GridListSection = /*#__PURE__*/ createBranchComponent(SectionNode, | |
delete DOMProps.id; | ||
|
||
return ( | ||
<section | ||
<div | ||
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. see Michael's comment for context regarding this change, but as a result, i needed to update the ref types. but that caused issues with the HeaderContext since it expects an HTMLElement. so i had to create a new context for GridListHeader rather than reuse the HeaderContext |
||
{...mergeProps(DOMProps, renderProps, rowGroupProps)} | ||
ref={ref}> | ||
<Provider | ||
values={[ | ||
[HeaderContext, {...rowProps, ref: headingRef}], | ||
[GridListHeaderContext, {...rowHeaderProps}] | ||
[GridListHeaderContext, {...rowProps, ref: headingRef}], | ||
[GridListHeaderInnerContext, {...rowHeaderProps}] | ||
]}> | ||
<CollectionBranch | ||
collection={state.collection} | ||
parent={item} /> | ||
</Provider> | ||
</section> | ||
</div> | ||
); | ||
}); | ||
|
||
const GridListHeaderContext = createContext<HTMLAttributes<HTMLElement> | null>(null); | ||
|
||
export const GridListHeader = /*#__PURE__*/ createLeafComponent(HeaderNode, function Header(props: HTMLAttributes<HTMLElement>, ref: ForwardedRef<HTMLElement>) { | ||
[props, ref] = useContextProps(props, ref, HeaderContext); | ||
let rowHeaderProps = useContext(GridListHeaderContext); | ||
export const GridListHeaderContext = createContext<ContextValue<HTMLAttributes<HTMLDivElement>, HTMLDivElement>>({}); | ||
const GridListHeaderInnerContext = createContext<HTMLAttributes<HTMLElement> | null>(null); | ||
|
||
export const GridListHeader = /*#__PURE__*/ createLeafComponent(HeaderNode, function Header(props: HTMLAttributes<HTMLDivElement>, ref: ForwardedRef<HTMLDivElement>) { | ||
[props, ref] = useContextProps(props, ref, GridListHeaderContext); | ||
let rowHeaderProps = useContext(GridListHeaderInnerContext); | ||
|
||
return ( | ||
<header className="react-aria-GridListHeader" ref={ref} {...props}> | ||
<div className="react-aria-GridListHeader" ref={ref} {...props}> | ||
<div {...rowHeaderProps} style={{display: 'contents'}}> | ||
{props.children} | ||
</div> | ||
</header> | ||
</div> | ||
); | ||
}); |
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.
updated so that we get the correct
aria-rowcount
, assuming that we want to count 'headers' as part of the rows