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

[EuiBasicTable][EuiInMemoryTable] Enable more action props to accept an optional callback + fix missing tooltips on collapsed actions #7373

Merged
merged 11 commits into from
Nov 20, 2023
Merged

[EuiBasicTable][EuiInMemoryTable] Enable more action props to accept an optional callback + fix missing tooltips on collapsed actions #7373

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
792b31b
Allow description to be a callback that changes depending on `item`
cee-chen Nov 16, 2023
bf1aaec
Let other props that seem useful take callbacks as well, and DRY out …
cee-chen Nov 16, 2023
b038a8c
[tech debt] misc `DefaultItemAction` cleanup
cee-chen Nov 16, 2023
6cf4211
[setup] DRY out utils to `action_types`
cee-chen Nov 17, 2023
26b2a79
Update default actions in collapsed popover
cee-chen Nov 17, 2023
af61c9b
Fix collapsed popovers not displaying descriptions as tooltips
cee-chen Nov 17, 2023
57e6003
Fix collapsed action to have the same tooltip delay as non-collapsed …
cee-chen Nov 17, 2023
9768b5a
[docs] Add examples of callback props
cee-chen Nov 17, 2023
87abdd3
Fix tooltip aria-describedby ID not correctly applying to buttons due…
cee-chen Nov 17, 2023
f3da152
Fix primary actions not correctly becoming visible on keyboard focus
cee-chen Nov 17, 2023
32bce13
[EuiContextMenu] Fixed missing underlines on hover
cee-chen Nov 17, 2023
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 changelogs/upcoming/7373.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,7 @@

- Deprecated `EuiContextMenuItem`'s `toolTipTitle` prop. Use `toolTipProps.title` instead
- Deprecated `EuiContextMenuItem`'s `toolTipPosition` prop. Use `toolTipProps.position` instead

**Accessibility**

- Fixed `EuiBasicTable` and `EuiInMemoryTable` actions not correctly reading out action descriptions to screen readers
34 changes: 18 additions & 16 deletions src/components/basic_table/__snapshots__/default_item_action.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,22 +21,24 @@ exports[`DefaultItemAction renders an EuiButtonEmpty when \`type="button" 1`] =
</span>
`;

exports[`DefaultItemAction renders an EuiButtonIcon when \`type="icon"\` 1`] = `
<span
class="euiToolTipAnchor emotion-euiToolTipAnchor-inlineBlock"
>
<button
aria-labelledby="generated-id"
class="euiButtonIcon emotion-euiButtonIcon-xs-empty-primary"
type="button"
exports[`DefaultItemAction renders an EuiButtonIcon with screen reader text when \`type="icon"\` 1`] = `
<div>
<span
class="euiToolTipAnchor emotion-euiToolTipAnchor-inlineBlock"
>
<span
aria-hidden="true"
class="euiButtonIcon__icon"
color="inherit"
data-euiicon-type="trash"
/>
</button>
<button
aria-labelledby="generated-id"
class="euiButtonIcon emotion-euiButtonIcon-xs-empty-primary"
type="button"
>
<span
aria-hidden="true"
class="euiButtonIcon__icon"
color="inherit"
data-euiicon-type="trash"
/>
</button>
</span>
<span
class="emotion-euiScreenReaderOnly"
id="generated-id"
Expand All @@ -45,5 +47,5 @@ exports[`DefaultItemAction renders an EuiButtonIcon when \`type="icon"\` 1`] = `
action1
</span>
</span>
</span>
</div>
`;
4 changes: 2 additions & 2 deletions src/components/basic_table/default_item_action.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ describe('DefaultItemAction', () => {
expect(container.firstChild).toMatchSnapshot();
});

it('renders an EuiButtonIcon when `type="icon"`', () => {
it('renders an EuiButtonIcon with screen reader text when `type="icon"`', () => {
const action: IconButtonAction<Item> = {
name: <span>action1</span>,
description: 'action 1',
Expand All @@ -59,7 +59,7 @@ describe('DefaultItemAction', () => {

const { container } = render(<DefaultItemAction {...props} />);

expect(container.firstChild).toMatchSnapshot();
expect(container).toMatchSnapshot();
});

it('renders an EuiButtonEmpty if no type is specified', () => {
Expand Down
57 changes: 34 additions & 23 deletions src/components/basic_table/default_item_action.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
* Side Public License, v 1.
*/

import React, { ReactElement } from 'react';
import React, { ReactElement, ReactNode } from 'react';

import { isString } from '../../services/predicate';
import {
Expand Down Expand Up @@ -63,29 +63,32 @@ export const DefaultItemAction = <T,>({
const dataTestSubj = callWithItemIfFunction(item)(action['data-test-subj']);

const ariaLabelId = useGeneratedHtmlId();
let ariaLabelledBy: ReactNode;

if (action.type === 'icon') {
if (!icon) {
throw new Error(`Cannot render item action [${action.name}]. It is configured to render as an icon but no
icon is provided. Make sure to set the 'icon' property of the action`);
}
button = (
<>
<EuiButtonIcon
className={className}
aria-labelledby={ariaLabelId}
isDisabled={!enabled}
color={color}
iconType={icon}
onClick={onClick}
href={href}
target={action.target}
data-test-subj={dataTestSubj}
/>
{/* actionContent (action.name) is a ReactNode and must be rendered to an element and referenced by ID for screen readers */}
<EuiScreenReaderOnly>
<span id={ariaLabelId}>{actionContent}</span>
</EuiScreenReaderOnly>
</>
<EuiButtonIcon
className={className}
aria-labelledby={ariaLabelId}
isDisabled={!enabled}
color={color}
iconType={icon}
onClick={onClick}
href={href}
target={action.target}
data-test-subj={dataTestSubj}
/>
);
// actionContent (action.name) is a ReactNode and must be rendered
// to an element and referenced by ID for screen readers
ariaLabelledBy = (
<EuiScreenReaderOnly>
<span id={ariaLabelId}>{actionContent}</span>
</EuiScreenReaderOnly>
Comment on lines +91 to +94
Copy link
Contributor

Choose a reason for hiding this comment

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

This will go a long way to ensuring screen reader users have context when tooltips appear. Users will know exactly what their next keypress will do in terms of adding / editing / deleting.

Copy link
Contributor

Choose a reason for hiding this comment

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

VoiceOver on Safari has changed the way it announces and handles aria-describedby and I haven't found much to explain why. This combination doesn't announce the tooltip text at all, though VoiceOver + Firefox does. Windows screen readers announce the tooltips properly.

The only thing I could find was this issue ticket filed last year that references the WCAG samples not working correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed this as well when I was testing tooltips on Safari+VO! But I found the same behavior on the EuiToolTip page as well so I concluded it wasn't my changes causing it 😬 I do worry a bit that maybe something we did on EUI caused it, let me try to dig into it a bit more separately from this PR

);
} else {
button = (
Expand All @@ -106,11 +109,19 @@ export const DefaultItemAction = <T,>({
);
}

return enabled && tooltipContent ? (
<EuiToolTip content={tooltipContent} delay="long">
{button}
</EuiToolTip>
return enabled ? (
<>
<EuiToolTip content={tooltipContent} delay="long">
{button}
</EuiToolTip>
{/* SR text has to be rendered outside the tooltip,
otherwise EuiToolTip's own aria-labelledby won't properly clone */}
{ariaLabelledBy}
</>
) : (
button
<>
{button}
{ariaLabelledBy}
</>
);
};