Skip to content

DocumentCard: parent container is no longer focusable and now has role=group to improve a11y experience#21461

Merged
smhigley merged 8 commits into
microsoft:masterfrom
TristanWatanabe:17718-take-2
Apr 22, 2022
Merged

DocumentCard: parent container is no longer focusable and now has role=group to improve a11y experience#21461
smhigley merged 8 commits into
microsoft:masterfrom
TristanWatanabe:17718-take-2

Conversation

@TristanWatanabe

@TristanWatanabe TristanWatanabe commented Jan 26, 2022

Copy link
Copy Markdown
Member

Pull request checklist

Description of changes

@codesandbox-ci

codesandbox-ci Bot commented Jan 26, 2022

Copy link
Copy Markdown

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:

Sandbox Source
@fluentui/react 8 starter Configuration
@fluentui/react-components 9 starter Configuration

@size-auditor

size-auditor Bot commented Jan 26, 2022

Copy link
Copy Markdown

Asset size changes

Project Bundle Baseline Size New Size Difference
office-ui-fabric-react fluentui-react-DocumentCard 202.33 kB 202.571 kB ExceedsBaseline     241 bytes

ExceedsTolerance Over Tolerance (1024 B) ExceedsBaseline Over Baseline BelowBaseline Below Baseline New New Deleted  Removed 1 kB = 1000 B

Baseline commit: eab6f0f033b2fad90a25c421ab19f85d98417743 (build)

@fabricteam

fabricteam commented Jan 26, 2022

Copy link
Copy Markdown
Collaborator

📊 Bundle size report

🤖 This report was generated against eab6f0f033b2fad90a25c421ab19f85d98417743

@fabricteam

fabricteam commented Jan 26, 2022

Copy link
Copy Markdown
Collaborator

Perf Analysis (@fluentui/react)

No significant results to display.

All results

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

@TristanWatanabe TristanWatanabe marked this pull request as ready for review January 26, 2022 21:53
@TristanWatanabe TristanWatanabe requested review from a team and yiminwu as code owners January 26, 2022 21:53
@ling1726 ling1726 closed this Feb 17, 2022
@ling1726 ling1726 reopened this Feb 17, 2022

@smhigley smhigley left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread packages/react/src/components/DocumentCard/DocumentCard.base.tsx Outdated

@smhigley smhigley left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like we'll also need :focus-visible styles for the title, but I'll approve this PR and open a separate issue

@smhigley smhigley merged commit 7d300d2 into microsoft:master Apr 22, 2022
marwan38 pushed a commit to marwan38/fluentui that referenced this pull request Jun 13, 2022
…e=group to improve a11y experience (microsoft#21461)

* pass role and tabindex from parent to DocumentTitle via context
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

DocumentCard has nested interactive items, structure, and very long link/button names

6 participants