Skip to content

Refactoring EuiKeyPadMenuItem beta badge for improved accessibility #5508

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

Closed
1Copenut wants to merge 8 commits into from
Closed

Refactoring EuiKeyPadMenuItem beta badge for improved accessibility #5508

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3258086
Refactoring EuiKeypadMenuItem beta badge for improved accessibility
Dec 28, 2021
c107d41
Adding a changelog entry for issue 5290.
Dec 28, 2021
8723f28
Refactoring CSS and adding JS guard check
Dec 29, 2021
1f09ad2
Refactoring ARIA label for localization.
Jan 4, 2022
6f679f3
Fixing a changelog conflict.
Jan 4, 2022
3744698
Refactoring to use a relatively positioned container.
Jan 11, 2022
ba06212
Refactoring for variable width parent, and fixed element width
Jan 12, 2022
a7d44b5
Removing unused variable and comments
Jan 13, 2022
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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

- Fixed a `EuiDataGrid` sizing bug which didn't account for a horizontal scrollbar ([#5478](https://github.com/elastic/eui/pull/5478))
- Fixed `EuiModalHeaderTitle` to conditionally wrap title strings in an H1 ([#5494](https://github.com/elastic/eui/pull/5494))
- Fixed `EuiKeypadMenuItem` accessibility issue where there was a nested focusable element ([#5508](https://github.com/elastic/eui/pull/5508))

**Deprecations**

Expand Down
2 changes: 1 addition & 1 deletion src/components/badge/beta_badge/beta_badge.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,7 @@ export const EuiBetaBadge: FunctionComponent<EuiBetaBadgeProps> = ({
content={tooltipContent}
title={title || label}
>
<span tabIndex={0} className={classes} {...rest}>
<span tabIndex={0} className={classes} role="button" {...rest}>

@1Copenut 1Copenut Dec 28, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I added this role="button" because Safari + VoiceOver wasn't consistently announcing the tooltip. Found this article Short note on aria-label, aria-labelledby, and aria-describedby by Leonie Watson that gave me a clue the span alone might not honor our ARIA markup consistently.

{icon || label}
</span>
</EuiToolTip>
Expand Down
138 changes: 74 additions & 64 deletions src/components/key_pad_menu/__snapshots__/key_pad_menu_item.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,96 +94,106 @@ exports[`EuiKeyPadMenuItem is rendered 1`] = `
`;

exports[`EuiKeyPadMenuItem props betaBadge renders 1`] = `
<button
class="euiKeyPadMenuItem euiKeyPadMenuItem--hasBetaBadge"
type="button"
>
<span
class="euiKeyPadMenuItem__inner"
Array [
<button
class="euiKeyPadMenuItem euiKeyPadMenuItem--hasBetaBadge"
type="button"
>
<span
class="euiBetaBadge euiBetaBadge--singleLetter euiBetaBadge--subdued euiBetaBadge--small euiKeyPadMenuItem__betaBadge"
title="Beta"
>
B
</span>
<span
class="euiKeyPadMenuItem__icon"
class="euiKeyPadMenuItem__inner"
>
Icon
</span>
<span
class="euiKeyPadMenuItem__label"
>
Label
<span
class="euiKeyPadMenuItem__icon"
>
Icon
</span>
<span
class="euiKeyPadMenuItem__label"
>
Label
</span>
</span>
</span>
</button>
</button>,
<span
aria-label="Label. Beta item."
class="euiBetaBadge euiBetaBadge--singleLetter euiBetaBadge--subdued euiBetaBadge--small euiKeyPadMenuItem__betaBadge"
title="Beta"
>
B
</span>,
]
`;

exports[`EuiKeyPadMenuItem props betaBadge renders with betaBadgeIconType 1`] = `
<button
class="euiKeyPadMenuItem euiKeyPadMenuItem--hasBetaBadge"
type="button"
>
<span
class="euiKeyPadMenuItem__inner"
Array [
<button
class="euiKeyPadMenuItem euiKeyPadMenuItem--hasBetaBadge"
type="button"
>
<span
class="euiBetaBadge euiBetaBadge--iconOnly euiBetaBadge--singleLetter euiBetaBadge--subdued euiBetaBadge--small euiKeyPadMenuItem__betaBadge"
title="Beta"
class="euiKeyPadMenuItem__inner"
>
<span
aria-hidden="true"
class="euiBetaBadge__icon"
color="inherit"
data-euiicon-type="bolt"
/>
</span>
<span
class="euiKeyPadMenuItem__icon"
>
Icon
class="euiKeyPadMenuItem__icon"
>
Icon
</span>
<span
class="euiKeyPadMenuItem__label"
>
Label
</span>
</span>
</button>,
<span
aria-label="Label. Beta item."
class="euiBetaBadge euiBetaBadge--iconOnly euiBetaBadge--singleLetter euiBetaBadge--subdued euiBetaBadge--small euiKeyPadMenuItem__betaBadge"
title="Beta"
>
<span
class="euiKeyPadMenuItem__label"
>
Label
</span>
</span>
</button>
aria-hidden="true"
class="euiBetaBadge__icon"
color="inherit"
data-euiicon-type="bolt"
/>
</span>,
]
`;

exports[`EuiKeyPadMenuItem props betaBadge renders with betaBadgeTooltipContent 1`] = `
<button
class="euiKeyPadMenuItem euiKeyPadMenuItem--hasBetaBadge"
type="button"
>
<span
class="euiKeyPadMenuItem__inner"
Array [
<button
class="euiKeyPadMenuItem euiKeyPadMenuItem--hasBetaBadge"
type="button"
>
<span
class="euiToolTipAnchor"
class="euiKeyPadMenuItem__inner"
>
<span
class="euiBetaBadge euiBetaBadge--singleLetter euiBetaBadge--subdued euiBetaBadge--small euiKeyPadMenuItem__betaBadge"
tabindex="0"
class="euiKeyPadMenuItem__icon"
>
B
Icon
</span>
<span
class="euiKeyPadMenuItem__label"
>
Label
</span>
</span>
</button>,
<span
class="euiToolTipAnchor"
>
<span
class="euiKeyPadMenuItem__icon"
>
Icon
</span>
<span
class="euiKeyPadMenuItem__label"
aria-label="Label. Beta item."
class="euiBetaBadge euiBetaBadge--singleLetter euiBetaBadge--subdued euiBetaBadge--small euiKeyPadMenuItem__betaBadge"
role="button"
tabindex="0"
>
Label
B
</span>
</span>
</button>
</span>,
]
`;

exports[`EuiKeyPadMenuItem props isDisabled renders with href 1`] = `
Expand Down
1 change: 1 addition & 0 deletions src/components/key_pad_menu/_key_pad_menu.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,5 +18,6 @@
> *:not(legend) {
margin-bottom: $euiKeyPadMenuMarginSize;
margin-right: $euiKeyPadMenuMarginSize;
position: relative;
}
}
13 changes: 9 additions & 4 deletions src/components/key_pad_menu/_key_pad_menu_item.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,10 +92,6 @@
.euiKeyPadMenuItem__checkableInput {
transform: scale(.75);
transform-origin: top right;
}

.euiKeyPadMenuItem__checkableInput,
.euiKeyPadMenuItem__betaBadge {
position: absolute;
top: $euiSizeXS;
right: $euiSizeXS;
Expand Down Expand Up @@ -126,3 +122,12 @@
border-color: $euiColorPrimaryText;
}
}

.euiKeyPadMenuListItem {

@1Copenut 1Copenut Dec 28, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Moved this out as a top-level element in the SCSS to ensure consistent absolute positioning and lessening risk of the cascade modifying something.

Comment thread
1Copenut marked this conversation as resolved.
Outdated
.euiToolTipAnchor {
position: absolute;
top: $euiSizeS;
right: $euiSizeS;
z-index: 3;
}
}
2 changes: 1 addition & 1 deletion src/components/key_pad_menu/key_pad_menu.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ export const EuiKeyPadMenu: FunctionComponent<EuiKeyPadMenuProps> = ({
) : (
<ul className={classes} {...rest}>
{React.Children.map(children, (child) => (
<li>{child}</li>
<li className="euiKeyPadMenuListItem">{child}</li>
Comment thread
1Copenut marked this conversation as resolved.
Outdated
))}
</ul>
);
Expand Down
2 changes: 1 addition & 1 deletion src/components/key_pad_menu/key_pad_menu_item.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ describe('EuiKeyPadMenuItem', () => {
</EuiKeyPadMenuItem>
);

$button.simulate('click');
$button.find('button').simulate('click');

expect(onClickHandler).toBeCalledTimes(1);
});
Expand Down
24 changes: 14 additions & 10 deletions src/components/key_pad_menu/key_pad_menu_item.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,7 @@ export const EuiKeyPadMenuItem: FunctionComponent<EuiKeyPadMenuItemProps> = ({

return (
<EuiBetaBadge
aria-label={`${label}. Beta item.`}

@chandlerprall chandlerprall Dec 29, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Any hard coded text should use the useEuiI18n utility to allow localization.

@chandlerprall chandlerprall Dec 29, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@cchaos does this labelling match the intent of specifying an iconType?

@cchaos cchaos Jan 6, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

My guess is that we should actually be labelling button as beta not the badge. And that we can’t hard-code “Beta item” at all, that the beta label needs to come from the betaBadgeLabel that the consumer supplies since it's not always going to mean "Beta" specifically.

@1Copenut I think we need to re-think this solution. Mainly, one of the issues I'm seeing is that if a consumer were to use EuiKeyPadMenuItem by itself, this beta badge would no longer be poisitioned against the button, but whatever next parent is position: relative.

Would it be a better solution to revert back the styling to include the beta badge within the button, but aria-hidden it and just apply the betaBadgeLabel as an extra aria-describedBy?

@1Copenut 1Copenut Jan 6, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thank you @cchaos for this use case my solution hadn't anticipated. I'll give it another pass in a new branch with the approach you outlined and keep things in sync here so we don't lose the history.

I talked about this with Chandler in our 1:1 yesterday. Refactored it to add a relatively positioned DIV. I couldn't keep the beta badge nested inside the button because the axe plugin was complaining about a nested element, and hiding it with aria-hidden="true" would have removed the tooltip context for screen readers.

Moving the beta badge out to a sibling of the button felt like the best approach to balance all concerns.

size="s"
color="subdued"
className="euiKeyPadMenuItem__betaBadge"
Expand All @@ -238,7 +239,7 @@ export const EuiKeyPadMenuItem: FunctionComponent<EuiKeyPadMenuItemProps> = ({

const renderContent = () => (
<span className="euiKeyPadMenuItem__inner">
{checkable ? renderCheckableElement() : renderBetaBadge()}
{checkable && renderCheckableElement()}

@cchaos cchaos Jan 11, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The checkable check is already in renderCheckableElement()

Suggested change
{checkable && renderCheckableElement()}
{renderCheckableElement()}

<span className="euiKeyPadMenuItem__icon">{children}</span>
<span className="euiKeyPadMenuItem__label">{label}</span>
</span>
Expand Down Expand Up @@ -269,14 +270,17 @@ export const EuiKeyPadMenuItem: FunctionComponent<EuiKeyPadMenuItemProps> = ({
}

return (
<Element
className={classes}
{...(relObj as ElementType)}
{...(rest as ElementType)}
// Unable to get past `LegacyRef` conflicts
ref={buttonRef as Ref<any>}
>
{renderContent()}
</Element>
<React.Fragment>
<Element
className={classes}
{...(relObj as ElementType)}
{...(rest as ElementType)}
// Unable to get past `LegacyRef` conflicts
ref={buttonRef as Ref<any>}
>
{renderContent()}
</Element>
{betaBadgeLabel && renderBetaBadge()}
Comment thread
chandlerprall marked this conversation as resolved.
Outdated
</React.Fragment>
);
};