chore: Refactor S2 tabs to fix accessibility issues #7600
Conversation
There was a problem hiding this comment.
Is this going to be confusing to users, swapping between two completely different components?
Especially if focus is lost, it may be non-obvious that you're back on the tab group because when it collapses, it no longer announces as a tab group. Maybe we can improve it by adding that to the Select's label.
I'm fine with the approach, I assume it aligns with the collapse work you were doing before across all components? you'd said you were able to get rid of the custom renderer there as well.
| } | ||
| prevFocused.current = state?.selectionManager.isFocused; | ||
| }, [state?.selectionManager.isFocused, state?.selectionManager.focusedKey, showItems]); | ||
| // let prevFocused = useRef<boolean | undefined>(false); |
There was a problem hiding this comment.
Noticed that both in the previous implementation and this one, swapping between the components will lose focus. I swear I had that addressed at one point, but we should probably include something so that doesn't occur. I suspect it was linked with this section of code originally
There was a problem hiding this comment.
It was definitely this code, however, it didn't restore back when the picker expanded back to the tabs, so I've omitted it for now to simplify getting the PR in
We can add that behavior later
That's pretty much what v3 does right? When it's collapsed the tablist is removed from the a11y tree. And it seemed kinda weird to have a |
Ok, yep, dang my recollection of v3 was bad on this one. Silly vacation-addled brain. |
# Conflicts: # packages/@react-spectrum/s2/src/Tabs.tsx
| }})({density})}> | ||
| <Picker | ||
| id={id} | ||
| aria-label={props['aria-label'] ?? stringFormatter.format('tabs.selectorLabel')} |
There was a problem hiding this comment.
Re-added a default here since aria-label isn't required for Tabs, but it is required for a picker that has no visible label.
There was a problem hiding this comment.
hmm should it be required for Tabs too?
There was a problem hiding this comment.
That's a possible solution. We haven't required that in the past.
That said, it seems like a nice thing to do for users.
Looks like the aria patterns page has this to say
If the tab list has a visible label, the element with role tablist has aria-labelledby set to a value that refers to the labelling element. Otherwise, the tablist element has a label provided by aria-label.
So that seems to say we should require either aria-labelledby or aria-label
| const tabPanel = style({ | ||
| display: 'flex', | ||
| marginTop: 4, | ||
| marginX: -4, |
There was a problem hiding this comment.
i adjusted some styles in here to make it easier to create a panel that scrolls for overflow and compensates for our focus halos automatically
it also automatically lays out in such a way that users can place a child which will stretch to the fill the space because it's now default display: flex
let me know if this is too controversial
another option was to use allowedOverrides for styles and add display/padding to it
snowystinger
changed the title
| @@ -25,6 +25,7 @@ | |||
| "table.sortAscending": "Sort Ascending", | |||
| "table.sortDescending": "Sort Descending", | |||
| "table.resizeColumn": "Resize column", | |||
| "tabs.selectorLabel": "Tab selector", | |||
There was a problem hiding this comment.
Do we need these additional strings? We didn't have these in v3. Is the label provided by the app enough?
There was a problem hiding this comment.
linking to discussion about this #7600 (comment)
| }})({density})}> | ||
| <Picker | ||
| id={id} | ||
| aria-label={props['aria-label'] ?? stringFormatter.format('tabs.selectorLabel')} |
There was a problem hiding this comment.
hmm should it be required for Tabs too?
# Conflicts: # packages/@react-spectrum/s2/package.json # yarn.lock
| ); | ||
| export const Example = { | ||
| render: (args: any) => ( | ||
| <Tabs {...args} styles={style({width: 450, height: 256})} aria-label="History of Ancient Rome"> |
There was a problem hiding this comment.
opinion, I changed S2 Tabs to require an aria-label or aria-labelledby following #7600 (comment)
I needed to move it to the Tabs component so that I could propagate it to the TabList OR the TabMenu. It's also important that the element is always in the DOM and the TabList isn't when we're collapsed.
Should I push some form of this change down to RAC Tabs? If so, what? I could remove the label props, but then extending them is weird.
We currently accept both label props on both Tabs and TabList, though putting it on RAC Tabs actually won't do anything for accessibility.
Do I remove them all (label/described/etc) from the S2 TabList?
There was a problem hiding this comment.
IMO I think it is fine to have the aria-label/labelledby props at the top level for S2 Tabs even if it differs from RAC Tabs. As for whether or not to change RAC Tabs, I guess that is dependent on whether we want to make re-creating this kind of collapsable behavior easier for end users (though they can implement a context shuttling down the aria-label/labelledby to their TabList/Tabpicker relatively easily I suppose).
We currently accept both label props on both Tabs and TabList, though putting it on RAC Tabs actually won't do anything for accessibility.
Not sure I follow this, looks like you omit those label props from S2 TabList? Unless you are referencing RAC Tabs but RAC Tabs doesn't take the label props in that case.
There was a problem hiding this comment.
Referring to RAC Tabs, looks like it takes label props?
There was a problem hiding this comment.
oh hm, well at the moment providing an aria-label to Tabs doesn't apply it anywhere I believe because
labelable, which on a 2nd read is what I guess you were saying with though putting it on RAC Tabs actually won't do anything for accessibility....
Actually looks like a bunch of our components (ComboBox, etc) that have a outer div wrapper have this mismatch, not sure we actually want to allow the container to have those label props...
There was a problem hiding this comment.
Ah looks like they propagate those label props to the inner input/etc so might be good to do the same for RAC Tabs
There was a problem hiding this comment.
Do we remove the labelable props on the TabList then? that'd technically be breaking
| isQuiet, | ||
| density | ||
| })}> | ||
| <SelectValue id={valueId} className={valueStyles + ' ' + raw('&> * {display: none;}')}> |
There was a problem hiding this comment.
Is it possible to attach an aria-label to the Icon only picker defaulting to the Tab's aria-label or the hidden Tab text here? Testing on my phone and it feels a bit awkward only having the overall Tabs aria label announced and then requiring the user to open the dropdown to see what tab is actually selected.
There was a problem hiding this comment.
same thing happens to the TabPanel as well in the Icons story, if "labelBehavior" is "hide" and the Tabs have been collapsed then the TabPanel only announces the aria-label passed to Tabs and loses the additional labeling information tied to the selected tab.
There was a problem hiding this comment.
ok, I think I've gotten it about as good as I can, there's a slight double announcement on TabPanel, but at least everything is announced
## API Changes
@react-spectrum/s2/@react-spectrum/s2:Heading Heading {
UNSAFE_className?: string
UNSAFE_style?: CSSProperties
children?: ReactNode
+ id?: string
isHidden?: boolean
level?: number
slot?: string | null
styles?: StyleString/@react-spectrum/s2:Header Header {
UNSAFE_className?: string
UNSAFE_style?: CSSProperties
children?: ReactNode
+ id?: string
isHidden?: boolean
slot?: string | null
styles?: StyleString
}/@react-spectrum/s2:Content Content {
UNSAFE_className?: string
UNSAFE_style?: CSSProperties
children?: ReactNode
+ id?: string
isHidden?: boolean
slot?: string | null
styles?: StyleString
}/@react-spectrum/s2:Footer Footer {
UNSAFE_className?: string
UNSAFE_style?: CSSProperties
children?: ReactNode
+ id?: string
isHidden?: boolean
slot?: string | null
styles?: StyleString
}/@react-spectrum/s2:Text Text {
UNSAFE_className?: string
UNSAFE_style?: CSSProperties
children?: ReactNode
+ id?: string
isHidden?: boolean
slot?: string | null
styles?: StyleString
}/@react-spectrum/s2:Keyboard Keyboard {
UNSAFE_className?: string
UNSAFE_style?: CSSProperties
children?: ReactNode
+ id?: string
isHidden?: boolean
slot?: string | null
styles?: StyleString
}/@react-spectrum/s2:Tabs Tabs {
UNSAFE_className?: string
UNSAFE_style?: CSSProperties
aria-describedby?: string
aria-details?: string
aria-label?: string
aria-labelledby?: string
children?: ReactNode
defaultSelectedKey?: Key
density?: 'compact' | 'regular' = 'regular'
disabledKeys?: Iterable<Key>
id?: string
isDisabled?: boolean
keyboardActivation?: 'automatic' | 'manual' = 'automatic'
+ labelBehavior?: 'show' | 'hide' = 'show'
onSelectionChange?: (Key) => void
orientation?: Orientation = 'horizontal'
selectedKey?: Key | null
slot?: string | null
}/@react-spectrum/s2:TabList TabList <T extends {}> {
UNSAFE_className?: string
UNSAFE_style?: CSSProperties
aria-describedby?: string
aria-details?: string
- aria-label?: string
- aria-labelledby?: string
- children?: ReactNode
+ children?: ReactNode | (T) => ReactNode
dependencies?: Array<any>
items?: Iterable<T>
styles?: StylesProp
}/@react-spectrum/s2:Tab Tab {
UNSAFE_className?: string
UNSAFE_style?: CSSProperties
aria-describedby?: string
aria-details?: string
aria-label?: string
aria-labelledby?: string
- children?: ReactNode
+ children: ReactNode
download?: boolean | string
href?: Href
hrefLang?: string
id?: Key
onHoverChange?: (boolean) => void
onHoverEnd?: (HoverEvent) => void
onHoverStart?: (HoverEvent) => void
ping?: string
referrerPolicy?: HTMLAttributeReferrerPolicy
rel?: string
routerOptions?: RouterOptions
styles?: StylesProp
target?: HTMLAttributeAnchorTarget
}/@react-spectrum/s2:TabsProps TabsProps {
UNSAFE_className?: string
UNSAFE_style?: CSSProperties
aria-describedby?: string
aria-details?: string
aria-label?: string
aria-labelledby?: string
children?: ReactNode
defaultSelectedKey?: Key
density?: 'compact' | 'regular' = 'regular'
disabledKeys?: Iterable<Key>
id?: string
isDisabled?: boolean
keyboardActivation?: 'automatic' | 'manual' = 'automatic'
+ labelBehavior?: 'show' | 'hide' = 'show'
onSelectionChange?: (Key) => void
orientation?: Orientation = 'horizontal'
selectedKey?: Key | null
slot?: string | null
}/@react-spectrum/s2:TabProps TabProps {
UNSAFE_className?: string
UNSAFE_style?: CSSProperties
aria-describedby?: string
aria-details?: string
aria-label?: string
aria-labelledby?: string
- children?: ReactNode
+ children: ReactNode
download?: boolean | string
href?: Href
hrefLang?: string
id?: Key
onHoverChange?: (boolean) => void
onHoverEnd?: (HoverEvent) => void
onHoverStart?: (HoverEvent) => void
ping?: string
referrerPolicy?: HTMLAttributeReferrerPolicy
rel?: string
routerOptions?: RouterOptions
styles?: StylesProp
target?: HTMLAttributeAnchorTarget
}/@react-spectrum/s2:TabListProps TabListProps <T> {
UNSAFE_className?: string
UNSAFE_style?: CSSProperties
aria-describedby?: string
aria-details?: string
- aria-label?: string
- aria-labelledby?: string
- children?: ReactNode
+ children?: ReactNode | (T) => ReactNode
dependencies?: Array<any>
items?: Iterable<T>
styles?: StylesProp
} |
Collapsed S2 tabs currently has a Picker inside a TabList, which is invalid aria according to axe. This refactors it to swap between RAC tabs and a simple picker + Group, which is similar to what we had in v3.
Probably some stuff still broken here, just a first pass.