-
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
Changes from all 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'; | ||
|
@@ -67,7 +68,7 @@ export function useGridListItem<T>(props: AriaGridListItemOptions, state: ListSt | |
|
||
// let stringFormatter = useLocalizedStringFormatter(intlMessages, '@react-aria/gridlist'); | ||
let {direction} = useLocale(); | ||
let {onAction, linkBehavior, keyboardNavigationBehavior, shouldSelectOnPressUp} = listMap.get(state)!; | ||
let {onAction, linkBehavior, keyboardNavigationBehavior, shouldSelectOnPressUp, hasSection} = listMap.get(state)!; | ||
let descriptionId = useSlotId(); | ||
|
||
// We need to track the key of the item at the time it was last focused so that we force | ||
|
@@ -277,6 +278,27 @@ 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 prevKey is null, then this is the first node in the collection so get number of row(s) | ||
if (node.prevKey === null) { | ||
return getNumberOfRows(node, state); | ||
} | ||
|
||
// If the node is an item inside of a section, get number of rows in the current section | ||
let parentNode = node.parentKey ? state.collection.getItem(node.parentKey) as CollectionNode<T> : null; | ||
if (parentNode && parentNode.type === 'section') { | ||
return sumOfNodes(parentNode); | ||
} | ||
|
||
// Otherwise, if the node is a section or item outside of a section, recursively call to get the current sum + get the number of row(s) | ||
let prevNode = state.collection.getItem(node.prevKey!) as CollectionNode<T>; | ||
if (prevNode) { | ||
return sumOfNodes(prevNode) + getNumberOfRows(node, state); | ||
} | ||
|
||
return 0; | ||
}; | ||
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 seems like it could be expensive for large collections. It's walking a large portion of the collection for each item on every render. We might need to find a way to do this more efficiently (e.g. in Document). |
||
|
||
let rowProps: DOMAttributes = mergeProps(itemProps, linkProps, { | ||
role: 'row', | ||
onKeyDownCapture, | ||
|
@@ -291,10 +313,26 @@ export function useGridListItem<T>(props: AriaGridListItemOptions, state: ListSt | |
}); | ||
|
||
if (isVirtualized) { | ||
let {collection} = state; | ||
let nodes = [...collection]; | ||
// 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 (hasSection) { | ||
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 support a gridlist that has sections but also has items that don't fall inside a section? I think that case isn't supported else where so since we can check the parent type, perhaps we can get rid of this 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 went under the assumption that we did support gridlists with items both within and outside of sections. i guess i didn't really see a reason to restrict 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. I think we chatted about a similar case for Menus/Listbox in the past and settled on saying that mixed case wouldn't be supported but lets bring this up in otters |
||
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 delta = lastChild ? lastChild.index - node.index : 0; | ||
if (parentNode && parentNode.prevKey) { | ||
rowProps['aria-rowindex'] = sumOfNodes(parentNode) - delta; | ||
} else { | ||
// If the item is within a section but the section is the first node in the collection | ||
rowProps['aria-rowindex'] = node.index + 1; | ||
} | ||
} else { | ||
rowProps['aria-rowindex'] = sumOfNodes(node as CollectionNode<T>); | ||
} | ||
} else { | ||
rowProps['aria-rowindex'] = node.index + 1; | ||
} | ||
} | ||
|
||
let gridCellProps = { | ||
|
@@ -324,3 +362,15 @@ function last(walker: TreeWalker) { | |
} while (last); | ||
return next; | ||
} | ||
|
||
export function getNumberOfRows<T>(node: RSNode<unknown>, state: ListState<T> | TreeState<T>) { | ||
if (node.type === 'section') { | ||
// Use the index of the last child to determine the number of nodes in the section | ||
let currentNode = node as CollectionNode<T>; | ||
let lastChild = currentNode.lastChildKey ? state.collection.getItem(currentNode.lastChildKey) : null; | ||
return lastChild ? lastChild.index + 1 : 0; | ||
} else if (node.type === 'item') { | ||
return 1; | ||
} | ||
return 0; | ||
} |
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 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. with regards to the |
||
{...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.
I think this will affect headers in other collections like ListBox where they should not be counted as a row...