Skip to content
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.

Sign up for GitHub

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

chore: Refactor S2 tabs to fix accessibility issues #7600

Open
wants to merge 19 commits into
base: main
Choose a base branch
from
+972 −264
Open

chore: Refactor S2 tabs to fix accessibility issues #7600

Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
e205d8d
wip: Refactor S2 tabs to fix accessibility issues
devongovett Jan 11, 2025
08ab651
yarn
devongovett Jan 11, 2025
1dbf1b8
Merge branch 'main' into tabs-refactor
snowystinger Jan 14, 2025
969e5d7
review comments
snowystinger Jan 14, 2025
e44da4a
fix lint
snowystinger Jan 14, 2025
b56a7a3
Merge branch 'main' into tabs-refactor
snowystinger Jan 14, 2025
23c4960
fix orientation case
snowystinger Jan 14, 2025
4c1bfa9
Merge branch 'tabs-refactor' of github.com:adobe/react-spectrum into …
snowystinger Jan 14, 2025
e803d9f
Fix orientation case not updating overflow
snowystinger Jan 14, 2025
783c241
Merge branch 'main' into tabs-refactor
snowystinger Jan 14, 2025
92cb122
Merge branch 'main' into tabs-refactor
snowystinger Jan 22, 2025
86a98e3
Add required labelling and other review comments
snowystinger Jan 22, 2025
ce5ac32
fix a few more announcements
snowystinger Jan 22, 2025
1a64592
fix lint
snowystinger Jan 22, 2025
d760020
fix chromatic
snowystinger Jan 22, 2025
143f0ce
remove translations
snowystinger Jan 22, 2025
8d848d9
fix announcements
snowystinger Jan 23, 2025
5b8d8a0
fix lint
snowystinger Jan 23, 2025
4340518
Merge branch 'main' into tabs-refactor
snowystinger Jan 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
129 changes: 68 additions & 61 deletions packages/@react-spectrum/s2/chromatic/Tabs.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import Heart from '../s2wf-icons/S2_Icon_Heart_20_N.svg';
import type {Meta} from '@storybook/react';
import {style} from '../style/spectrum-theme' with { type: 'macro' };
import {Tab, TabList, TabPanel, Tabs} from '../src/Tabs';
import {Text} from '@react-spectrum/s2';

const meta: Meta<typeof Tabs> = {
component: Tabs,
Expand All @@ -27,66 +28,72 @@ const meta: Meta<typeof Tabs> = {

export default meta;

export const Example = (args: any) => (
<Tabs {...args} styles={style({width: 450, height: 256})}>
<TabList aria-label="History of Ancient Rome">
<Tab id="FoR"><Edit />Founding of Rome</Tab>
<Tab id="MaR">Monarchy and Republic</Tab>
<Tab id="Emp">Empire</Tab>
</TabList>
<TabPanel id="FoR" UNSAFE_style={{display: 'flex'}}>
<div style={{overflow: 'auto'}}>
<p>Lorem ipsum dolor sit amet, consectetur adipiscing elit. Vestibulum non rutrum augue, a dictum est. Sed ultricies vel orci in blandit. Morbi sed tempor leo. Phasellus et sollicitudin nunc, a volutpat est. In volutpat molestie velit, nec rhoncus felis vulputate porttitor. In efficitur nibh tortor, maximus imperdiet libero sollicitudin sed. Pellentesque dictum, quam id scelerisque rutrum, lorem augue suscipit est, nec ultricies ligula lorem id dui. Cras lacus tortor, fringilla nec ligula quis, semper imperdiet ex.</p>
</div>
</TabPanel>
<TabPanel id="MaR" UNSAFE_style={{display: 'flex'}}>
<div style={{overflow: 'auto'}}>
<p>Lorem ipsum dolor sit amet, consectetur adipiscing elit. Sed ut vulputate justo. Suspendisse potenti. Nunc id fringilla leo, at luctus quam. Maecenas et ipsum nisi. Curabitur in porta purus, a pretium est. Fusce eu urna diam. Sed nunc neque, consectetur ut purus nec, consequat elementum libero. Sed ut diam in quam maximus condimentum at non erat. Vestibulum sagittis rutrum velit, vitae suscipit arcu. Nulla ac feugiat ante, vitae laoreet ligula. Maecenas sed molestie ligula. Nulla sed fringilla ex. Nulla viverra tortor at enim condimentum egestas. Nulla sed tristique sapien. Integer ligula quam, vulputate eget mollis eu, interdum sit amet justo.</p>
<p>Vivamus dignissim tortor ut sapien congue tristique. Sed ac aliquet mauris. Nulla metus dui, elementum sit amet luctus eu, condimentum id elit. Praesent id nibh sed ligula congue venenatis. Pellentesque urna turpis, eleifend id pellentesque a, auctor nec neque. Vestibulum ipsum mauris, rutrum sit amet magna et, aliquet mollis tellus. Pellentesque nec ultricies nibh, at tempus massa. Phasellus dictum turpis et interdum scelerisque. Aliquam fermentum tincidunt ipsum sit amet suscipit. Fusce non dui sed diam lacinia mattis fermentum eu urna. Cras pretium id nunc in elementum. Mauris laoreet odio vitae laoreet dictum. In non justo nec nunc vehicula posuere non non ligula. Nullam eleifend scelerisque nibh, in sollicitudin tortor ullamcorper vel. Praesent sagittis risus in erat dignissim, non lacinia elit efficitur. Quisque maximus nulla vel luctus pharetra.</p>
</div>
</TabPanel>
<TabPanel id="Emp" UNSAFE_style={{display: 'flex'}}>
<div style={{overflow: 'auto'}}>
<p>Alea jacta est.</p>
</div>
</TabPanel>
</Tabs>
);
export const Example = {
render: (args: any) => (
<Tabs {...args} styles={style({width: 450, height: 256})} aria-label="History of Ancient Rome">
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Referring to RAC Tabs, looks like it takes label props?
Screenshot 2025-01-23 at 6 22 06 am

Copy link
Member

@LFDanLu LFDanLu Jan 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

oh hm, well at the moment providing an aria-label to Tabs doesn't apply it anywhere I believe because

react-spectrum/packages/react-aria-components/src/Tabs.tsx

Line 167 in 7b7b461

{...filterDOMProps(props as any)}
doesn't have 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...

Copy link
Member

Choose a reason for hiding this comment

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

Ah looks like they propagate those label props to the inner input/etc so might be good to do the same for RAC Tabs

Copy link
Member

Choose a reason for hiding this comment

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

Do we remove the labelable props on the TabList then? that'd technically be breaking

<TabList>
<Tab id="FoR">Founding of Rome</Tab>
<Tab id="MaR">Monarchy and Republic</Tab>
<Tab id="Emp">Empire</Tab>
</TabList>
<TabPanel id="FoR">
<div>
<div>Lorem ipsum dolor sit amet, consectetur adipiscing elit. Vestibulum non rutrum augue, a dictum est. Sed ultricies vel orci in blandit. Morbi sed tempor leo. Phasellus et sollicitudin nunc, a volutpat est. In volutpat molestie velit, nec rhoncus felis vulputate porttitor. In efficitur nibh tortor, maximus imperdiet libero sollicitudin sed. Pellentesque dictum, quam id scelerisque rutrum, lorem augue suscipit est, nec ultricies ligula lorem id dui. Cras lacus tortor, fringilla nec ligula quis, semper imperdiet ex.</div>
</div>
</TabPanel>
<TabPanel id="MaR">
<div>
<div>Lorem ipsum dolor sit amet, consectetur adipiscing elit. Sed ut vulputate justo. Suspendisse potenti. Nunc id fringilla leo, at luctus quam. Maecenas et ipsum nisi. Curabitur in porta purus, a pretium est. Fusce eu urna diam. Sed nunc neque, consectetur ut purus nec, consequat elementum libero. Sed ut diam in quam maximus condimentum at non erat. Vestibulum sagittis rutrum velit, vitae suscipit arcu. Nulla ac feugiat ante, vitae laoreet ligula. Maecenas sed molestie ligula. Nulla sed fringilla ex. Nulla viverra tortor at enim condimentum egestas. Nulla sed tristique sapien. Integer ligula quam, vulputate eget mollis eu, interdum sit amet justo.</div>
<div>Vivamus dignissim tortor ut sapien congue tristique. Sed ac aliquet mauris. Nulla metus dui, elementum sit amet luctus eu, condimentum id elit. Praesent id nibh sed ligula congue venenatis. Pellentesque urna turpis, eleifend id pellentesque a, auctor nec neque. Vestibulum ipsum mauris, rutrum sit amet magna et, aliquet mollis tellus. Pellentesque nec ultricies nibh, at tempus massa. Phasellus dictum turpis et interdum scelerisque. Aliquam fermentum tincidunt ipsum sit amet suscipit. Fusce non dui sed diam lacinia mattis fermentum eu urna. Cras pretium id nunc in elementum. Mauris laoreet odio vitae laoreet dictum. In non justo nec nunc vehicula posuere non non ligula. Nullam eleifend scelerisque nibh, in sollicitudin tortor ullamcorper vel. Praesent sagittis risus in erat dignissim, non lacinia elit efficitur. Quisque maximus nulla vel luctus pharetra.</div>
</div>
</TabPanel>
<TabPanel id="Emp">
<div>
<div>Alea jacta est.</div>
</div>
</TabPanel>
</Tabs>
)
};

export const Disabled = (args: any) => (
<Tabs {...args} styles={style({width: 450, height: 144})} disabledKeys={['FoR', 'MaR', 'Emp']}>
<TabList aria-label="History of Ancient Rome">
<Tab id="FoR"><Edit />Founding of Rome</Tab>
<Tab id="MaR">Monarchy and Republic</Tab>
<Tab id="Emp">Empire</Tab>
</TabList>
<TabPanel id="FoR">
Arma virumque cano, Troiae qui primus ab oris.
</TabPanel>
<TabPanel id="MaR">
Senatus Populusque Romanus.
</TabPanel>
<TabPanel id="Emp">
Alea jacta est.
</TabPanel>
</Tabs>
);
export const Disabled = {
render: (args: any) => (
<Tabs {...args} aria-label="History of Ancient Rome" styles={style({width: 450, height: 144})} disabledKeys={['FoR', 'MaR', 'Emp']}>
<TabList>
<Tab id="FoR" aria-label="Edit"><Edit /><Text>Edit</Text></Tab>
<Tab id="MaR" aria-label="Notifications"><Bell /><Text>Notifications</Text></Tab>
<Tab id="Emp" aria-label="Likes"><Heart /><Text>Likes</Text></Tab>
</TabList>
<TabPanel id="FoR">
Arma virumque cano, Troiae qui primus ab oris.
</TabPanel>
<TabPanel id="MaR">
Senatus Populusque Romanus.
</TabPanel>
<TabPanel id="Emp">
Alea jacta est.
</TabPanel>
</Tabs>
)
};

export const Icons = (args: any) => (
<Tabs {...args} styles={style({width: 208, height: 144})}>
<TabList aria-label="History of Ancient Rome">
<Tab id="FoR" aria-label="Edit"><Edit /></Tab>
<Tab id="MaR" aria-label="Notifications"><Bell /></Tab>
<Tab id="Emp" aria-label="Likes"><Heart /></Tab>
</TabList>
<TabPanel id="FoR">
Arma virumque cano, Troiae qui primus ab oris.
</TabPanel>
<TabPanel id="MaR">
Senatus Populusque Romanus.
</TabPanel>
<TabPanel id="Emp">
Alea jacta est.
</TabPanel>
</Tabs>
);
export const Icons = {
render: (args: any) => (
<Tabs {...args} aria-label="History of Ancient Rome" styles={style({width: 208, height: 144})} labelBehavior="hide">
<TabList>
<Tab id="FoR" aria-label="Edit"><Edit /><Text>Edit</Text></Tab>
<Tab id="MaR" aria-label="Notifications"><Bell /><Text>Notifications</Text></Tab>
<Tab id="Emp" aria-label="Likes"><Heart /><Text>Likes</Text></Tab>
</TabList>
<TabPanel id="FoR">
Arma virumque cano, Troiae qui primus ab oris.
</TabPanel>
<TabPanel id="MaR">
Senatus Populusque Romanus.
</TabPanel>
<TabPanel id="Emp">
Alea jacta est.
</TabPanel>
</Tabs>
)
};
1 change: 1 addition & 0 deletions packages/@react-spectrum/s2/intl/en-US.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
"table.sortAscending": "Sort Ascending",
"table.sortDescending": "Sort Descending",
"table.resizeColumn": "Resize column",
"tabs.selectorLabel": "Tab selector",
Copy link
Member Author

Choose a reason for hiding this comment

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

Do we need these additional strings? We didn't have these in v3. Is the label provided by the app enough?

Copy link
Member

Choose a reason for hiding this comment

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

linking to discussion about this #7600 (comment)

"tag.showAllButtonLabel": "Show all ({tagCount, number})",
"tag.hideButtonLabel": "Show less",
"tag.actions": "Actions",
Expand Down
1 change: 1 addition & 0 deletions packages/@react-spectrum/s2/intl/he-IL.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
"table.resizeColumn": "שנה את גודל העמודה",
"table.sortAscending": "מיין בסדר עולה",
"table.sortDescending": "מיין בסדר יורד",
"tabs.selectorLabel": "Tab selector",
"tag.actions": "פעולות",
"tag.hideButtonLabel": "הצג פחות",
"tag.noTags": "ללא",
Expand Down
1 change: 1 addition & 0 deletions packages/@react-spectrum/s2/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,7 @@
},
"dependencies": {
"@react-aria/collections": "3.0.0-alpha.7",
"@react-aria/focus": "^3.19.1",
"@react-aria/i18n": "^3.12.5",
"@react-aria/interactions": "^3.23.0",
"@react-aria/live-announcer": "^3.4.1",
Expand Down
Loading
Loading