DocumentCard: parent container is no longer focusable and now has role=group to improve a11y experience#21461
Conversation
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 13000eb:
|
Asset size changes
Baseline commit: eab6f0f033b2fad90a25c421ab19f85d98417743 (build) |
📊 Bundle size report🤖 This report was generated against eab6f0f033b2fad90a25c421ab19f85d98417743 |
Perf Analysis (
|
| Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
|---|---|---|---|---|---|
| BaseButton | mount | 954 | 956 | 5000 | |
| Breadcrumb | mount | 2893 | 2878 | 1000 | |
| Checkbox | mount | 1532 | 1529 | 5000 | |
| CheckboxBase | mount | 1304 | 1328 | 5000 | |
| ChoiceGroup | mount | 4922 | 4903 | 5000 | |
| ComboBox | mount | 1037 | 1015 | 1000 | |
| CommandBar | mount | 10954 | 10924 | 1000 | |
| ContextualMenu | mount | 12023 | 12091 | 1000 | |
| DefaultButton | mount | 1182 | 1183 | 5000 | |
| DetailsRow | mount | 3986 | 4001 | 5000 | |
| DetailsRowFast | mount | 3974 | 4011 | 5000 | |
| DetailsRowNoStyles | mount | 3843 | 3804 | 5000 | |
| Dialog | mount | 2261 | 2288 | 1000 | |
| DocumentCardTitle | mount | 185 | 185 | 1000 | |
| Dropdown | mount | 3454 | 3449 | 5000 | |
| FocusTrapZone | mount | 1855 | 1929 | 5000 | |
| FocusZone | mount | 1887 | 1924 | 5000 | |
| IconButton | mount | 1815 | 1858 | 5000 | |
| Label | mount | 356 | 358 | 5000 | |
| Layer | mount | 3124 | 3112 | 5000 | |
| Link | mount | 475 | 480 | 5000 | |
| MenuButton | mount | 1553 | 1560 | 5000 | |
| MessageBar | mount | 2260 | 2284 | 5000 | |
| Nav | mount | 3523 | 3457 | 1000 | |
| OverflowSet | mount | 1149 | 1118 | 5000 | |
| Panel | mount | 2192 | 2225 | 1000 | |
| Persona | mount | 1027 | 1029 | 1000 | |
| Pivot | mount | 1642 | 1503 | 1000 | |
| PrimaryButton | mount | 1346 | 1377 | 5000 | |
| Rating | mount | 8096 | 8070 | 5000 | |
| SearchBox | mount | 1327 | 1384 | 5000 | |
| Shimmer | mount | 2582 | 2603 | 5000 | |
| Slider | mount | 2023 | 2011 | 5000 | |
| SpinButton | mount | 5236 | 5230 | 5000 | |
| Spinner | mount | 441 | 454 | 5000 | |
| SplitButton | mount | 3310 | 3355 | 5000 | |
| Stack | mount | 526 | 527 | 5000 | |
| StackWithIntrinsicChildren | mount | 2394 | 2356 | 5000 | |
| StackWithTextChildren | mount | 5451 | 5470 | 5000 | |
| SwatchColorPicker | mount | 12042 | 12031 | 5000 | |
| TagPicker | mount | 2867 | 2814 | 5000 | |
| TeachingBubble | mount | 98755 | 99369 | 5000 | |
| Text | mount | 442 | 450 | 5000 | |
| TextField | mount | 1477 | 1484 | 5000 | |
| ThemeProvider | mount | 1212 | 1251 | 5000 | |
| ThemeProvider | virtual-rerender | 660 | 676 | 5000 | |
| ThemeProvider | virtual-rerender-with-unmount | 1939 | 1931 | 5000 | |
| Toggle | mount | 827 | 866 | 5000 | |
| buttonNative | mount | 136 | 136 | 5000 |
smhigley
left a comment
There was a problem hiding this comment.
Question on this -- the React.Children approach seems to depend on DocumentCardTitle being the only child that has a title prop, which is problematic since title is also an HTML prop.
So, for instance, this will cause unexpected consequences for the button:
<DocumentCard
aria-label="Default Document Card with large file name. Created by Annie Lindqvist a few minutes ago."
onClickHref="http://bing.com"
>
<DocumentCardPreview {...previewProps} />
<DocumentCardTitle title={'File_name.txt'} />
<button title="tooltip text">Some action</button>
<DocumentCardActivity activity="Created a few minutes ago" people={DocumentCardActivityPeople} />
</DocumentCard>I think it'd probably be better to implement a context, and have DocumentCardTitle directly pull role/tabindex from it. Some v8 components already do this -- ResizeGroup, ScrollablePane, and ContextualMenu all look like they do, so it wouldn't be unprecedented.
smhigley
left a comment
There was a problem hiding this comment.
It looks like we'll also need :focus-visible styles for the title, but I'll approve this PR and open a separate issue
…e=group to improve a11y experience (microsoft#21461) * pass role and tabindex from parent to DocumentTitle via context
Pull request checklist
$ yarn changeDescription of changes
DocumentCardnow hastabIndexremoved and now has staticrole="group"so it's no longer focusable.Focus now moves to
DocumentCardTitle.DocumentCardnow passes propsroleandtabIndextoDocumentCardTitlevia a new react context calledDocumentCardContext.Redo of DocumentCard: parent container is no longer focusable and now has role=group to improve a11y experience #17806