-
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 6 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 |
---|---|---|
|
@@ -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 |
---|---|---|
|
@@ -494,6 +494,85 @@ describe('GridList', () => { | |
expect(items).toHaveLength(2); | ||
}); | ||
|
||
it('should calculate the correct aria-rowindex when gridlist is made up of only sections', () => { | ||
let sections = []; | ||
for (let s = 0; s < 5; s++) { | ||
let items = []; | ||
for (let i = 0; i < 2; i++) { | ||
items.push({id: `item_${s}_${i}`, name: `Section ${s}, Item ${i}`}); | ||
} | ||
sections.push({id: `section_${s}`, name: `Section ${s}`, children: items}); | ||
} | ||
|
||
jest.spyOn(window.HTMLElement.prototype, 'clientWidth', 'get').mockImplementation(() => 100); | ||
jest.spyOn(window.HTMLElement.prototype, 'clientHeight', 'get').mockImplementation(() => 300); | ||
|
||
let {getAllByRole} = render( | ||
<Virtualizer layout={ListLayout} layoutOptions={{headingHeight: 25, rowHeight: 25}}> | ||
<GridList aria-label="Test" items={sections}> | ||
<Collection items={sections}> | ||
{section => ( | ||
<GridListSection> | ||
<GridListHeader>{section.name}</GridListHeader> | ||
<Collection items={section.children} > | ||
{item => <GridListItem>{item.name}</GridListItem>} | ||
</Collection> | ||
</GridListSection> | ||
)} | ||
</Collection> | ||
</GridList> | ||
</Virtualizer> | ||
); | ||
|
||
let rows = getAllByRole('row'); | ||
expect(rows).toHaveLength(15); | ||
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. It seems that the In the Virtualized GridList Section example, the |
||
expect(rows.map(r => r.textContent)).toEqual(['Section 0', 'Section 0, Item 0', 'Section 0, Item 1', 'Section 1', 'Section 1, Item 0', 'Section 1, Item 1', 'Section 2', 'Section 2, Item 0', 'Section 2, Item 1', 'Section 3', 'Section 3, Item 0', 'Section 3, Item 1', 'Section 4', 'Section 4, Item 0', 'Section 4, Item 1']); | ||
|
||
for (let i = 0; i < 15; i++) { | ||
expect(rows[i]).toHaveAttribute('aria-rowindex'); | ||
expect(rows[i].getAttribute('aria-rowindex')).toEqual(`${i + 1}`); | ||
} | ||
}); | ||
|
||
it('should calculate the correct aria-rowindex when there is a mix of sections and individual items', () => { | ||
jest.spyOn(window.HTMLElement.prototype, 'clientWidth', 'get').mockImplementation(() => 100); | ||
jest.spyOn(window.HTMLElement.prototype, 'clientHeight', 'get').mockImplementation(() => 300); | ||
|
||
let {getAllByRole} = render( | ||
<Virtualizer layout={ListLayout} layoutOptions={{headingHeight: 25, rowHeight: 25}}> | ||
<GridList aria-label="Test"> | ||
<GridListItem>Home</GridListItem> | ||
<GridListItem>School</GridListItem> | ||
<GridListSection> | ||
<GridListHeader>Pets</GridListHeader> | ||
<GridListItem>Cat</GridListItem> | ||
<GridListItem>Dog</GridListItem> | ||
</GridListSection> | ||
<GridListSection aria-label="Ice cream flavors"> | ||
<GridListItem>Vanilla</GridListItem> | ||
<GridListItem>Chocolate</GridListItem> | ||
</GridListSection> | ||
<GridListItem>City Hall</GridListItem> | ||
<GridListItem>Pharmacy</GridListItem> | ||
<GridListSection> | ||
<GridListHeader>Plants</GridListHeader> | ||
<GridListItem>Sunflower</GridListItem> | ||
<GridListItem>Daffodil</GridListItem> | ||
</GridListSection> | ||
</GridList> | ||
</Virtualizer> | ||
); | ||
|
||
let rows = getAllByRole('row'); | ||
expect(rows).toHaveLength(12); | ||
expect(rows.map(r => r.textContent)).toEqual(['Home', 'School', 'Pets', 'Cat', 'Dog', 'Vanilla', 'Chocolate', 'City Hall', 'Pharmacy', 'Plants', 'Sunflower', 'Daffodil']); | ||
|
||
for (let i = 0; i < 12; i++) { | ||
expect(rows[i]).toHaveAttribute('aria-rowindex'); | ||
expect(rows[i].getAttribute('aria-rowindex')).toEqual(`${i + 1}`); | ||
} | ||
}); | ||
|
||
describe('selectionBehavior="replace"', () => { | ||
// Required for proper touch detection | ||
installPointerEvent(); | ||
|
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 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 (
hasSections
) via our hooks context or in the state?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.
yeah we could add a new prop to
useGridListItem
called somethingisInSection
and then determine that value inside of GridListItem by checking whether the parent node's type is a section. how does that sound?Uh oh!
There was an error while loading. Please reload this page.
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.
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
collectionHasSections
in useGridList once, then passing it along on the hook contextreact-spectrum/packages/@react-aria/gridlist/src/useGridList.ts
Line 148 in d434b6d
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 comment
The reason will be displayed to describe this comment to others. Learn more.
ohhhh right right, sorry i see what you mean