Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 4 additions & 0 deletions packages/eui/changelogs/upcoming/9140.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
**Accessibility**

- Improved the accessibility experience of `EuiBasicTable` and `EuiInMemoryTable` by ensuring tooltips displayed for action buttons are visual-only whenever the `action.name` and `action.description` are exactly the same

Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
waitForEuiPopoverOpen,
waitForEuiPopoverClose,
waitForEuiToolTipVisible,
waitForEuiToolTipHidden,
} from '../../test/rtl';

import { CollapsedItemActions } from './collapsed_item_actions';
Expand Down Expand Up @@ -156,4 +157,54 @@ describe('CollapsedItemActions', () => {
fireEvent.click(getByTestSubject('customAction'));
await waitForEuiPopoverClose();
});

// If `name` and `description` are exactly the same
// we don't want screen readers announcing the same text twice
test('tooltip screen-reader output', async () => {
const props = {
actions: [
{
name: 'same',
description: 'different',
onClick: () => {},
'data-test-subj': 'different',
},
{
name: 'same',
description: 'same',
onClick: () => {},
'data-test-subj': 'same',
},
],
itemId: 'id',
item: { id: 'abc' },
actionsDisabled: false,
displayedRowIndex: 0,
};

const { getByTestSubject, getByRole } = render(
<CollapsedItemActions {...props} />
);
fireEvent.click(getByTestSubject('euiCollapsedItemActionsButton'));
await waitForEuiPopoverOpen();

const actionDifferent = getByTestSubject('different');
fireEvent.mouseOver(actionDifferent);
await waitForEuiToolTipVisible();
const tooltipDifferent = getByRole('tooltip');
expect(tooltipDifferent).toHaveTextContent('different');
expect(actionDifferent).toHaveAttribute('aria-describedby');
expect(actionDifferent.getAttribute('aria-describedby')).toEqual(
tooltipDifferent.id
);
fireEvent.mouseOut(actionDifferent);
await waitForEuiToolTipHidden();

const actionSame = getByTestSubject('same');
fireEvent.mouseOver(actionSame);
await waitForEuiToolTipVisible();
const tooltipSame = getByRole('tooltip');
expect(tooltipSame).toHaveTextContent('same');
expect(actionSame).not.toHaveAttribute('aria-describedby');
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,13 @@ export const CollapsedItemActions = <T extends {}>({
if (!event.isPropagationStopped()) closePopover();
}}
toolTipContent={toolTipContent}
toolTipProps={{ delay: 'long' }}
toolTipProps={{
delay: 'long',
// Avoid screen-readers announcing the same text twice
disableScreenReaderOutput:
Copy link
Contributor

Choose a reason for hiding this comment

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

Non blocking suggestion:
Looking at the documentation, I feel like we could add some information about the expected usage to ensure accessibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's a really good point, I will try and something in this regard to the docs 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added an admonition in 667eac0

please let me know what you think 🙏


image

Copy link
Contributor

Choose a reason for hiding this comment

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

💭 The main issue I have with this is that this is a callout and we're placing too much content in it. Callouts should just be text.

How about we add an "Accessibility" section with admonition and snippet instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in 667eac0

(feedback and suggestions very much appreciated 🙇)

typeof buttonContent === 'string' &&
buttonContent === toolTipContent,
}}
>
{buttonContent}
</EuiContextMenuItem>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,59 @@ describe('DefaultItemAction', () => {
expect(getByText('goodbye tooltip')).toBeInTheDocument();
});

it('is described by the tooltip via aria-describedby', async () => {
const actionWithDifferentNameAndDescription: EmptyButtonAction<Item> = {
name: 'same',
description: 'different',
'data-test-subj': 'different',
type: 'button',
onClick: () => {},
};
const { getByTestSubject, getByRole } = render(
<DefaultItemAction
action={actionWithDifferentNameAndDescription}
enabled={true}
item={{ id: 'differentId' }}
/>
);

const action = getByTestSubject('different');
fireEvent.mouseOver(action);
await waitForEuiToolTipVisible();
const tooltip = getByRole('tooltip');
expect(tooltip).toHaveTextContent('different');
expect(tooltip).toBeInTheDocument();
expect(action).toHaveAttribute('aria-describedby');
expect(action.getAttribute('aria-describedby')).toEqual(tooltip.id);
});

// If `name` and `description` are exactly the same
// we don't want screen readers announcing the same text twice
it('has visual-only tooltip when `name` equals `description`', async () => {
const actionWithEqualNameAndDescription: EmptyButtonAction<Item> = {
name: 'same',
description: 'same',
'data-test-subj': 'same',
type: 'button',
onClick: () => {},
};
const { getByTestSubject, getByRole } = render(
<DefaultItemAction
action={actionWithEqualNameAndDescription}
enabled={true}
item={{ id: 'sameId' }}
/>
);

const action = getByTestSubject('same');
fireEvent.mouseOver(action);
await waitForEuiToolTipVisible();
const tooltip = getByRole('tooltip');
expect(tooltip).toBeInTheDocument();
expect(tooltip).toHaveTextContent('same');
expect(action).not.toHaveAttribute('aria-describedby');
});

it('passes back the original click event as well as the row item to onClick', () => {
const onClick = jest.fn((item, event) => {
event.preventDefault();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import {
EuiButtonEmptyProps,
EuiButtonIconProps,
} from '../button';
import { EuiToolTip } from '../tool_tip';
import { EuiToolTip, EuiToolTipProps } from '../tool_tip';
import { useGeneratedHtmlId } from '../../services/accessibility';
import { EuiScreenReaderOnly } from '../accessibility';

Expand Down Expand Up @@ -58,6 +58,13 @@ export const DefaultItemAction = <T extends object>({
: undefined;
const actionContent = callWithItemIfFunction(item)(action.name);
const tooltipContent = callWithItemIfFunction(item)(action.description);
const tooltipProps: Omit<EuiToolTipProps, 'position' | 'children'> = {
content: tooltipContent,
delay: 'long',
// Avoid screen-readers announcing the same text twice
disableScreenReaderOutput:
typeof actionContent === 'string' && actionContent === tooltipContent,
};
const href = callWithItemIfFunction(item)(action.href);
const dataTestSubj = callWithItemIfFunction(item)(action['data-test-subj']);

Expand Down Expand Up @@ -114,9 +121,7 @@ export const DefaultItemAction = <T extends object>({

return enabled ? (
<>
<EuiToolTip content={tooltipContent} delay="long">
{button}
</EuiToolTip>
<EuiToolTip {...tooltipProps}>{button}</EuiToolTip>
{/* SR text has to be rendered outside the tooltip,
otherwise EuiToolTip's own aria-labelledby won't properly clone */}
{ariaLabelledBy}
Expand Down
15 changes: 15 additions & 0 deletions packages/website/docs/components/tables/basic.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,21 @@ Actions follow these UI/UX rules:
- Actions become semi-transparent when you hover over the row. With more than 2 actions, only the ellipsis button stays visible at all times.
- When one or more rows are selected, all individual actions are disabled. Users should use bulk actions outside the table instead.

### Accessibility

`action.name` and `action.description` should be different. The name is a short identifier, while the description should explain the purpose of the action or provide hints.

```jsx
const action = {
name: 'User profile',
description:
({ firstName, lastName }) => `Visit ${firstName} ${lastName}'s profile`,
// ...
};
```

If a meaningful description for an action is not possible, setting the exact same value for `action.name` and `action.description` as a `string` will ensure the button's tooltip is visual-only and the text is not announced to screen-readers twice.

```tsx interactive
import React, { useState, useMemo } from 'react';
import {
Expand Down