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

fix: menu item a11y for disable items #2628

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+14 −4
Open

fix: menu item a11y for disable items #2628

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d18bc45
menu item a11y for disable items
doronbrikman Dec 5, 2024
16ac990
Merge branch 'master' into master
doronbrikman Dec 15, 2024
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
2 changes: 1 addition & 1 deletion packages/core/src/components/Menu/Menu/utils/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { ReactElement } from "react";
import { MenuChild } from "../MenuConstants";

export function isMenuChildSelectable(child: MenuChild): boolean {
return !!child.type.isSelectable && !child.props.disabled;
return !!child.type.isSelectable;
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we only want it to be selectable if it is disabled and has a tooltip (but should not be selectable when it is disabled without a tooltip), the check for disabled should stay, and there should be an additional check if has tooltip.

}

export const generateMenuItemId = (menuId: string, child: ReactElement, index: number) => {
Expand Down
5 changes: 5 additions & 0 deletions packages/core/src/components/Menu/MenuItem/MenuItem.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,8 @@ export interface MenuItemProps extends VibeComponentProps {
splitMenuItem?: boolean;
"aria-label"?: AriaAttributes["aria-label"];
submenuPosition?: SubmenuPosition;
tooltipId?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of adding an id, can be implemented using the id from tooltipProps

shouldShowTooltip?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this prop? there is already logic in the component for this here.

}

export interface MenuItemTitleComponentProps extends Omit<MenuItemProps, "title"> {
Expand Down Expand Up @@ -121,6 +123,7 @@ const MenuItem: VibeComponent<MenuItemProps | MenuItemTitleComponentProps> & {
content={shouldShowTooltip ? finalTooltipContent : null}
position={tooltipPosition}
showDelay={tooltipShowDelay}
childId={baseMenuProps.id}
Copy link
Contributor

Choose a reason for hiding this comment

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

Tooltip has an id prop do we need to add a childId?

{...tooltipProps}
>
<BaseMenuItem
Expand All @@ -130,6 +133,8 @@ const MenuItem: VibeComponent<MenuItemProps | MenuItemTitleComponentProps> & {
className={className}
disabled={disabled}
selected={selected}
tooltipId={tooltipProps?.id}
shouldShowTooltip={!!shouldShowTooltip}
Comment on lines +136 to +137
Copy link
Contributor

Choose a reason for hiding this comment

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

In order to simplify it it might be clearer to add aria-disabled and aria-describedby props instead of the tooltip specific props so that the logic can be done here.

{...baseMenuProps}
>
{Boolean(icon) && (
Expand Down
6 changes: 5 additions & 1 deletion packages/core/src/components/Menu/MenuItem/components/BaseMenuItem/BaseMenuItem.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,9 @@ const BaseMenuItem = forwardRef(
"data-testid": dataTestId,
splitMenuItem = false,
children,
submenuPosition = "right"
submenuPosition = "right",
tooltipId,
shouldShowTooltip = false
}: BaseMenuItemProps,
ref: React.ForwardedRef<HTMLElement>
) => {
Expand Down Expand Up @@ -122,6 +124,8 @@ const BaseMenuItem = forwardRef(
type="text2"
aria-haspopup={subMenu ? true : undefined}
aria-expanded={subMenu ? shouldShowSubMenu : undefined}
aria-disabled={disabled}
aria-describedby={shouldShowTooltip ? tooltipId || `${id}-tooltip` : undefined}
data-testid={dataTestId || getTestId(ComponentDefaultTestId.MENU_ITEM, index)}
className={cx(styles.item, className, {
[styles.disabled]: disabled,
Expand Down
5 changes: 3 additions & 2 deletions packages/core/src/components/Tooltip/Tooltip.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ interface TooltipBaseProps extends VibeComponentProps {
* how much delay should the Dialog wait until it should trigger the show in MS
*/
showDelay?: number;
childId?: string;
disableDialogSlide?: boolean;
animationType?: DialogAnimationType;
withoutDialog?: boolean;
Expand Down Expand Up @@ -178,7 +179,7 @@ export default class Tooltip extends PureComponent<TooltipProps> {
}

renderTooltipContent() {
const { theme, content, className, style, maxWidth, title, image, icon } = this.props;
const { theme, content, className, style, maxWidth, title, image, icon, id, childId } = this.props;
if (!content) {
// don't render empty tooltip
return null;
Expand All @@ -205,7 +206,7 @@ export default class Tooltip extends PureComponent<TooltipProps> {
className={cx(styles.tooltip, getStyle(styles, camelCase(theme)), className)}
>
{image && <img className={styles.image} src={image} alt="" />}
<div className={cx(styles.content)}>
<div id={id || `${childId}-tooltip`} className={cx(styles.content)}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Would the id be enough? why need to add childId?

{title && (
<Flex gap="xs">
{icon && <Icon iconSize="20" icon={icon} />}
Expand Down