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.

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

[EuiCollapsibleNavBeta] Improve collapsed button icon screen reader UX #7055

Merged
merged 11 commits into from
Aug 11, 2023
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ exports[`EuiCollapsibleNavItem renders a collapsed button icon when in a collaps
data-test-subj="test subject string"
>
<button
aria-label="Item"
class="euiButtonIcon euiCollapsedNavButton emotion-euiButtonIcon-s-empty-text-euiCollapsedNavButton"
data-test-subj="euiCollapsedNavButton"
type="button"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ exports[`EuiCollapsedNavButton renders a tooltip around the icon button 1`] = `
data-test-subj="test subject string"
>
<button
aria-describedby="generated-id"
aria-label="Nav item"
class="euiButtonIcon euiCollapsedNavButton emotion-euiButtonIcon-s-empty-text-euiCollapsedNavButton"
data-test-subj="euiCollapsedNavButton"
type="button"
Expand Down Expand Up @@ -54,6 +54,7 @@ exports[`EuiCollapsedNavButton renders isSelected 1`] = `
class="euiToolTipAnchor emotion-euiToolTipAnchor-block"
>
<a
aria-label="Nav item"
class="euiButtonIcon euiCollapsedNavButton emotion-euiButtonIcon-s-empty-text-euiCollapsedNavButton-isSelected"
data-test-subj="euiCollapsedNavButton"
href="#"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ exports[`EuiCollapsedNavItem renders a popover if items exist 1`] = `
class="euiToolTipAnchor emotion-euiToolTipAnchor-block"
>
<button
aria-label="Item"
class="euiButtonIcon euiCollapsedNavButton emotion-euiButtonIcon-s-empty-text-euiCollapsedNavButton"
data-test-subj="euiCollapsedNavButton"
type="button"
Expand All @@ -35,6 +36,7 @@ exports[`EuiCollapsedNavItem renders an icon button/link if items are missing or
data-test-subj="test subject string"
>
<a
aria-label="Item"
class="euiButtonIcon euiCollapsedNavButton emotion-euiButtonIcon-s-empty-text-euiCollapsedNavButton"
data-test-subj="euiCollapsedNavButton"
href="#"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ exports[`EuiCollapsedNavPopover renders the title and sub-items within the popov
class="euiToolTipAnchor emotion-euiToolTipAnchor-block"
>
<button
aria-label="Item"
class="euiButtonIcon euiCollapsedNavButton emotion-euiButtonIcon-s-empty-text-euiCollapsedNavButton"
data-test-subj="euiCollapsedNavButton"
type="button"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ export const EuiCollapsedNavButton: FunctionComponent<
color="text"
href={href}
onClick={onClick}
aria-label={title}
{...(linkProps as EuiButtonIconPropsForAnchor)} // Exclusive union shenanigans
className={buttonClassName}
css={buttonCssStyles}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,7 @@
* Side Public License, v 1.
*/

import React, {
FunctionComponent,
ReactNode,
HTMLAttributes,
useContext,
} from 'react';
import React, { FunctionComponent, HTMLAttributes, useContext } from 'react';
import classNames from 'classnames';

import { useEuiTheme } from '../../../services';
Expand Down Expand Up @@ -65,11 +60,11 @@ export type _SharedEuiCollapsibleNavItemProps = HTMLAttributes<HTMLElement> &

export type EuiCollapsibleNavItemProps = {
/**
* ReactNode to render as this component's title
* Required Text to render as the nav item title
cee-chen marked this conversation as resolved.
Show resolved Hide resolved
*/
title: ReactNode;
title: string;
/**
* Allows customizing title's element.
* Allows customizing the title element.
* Consider using a heading element for better accessibility.
* Defaults to an unsemantic `span` or `div`, depending on context.
*/
Expand Down
32 changes: 32 additions & 0 deletions src/components/tool_tip/tool_tip.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,8 @@ describe('EuiToolTip', () => {
).not.toBeNull();

jest.useRealTimers();

component.unmount(); // Enzyme's portals stay in the DOM otherwise
});

test('display prop renders block', () => {
Expand Down Expand Up @@ -150,6 +152,36 @@ describe('EuiToolTip', () => {
expect(removeEventSpy).toHaveBeenCalledWith('scroll', repositionFn, true);
});

describe('aria-describedby', () => {
it('by default, sets an `aria-describedby` on the anchor when the tooltip is visible', async () => {
const { getByTestSubject } = render(
<EuiToolTip content="Tooltip content" id="toolTipId">
<button data-test-subj="anchor" />
</EuiToolTip>
);
fireEvent.mouseOver(getByTestSubject('anchor'));
await waitForEuiToolTipVisible();

expect(
getByTestSubject('anchor').getAttribute('aria-describedby')
).toEqual('toolTipId');
});

it('merges with custom consumer `aria-describedby`s', async () => {
const { getByTestSubject } = render(
<EuiToolTip content="Tooltip content" id="toolTipId">
<button data-test-subj="anchor" aria-describedby="customId" />
</EuiToolTip>
);
fireEvent.mouseOver(getByTestSubject('anchor'));
await waitForEuiToolTipVisible();

expect(
getByTestSubject('anchor').getAttribute('aria-describedby')
).toEqual('toolTipId customId');
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this order of concatenation. Screen readers key off IDs in order, so it'll always read our described by text first, then additional.

});
});

describe('ref methods', () => {
// Although we don't publicly recommend it, consumers may need to reach into EuiToolTip
// class methods to manually control visibility state via `show/hideToolTip`.
Expand Down
22 changes: 5 additions & 17 deletions src/components/tool_tip/tool_tip.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import React, {
} from 'react';
import classNames from 'classnames';

import { CommonProps, keysOf } from '../common';
import { CommonProps } from '../common';
import { findPopoverPosition, htmlIdGenerator } from '../../services';
import { enqueueStateChange } from '../../services/react';
import { EuiResizeObserver } from '../observer/resize_observer';
Expand All @@ -26,14 +26,8 @@ import { EuiToolTipAnchor } from './tool_tip_anchor';
import { EuiToolTipArrow } from './tool_tip_arrow';
import { toolTipManager } from './tool_tip_manager';

const positionsToClassNameMap: { [key in ToolTipPositions]: string } = {
top: 'euiToolTip--top',
right: 'euiToolTip--right',
bottom: 'euiToolTip--bottom',
left: 'euiToolTip--left',
};

export const POSITIONS = keysOf(positionsToClassNameMap);
export const POSITIONS = ['top', 'right', 'bottom', 'left'] as const;
const DISPLAYS = ['inlineBlock', 'block'] as const;

export type ToolTipDelay = 'regular' | 'long';

Expand All @@ -50,11 +44,6 @@ interface ToolTipStyles {
visibility?: 'hidden';
}

const displayToClassNameMap = {
inlineBlock: undefined,
block: 'euiToolTipAnchor--displayBlock',
};

const DEFAULT_TOOLTIP_STYLES: ToolTipStyles = {
// position the tooltip content near the top-left
// corner of the window so it can't create scrollbars
Expand Down Expand Up @@ -92,7 +81,7 @@ export interface EuiToolTipProps extends CommonProps {
/**
* Common display alternatives for the anchor wrapper
*/
display?: keyof typeof displayToClassNameMap;
display?: (typeof DISPLAYS)[number];
/**
* Delay before showing tooltip. Good for repeatable items.
*/
Expand All @@ -116,7 +105,6 @@ export interface EuiToolTipProps extends CommonProps {
* When nesting an `EuiTooltip` in a scrollable container, `repositionOnScroll` should be `true`
*/
repositionOnScroll?: boolean;

/**
* If supplied, called when mouse movement causes the tool tip to be
* hidden.
Expand Down Expand Up @@ -337,7 +325,7 @@ export class EuiToolTip extends Component<EuiToolTipProps, State> {
onFocus={this.onFocus}
onMouseOver={this.showToolTip}
onMouseOut={this.onMouseOut}
id={this.state.id}
id={id}
className={anchorClasses}
display={display!}
isVisible={visible}
Expand Down
32 changes: 14 additions & 18 deletions src/components/tool_tip/tool_tip_anchor.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,27 +6,21 @@
* Side Public License, v 1.
*/

import React, {
cloneElement,
HTMLAttributes,
ReactElement,
forwardRef,
} from 'react';
import React, { cloneElement, HTMLAttributes, forwardRef } from 'react';
import classNames from 'classnames';

import type { EuiToolTipProps } from './tool_tip';
import { euiToolTipAnchorStyles } from './tool_tip.styles';

export interface EuiToolTipAnchorProps
extends Omit<
HTMLAttributes<HTMLSpanElement>,
'onBlur' | 'onFocus' | 'children'
> {
onBlur: () => void;
onFocus: () => void;
children: ReactElement;
isVisible: boolean;
display: 'block' | 'inlineBlock';
}
export type EuiToolTipAnchorProps = Omit<
HTMLAttributes<HTMLSpanElement>,
'onBlur' | 'onFocus' | 'children'
> &
Required<Pick<EuiToolTipProps, 'display' | 'children'>> & {
onBlur: () => void;
onFocus: () => void;
isVisible: boolean;
};

export const EuiToolTipAnchor = forwardRef<
HTMLSpanElement,
Expand Down Expand Up @@ -79,7 +73,9 @@ export const EuiToolTipAnchor = forwardRef<
onBlur();
children.props.onBlur && children.props.onBlur(e);
},
...(isVisible && { 'aria-describedby': id }),
'aria-describedby': isVisible
? classNames(id, children.props['aria-describedby'])
: children.props['aria-describedby'],
})}
</span>
);
Expand Down
3 changes: 3 additions & 0 deletions upcoming_changelogs/7055.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
**Bug fixes**

- Fixed `EuiToolTip` overriding instead of merging its `aria-describedby` tooltip ID with any existing `aria-describedby`s