-
Notifications
You must be signed in to change notification settings - Fork 1.3k
feat: support sections and headers in RAC gridlist #8667
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! 🎉 |
* See `useGridList` for more details about grid list. | ||
* @param props - Props for the section. | ||
*/ | ||
export function useGridListSection(props: AriaGridListSectionProps): GridListSectionAria { |
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.
pass state and ref, we always seem to regret not passing them and it's breaking to add them later
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.
we could pass it but wouldn't they be unused?
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.
still need to declare them in the signature so that others using the hook know they should supply them. that's the only way it can be non-breaking
|
||
const GridListHeaderContext = createContext<HTMLAttributes<HTMLElement> | null>(null); | ||
|
||
export const GridListHeader = /*#__PURE__*/ createLeafComponent('header', function Header(props: HTMLAttributes<HTMLElement>, ref: ForwardedRef<HTMLElement>) { |
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.
with ListBox, you can just use a normal Header
, but with GridList, in order to follow correct aria-pattern, the header must be inside a div with a role=row
, hence why i've created a new component called GridListHeader. are we okay with that?
from WAI-ARIA:
Each cell is either a DOM descendant of or owned by a row element and has one of the following roles:
- columnheader if the cell contains a title or header information for the column.
- rowheader if the cell contains title or header information for the row.
- gridcell if the cell does not contain column or row header information.
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 is ok, but the row
itself should get aria-attributes like aria-rowindex
when virtualized if so.
Build successful! 🎉 |
|
||
return ( | ||
<div {...rowProps} > | ||
<header className="react-aria-Header" {...props} ref={ref}> |
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.
Having two elements will make this harder to style. Maybe we can use display: 'contents'
on the inner one so we have the correct ARIA structure, but you only need to style the outer one:
<header {...rowProps} ref={ref}>
<div {...rowHeaderProps} style={{display: 'contents'}}>
{children}
</div>
</header>
This would match the structure of GridListItem
too.
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.
linking related discussion #8667 (comment)
I think I like the display contents better
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.
+1, I did the same for the load more elements too (i.e. TableLoadMoreItem)
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.
@snowystinger Regarding #8667 (comment)
I think we could provide a custom CollectionNode to wrap headers children with the row div.
Otherwise we could expose an internal context in Header
to be set by the GridList
? I think its worth to try and maintain a universal API if somehow possible.
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 we could provide a custom #8523 to wrap headers children with the row div.
Definitely an interesting idea. Though I imagine that would have the same issue with styling, so we'd probably still use 'display: contents'
, or possibly I haven't thought through your other PR enough yet?
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, styling issue persists. This just enables re-use of Header
.
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.
We've decided as a team to go ahead and use GridListHeader
since it's the most straightforward in terms of implementation. While the re-use of Header would be nice, with the exception of ListBox, we do have a history in RAC of having collection specific components ListBoxSection
vs. GridListSection
, or ListBoxItem
vs GridListItem
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.
fine with that 👍
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.
Just so we have the history, it was noted that we'd need a custom renderer as well in order to reuse the Header
in this way.
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.
still need to test the screenreader behavior but just some things I noted when scanning the code
|
||
const GridListHeaderContext = createContext<HTMLAttributes<HTMLElement> | null>(null); | ||
|
||
export const GridListHeader = /*#__PURE__*/ createLeafComponent('header', function Header(props: HTMLAttributes<HTMLElement>, ref: ForwardedRef<HTMLElement>) { |
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 is ok, but the row
itself should get aria-attributes like aria-rowindex
when virtualized if so.
Build successful! 🎉 |
Build successful! 🎉 |
Build successful! 🎉 |
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; |
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 did end up changing how we calculate rowIndex for rows inside Sections since Safari VO would read out the rows incorrectly otherwise. for example, say you were on the first item in Section 3, instead of saying something like "row 8 of 12" it would say "row 1 of 12"
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.
wonder if it would reasonable to use aria-posinset
like tree does, did you try that before?
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 way we calculate posinset for tree is also node.index + 1
so i'm not sure if that would be any different. either way, i think the calculation for the index/posinset will need to be adjusted unless you mean something else?
Build successful! 🎉 |
## API Changes
react-aria-components/react-aria-components:Popover Popover {
aria-describedby?: string
aria-details?: string
aria-label?: string
aria-labelledby?: string
arrowBoundaryOffset?: number = 0
- arrowRef?: RefObject<Element | null>
boundaryElement?: Element = document.body
children?: ChildrenOrFunction<PopoverRenderProps>
className?: ClassNameOrFunction<PopoverRenderProps>
containerPadding?: number = 12
defaultOpen?: boolean
isEntering?: boolean
isExiting?: boolean
isKeyboardDismissDisabled?: boolean = false
isNonModal?: boolean
isOpen?: boolean
maxHeight?: number
offset?: number = 8
onOpenChange?: (boolean) => void
placement?: Placement = 'bottom'
scrollRef?: RefObject<Element | null> = overlayRef
shouldCloseOnInteractOutside?: (Element) => boolean
shouldFlip?: boolean = true
shouldUpdatePosition?: boolean = true
slot?: string | null
style?: StyleOrFunction<PopoverRenderProps>
trigger?: string
triggerRef?: RefObject<Element | null>
} /react-aria-components:VisuallyHidden-VisuallyHidden {
- children?: ReactNode
- className?: string | undefined
- elementType?: string | JSXElementConstructor<any> = 'div'
- id?: string | undefined
- isFocusable?: boolean
- role?: AriaRole | undefined
- style?: CSSProperties | undefined
- tabIndex?: number | undefined
-} /react-aria-components:PopoverProps PopoverProps {
aria-describedby?: string
aria-details?: string
aria-label?: string
aria-labelledby?: string
arrowBoundaryOffset?: number = 0
- arrowRef?: RefObject<Element | null>
boundaryElement?: Element = document.body
children?: ChildrenOrFunction<PopoverRenderProps>
className?: ClassNameOrFunction<PopoverRenderProps>
containerPadding?: number = 12
defaultOpen?: boolean
isEntering?: boolean
isExiting?: boolean
isKeyboardDismissDisabled?: boolean = false
isNonModal?: boolean
isOpen?: boolean
maxHeight?: number
offset?: number = 8
onOpenChange?: (boolean) => void
placement?: Placement = 'bottom'
scrollRef?: RefObject<Element | null> = overlayRef
shouldCloseOnInteractOutside?: (Element) => boolean
shouldFlip?: boolean = true
shouldUpdatePosition?: boolean = true
slot?: string | null
style?: StyleOrFunction<PopoverRenderProps>
trigger?: string
triggerRef?: RefObject<Element | null>
} /react-aria-components:GridLayoutOptions GridLayoutOptions {
dropIndicatorThickness?: number = 2
maxColumns?: number = Infinity
- maxHorizontalSpace?: number = Infinity
maxItemSize?: Size = Infinity
minItemSize?: Size = 200 x 200
minSpace?: Size = 18 x 18
preserveAspectRatio?: boolean = false /react-aria-components:GridListHeader+GridListHeader {
+ UNTYPED
+} /react-aria-components:GridListSection+GridListSection <T extends {}> {
+ aria-label?: string
+ children?: ReactNode | ({}) => ReactElement
+ className?: string
+ dependencies?: ReadonlyArray<any>
+ id?: Key
+ items?: Iterable<{}>
+ style?: CSSProperties
+ value?: {}
+} @internationalized/date/@internationalized/date:setLocalTimeZone-setLocalTimeZone {
- timeZone: string
- returnVal: undefined
-} /@internationalized/date:resetLocalTimeZone-resetLocalTimeZone {
- returnVal: undefined
-} @react-aria/gridlist/@react-aria/gridlist:useGridListSection+useGridListSection <T> {
+ props: AriaGridListSectionProps
+ state: ListState<T>
+ ref: RefObject<HTMLElement | null>
+ returnVal: undefined
+} @react-aria/i18n/@react-aria/i18n:isRTL-isRTL {
- localeString: string
- returnVal: undefined
-} @react-aria/overlays/@react-aria/overlays:AriaPositionProps AriaPositionProps {
arrowBoundaryOffset?: number = 0
- arrowRef?: RefObject<Element | null>
arrowSize?: number = 0
boundaryElement?: Element = document.body
containerPadding?: number = 12
crossOffset?: number = 0
maxHeight?: number
offset?: number = 0
onClose?: () => void | null
overlayRef: RefObject<Element | null>
placement?: Placement = 'bottom'
scrollRef?: RefObject<Element | null> = overlayRef
shouldFlip?: boolean = true
shouldUpdatePosition?: boolean = true
targetRef: RefObject<Element | null>
} /@react-aria/overlays:PositionAria PositionAria {
arrowProps: DOMAttributes
overlayProps: DOMAttributes
placement: PlacementAxis | null
- triggerOrigin: {
- x: number
- y: number
-} | null
updatePosition: () => void
} /@react-aria/overlays:AriaPopoverProps AriaPopoverProps {
arrowBoundaryOffset?: number = 0
- arrowRef?: RefObject<Element | null>
arrowSize?: number = 0
boundaryElement?: Element = document.body
containerPadding?: number = 12
crossOffset?: number = 0
isKeyboardDismissDisabled?: boolean = false
isNonModal?: boolean
maxHeight?: number
offset?: number = 0
placement?: Placement = 'bottom'
popoverRef: RefObject<Element | null>
scrollRef?: RefObject<Element | null> = overlayRef
shouldCloseOnInteractOutside?: (Element) => boolean
shouldFlip?: boolean = true
shouldUpdatePosition?: boolean = true
triggerRef: RefObject<Element | null>
} /@react-aria/overlays:PopoverAria PopoverAria {
arrowProps: DOMAttributes
placement: PlacementAxis | null
popoverProps: DOMAttributes
- triggerOrigin: {
- x: number
- y: number
-} | null
underlayProps: DOMAttributes
} @react-spectrum/overlays/@react-spectrum/overlays:Popover Popover {
UNSAFE_className?: string
UNSAFE_style?: CSSProperties
alignSelf?: Responsive<'auto' | 'normal' | 'start' | 'end' | 'center' | 'flex-start' | 'flex-end' | 'self-start' | 'self-end' | 'stretch'>
arrowBoundaryOffset?: number = 0
- arrowRef?: RefObject<Element | null>
arrowSize?: number = 0
bottom?: Responsive<DimensionValue>
boundaryElement?: Element = document.body
children: ReactNode
containerPadding?: number = 12
crossOffset?: number = 0
disableFocusManagement?: boolean
enableBothDismissButtons?: boolean
end?: Responsive<DimensionValue>
flex?: Responsive<string | number | boolean>
flexBasis?: Responsive<number | string>
flexGrow?: Responsive<number>
flexShrink?: Responsive<number>
gridArea?: Responsive<string>
gridColumn?: Responsive<string>
gridColumnEnd?: Responsive<string>
gridColumnStart?: Responsive<string>
gridRow?: Responsive<string>
gridRowEnd?: Responsive<string>
gridRowStart?: Responsive<string>
groupRef?: RefObject<Element | null>
height?: Responsive<DimensionValue>
hideArrow?: boolean
isDisabled?: boolean
isHidden?: Responsive<boolean>
isKeyboardDismissDisabled?: boolean = false
isNonModal?: boolean
justifySelf?: Responsive<'auto' | 'normal' | 'start' | 'end' | 'flex-start' | 'flex-end' | 'self-start' | 'self-end' | 'center' | 'left' | 'right' | 'stretch'>
left?: Responsive<DimensionValue>
margin?: Responsive<DimensionValue>
marginBottom?: Responsive<DimensionValue>
marginEnd?: Responsive<DimensionValue>
marginStart?: Responsive<DimensionValue>
marginTop?: Responsive<DimensionValue>
marginX?: Responsive<DimensionValue>
marginY?: Responsive<DimensionValue>
maxHeight?: Responsive<DimensionValue>
maxWidth?: Responsive<DimensionValue>
minHeight?: Responsive<DimensionValue>
minWidth?: Responsive<DimensionValue>
offset?: number = 0
onBlurWithin?: (FocusEvent) => void
onDismissButtonPress?: () => void
onEnter?: () => void
onEntered?: () => void
onEntering?: () => void
onExit?: () => void
onExited?: () => void
onExiting?: () => void
onFocusWithin?: (FocusEvent) => void
onFocusWithinChange?: (boolean) => void
order?: Responsive<number>
placement?: Placement = 'bottom'
position?: Responsive<'static' | 'relative' | 'absolute' | 'fixed' | 'sticky'>
right?: Responsive<DimensionValue>
scrollRef?: RefObject<Element | null> = overlayRef
shouldCloseOnInteractOutside?: (Element) => boolean
shouldContainFocus?: boolean
shouldFlip?: boolean = true
shouldUpdatePosition?: boolean = true
start?: Responsive<DimensionValue>
state: OverlayTriggerState
top?: Responsive<DimensionValue>
triggerRef: RefObject<Element | null>
width?: Responsive<DimensionValue>
zIndex?: Responsive<number>
} @react-spectrum/s2/@react-spectrum/s2:PopoverProps PopoverProps {
UNSAFE_className?: UnsafeClassName
UNSAFE_style?: CSSProperties
aria-describedby?: string
aria-details?: string
aria-label?: string
aria-labelledby?: string
- arrowRef?: RefObject<Element | null>
boundaryElement?: Element = document.body
children?: ChildrenOrFunction<PopoverRenderProps>
className?: ClassNameOrFunction<PopoverRenderProps>
containerPadding?: number = 12
defaultOpen?: boolean
hideArrow?: boolean = false
isEntering?: boolean
isExiting?: boolean
isOpen?: boolean
maxHeight?: number
offset?: number = 8
onOpenChange?: (boolean) => void
placement?: Placement = 'bottom'
scrollRef?: RefObject<Element | null> = overlayRef
shouldFlip?: boolean = true
size?: 'S' | 'M' | 'L'
slot?: string | null
style?: StyleOrFunction<PopoverRenderProps>
styles?: StyleString
trigger?: string
triggerRef?: RefObject<Element | null>
} @react-stately/data/@react-stately/data:ListData ListData <T> {
- addKeysToSelection: (Selection) => void
append: (Array<T>) => void
filterText: string
getItem: (Key) => T | undefined
insert: (number, Array<T>) => void
insertAfter: (Key, Array<T>) => void
insertBefore: (Key, Array<T>) => void
items: Array<T>
move: (Key, number) => void
moveAfter: (Key, Iterable<Key>) => void
moveBefore: (Key, Iterable<Key>) => void
prepend: (Array<T>) => void
remove: (Array<Key>) => void
- removeKeysFromSelection: (Selection) => void
removeSelectedItems: () => void
selectedKeys: Selection
setFilterText: (string) => void
setSelectedKeys: (Selection) => void
} /@react-stately/data:AsyncListData AsyncListData <T> {
- addKeysToSelection: (Selection) => void
append: (Array<T>) => void
error?: Error
filterText: string
getItem: (Key) => T | undefined
insert: (number, Array<T>) => void
insertAfter: (Key, Array<T>) => void
insertBefore: (Key, Array<T>) => void
isLoading: boolean
items: Array<T>
loadMore: () => void
loadingState: LoadingState
move: (Key, number) => void
moveAfter: (Key, Iterable<Key>) => void
moveBefore: (Key, Iterable<Key>) => void
prepend: (Array<T>) => void
reload: () => void
remove: (Array<Key>) => void
- removeKeysFromSelection: (Selection) => void
removeSelectedItems: () => void
selectedKeys: Selection
setFilterText: (string) => void
setSelectedKeys: (Selection) => void
sortDescriptor?: SortDescriptor
update: (Key, T) => void
} @react-stately/layout/@react-stately/layout:GridLayoutOptions GridLayoutOptions {
dropIndicatorThickness?: number = 2
maxColumns?: number = Infinity
- maxHorizontalSpace?: number = Infinity
maxItemSize?: Size = Infinity
minItemSize?: Size = 200 x 200
minSpace?: Size = 18 x 18
preserveAspectRatio?: boolean = false |
}); | ||
|
||
return { | ||
rowProps: { |
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.
so ideally rowProps
would have aria-rowindex
but it's difficult to determine what the correct index is for the header. especially in the case where a gridlist has sections that both contain headers and don't container headers. basically, we rely on knowing the index of the previous section to determine what the index of the header should be. at the same time, we don't actually want to include sections in our calculations because they don't have the rowindex
value which is what makes this tricky.
it'd be easy to determine if we calculated it inside GridListHeader but that wouldn't match any of our existing API and would sort of defeat the purpose of having this hook supply this information...
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.
just thinking out loud (and definitely don't implement this yet), but is it possible to:
- Get the parent section
- Get section's previous key and source the previous section node
- Get the last child key of that section node and grab that key's node's index. That index should equal how many rows were in that section
- Repeat by going through each section before that and total up the total number of rows
I think the ideal way to calculate this will come from the collection nodes containing more information than they do now, include ways to get the "indexOfSameType" kind of information, something that maybe easier with some of the BaseCollection work in some of my other PRs
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 i think we could just do a sum. i can look into it a bit more
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.
Behavior looks good to me, JAWS and NVDA don't really seem to care about the missing row indexes on the Section headers so I think this is fine for now. Will need to test some of the more complicated use cases like DnD and make sure things like keyboard drag nav/drop indicators still work as expected
Ah yeah, DnD does not work. I actually removed some of the DnD stuff because it didn't make sense to have it before we get it fully working. We decided sometime during standup to not support DnD for the alpha state. It also doesn't work in ListBox sections, so we'll need to get dnd to work with sections in general. |
Closes
btw dnd it not supported in this pr, that'll be follow-up when we bump up sections to beta/rc/ga (one of them)
✅ Pull Request Checklist:
📝 Test Instructions:
test using the two added stories in RAC. one is virtualized + dynamic, the other is non-virtualized static
🧢 Your Project: