Dropdown menus - option selection : add aria-hidden on icons #1036
Conversation
AntoLC
force-pushed
the
fix/967
branch
7 times, most recently
from
02b616e to
8e7988b
Compare
AntoLC
changed the title
Add aria-hidden attribute to Icon component to improve accessibility. Add aria-label to DropdownMenu component when selected.
| @@ -41,7 +41,7 @@ export const DocShareModalFooter = ({ | |||
| fullWidth={false} | |||
| onClick={copyDocLink} | |||
| color="tertiary" | |||
| icon={<span className="material-icons">add_link</span>} | |||
| icon={<Icon iconName="add_link" />} | |||
There was a problem hiding this comment.
| icon={<Icon iconName="add_link" />} | |
| icon={<Icon iconName="add_link" aria-hidden="true" />} |
| @@ -14,6 +14,7 @@ export const Icon = ({ | |||
| }: IconProps) => { | |||
There was a problem hiding this comment.
Make the Icon component more accessible by adding:
in the IconProps Type :
ariaLabel?: string;
ariaHidden?: boolean;
role?: string;
| }: IconProps) => { | |
| ariaLabel, | |
| ariaHidden, | |
| role, | |
| }: IconProps) => { |
There was a problem hiding this comment.
Just a quick note: aria-hidden={!!!textProps['aria-label']} works but is a bit hard to read and not very flexible.
It might be cleaner to accept explicit ariaLabel, ariaHidden, and role props. That would make the intent clearer and help in cases where we want to force aria-hidden={false} (e.g. for selected items).
Happy to discuss it further if neededfeel free to reach out!
I’ll suggest a snippet below feel free to use it or adapt it as you see fit.
| @@ -14,6 +14,7 @@ export const Icon = ({ | |||
| }: IconProps) => { | |||
| return ( | |||
| <Text | |||
| aria-hidden={!!!textProps['aria-label']} | |||
There was a problem hiding this comment.
| aria-hidden={!!!textProps['aria-label']} | |
| aria-label={ariaLabel} | |
| aria-hidden={computedAriaHidden} | |
| role={computedRole} |
| @@ -14,6 +14,7 @@ export const Icon = ({ | |||
| }: IconProps) => { | |||
There was a problem hiding this comment.
| }: IconProps) => { | |
| }: IconProps) => { | |
| const computedAriaHidden = ariaHidden ?? !ariaLabel; | |
| const computedRole = !computedAriaHidden ? role ?? 'img' : undefined; |
| /> | ||
| )} | ||
| <DropdownMenu options={options}> | ||
| <DropdownMenu options={options} label={t('Open the document options')}> | ||
| <IconOptions | ||
| isHorizontal | ||
| $theme="primary" |
There was a problem hiding this comment.
Since the DropdownMenu now has a label prop, the aria-label on IconOptions is no longer needed and creates redundancy. ( line 275 )
I'd suggest removing it and letting the menu handle accessibility.
There was a problem hiding this comment.
Totally optional: you could create a variable like
const exportButtonLabel = 'Export the document'
to avoid repeating the string and make future changes easier.
Not a blocker at all, just a small DX improvement suggestion :)
| @@ -18,7 +18,7 @@ test.describe('Doc Export', () => { | |||
| await createDoc(page, 'doc-editor', browserName, 1); | |||
| await page | |||
| .getByRole('button', { | |||
| name: 'download', | |||
| name: 'Export the document', | |||
There was a problem hiding this comment.
| name: 'Export the document', | |
| name: exportButtonLabel, |
| ).toBeHidden(); | ||
|
|
||
| const memberCard = shareModal.getByLabel('List members card'); | ||
| await expect(memberCard.getByText('test@accesses.test')).toBeVisible(); | ||
| await expect(memberCard.getByLabel('doc-role-text')).toBeVisible(); | ||
| await expect( | ||
| memberCard.getByRole('button', { name: 'more_horiz' }), | ||
| memberCard.getByRole('button', { name: 'Open the member options' }), |
There was a problem hiding this comment.
Same as above, extract into a variable if you'd like.
ajout aria-hidden sur les icônes + aria-selected pour le dropdown du document sharing
Issue associée : #967