-
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?
Conversation
Build successful! 🎉 |
Build successful! 🎉 |
Build successful! 🎉 |
); | ||
|
||
let rows = getAllByRole('row'); | ||
expect(rows).toHaveLength(15); |
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.
It seems that the aria-rowcount
prop on the role="grid"
does is not including the count for the number of section header roles.
In the Virtualized GridList Section example, the aria-colcount
on the grid is 50
, when if we were to include the header rows, the count should be 60
.
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 Accessibility checker within Storybook is throwing errors for an invalid aria role on the section
and header
elements used to define the Section and Section Headers. Can we use generic div
s with the same rowgroup
and row
roles instead?
See: https://w3c.github.io/html-aria/#el-header and https://w3c.github.io/html-aria/#el-section regarding the roles permitted on section
and header
.
Build successful! 🎉 |
@@ -254,7 +254,7 @@ export class BaseCollection<T> implements ICollection<Node<T>> { | |||
throw new Error('Cannot add a node to a frozen collection'); | |||
} | |||
|
|||
if (node.type === 'item' && this.keyMap.get(node.key) == null) { | |||
if (node.type === 'item' && this.keyMap.get(node.key) == null || node.type === 'header' && this.keyMap.get(node.key) == null) { |
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
@@ -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 comment
The 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
|
||
let sumOfNodes = (node: RSNode<unknown>): number => { | ||
if (node.prevKey === null) { | ||
let lastChildKey = (node as CollectionNode<T>).lastChildKey; |
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.
if we want to use lastChildKey
, the node types needs to defined as a CollectionNode<T>
. however, createBranchComponent
expects the item
to be type Node<T>
which doesn't have lastChildKey defined and i don't think i should change the type declarations in createBranchComponent
. as a result, i end up having to cast the type here as CollectionNode
. it should be fine tho since the BaseCollection uses CollectionNodes anyway
Build successful! 🎉 |
@@ -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 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 something isInSection
and then determine that value inside of GridListItem by checking whether the parent node's type is a section. how does that sound?
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:
let parentNode = node.parentKey ? state.collection.getItem(node.parentKey) as CollectionNode<T> : null;
let isInSection = parentNode && parentNode.type === 'section';
This line here:
let nodes = [...collection];
if (nodes.some(node => node.type === 'section')) {
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 context
listMap.set(state, {id, onAction, linkBehavior, keyboardNavigationBehavior, shouldSelectOnPressUp}); |
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.
@@ -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 comment
The 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
it's also recursive, is that to handle nesting of sections inside sections?
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.
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...
Build successful! 🎉 |
Build successful! 🎉 |
## API Changes
react-aria-components/react-aria-components:GridListHeaderContext+GridListHeaderContext {
+ UNTYPED
+} |
Adds
aria-rowindex
to GridListSections and fixes howaria-rowindex
is calculated in GridListItems when there is a mix of items inside and outside of Sections.Adds 'headers' to be included in
aria-rowcount
Some things to note about the screen reader experience which are likely screenreader bugs:
Some screenreader bugs reported by Michael already:
https://bugs.webkit.org/buglist.cgi?bug_status=UNCONFIRMED&bug_status=NEW&bug_status=ASSIGNED&bug_status=REOPENED&component=Accessibility&email1=mijordan%40adobe.com&emailassigned_to1=1&emailreporter1=1&emailtype1=substring&list_id=12542061&query_format=advanced&short_desc=grid&short_desc_type=allwordssubstr
✅ Pull Request Checklist:
📝 Test Instructions:
🧢 Your Project: