fix: menu item a11y for disable items #2628
Conversation
doronbrikman
changed the title
| @@ -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; | |||
There was a problem hiding this comment.
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.
| @@ -56,6 +56,8 @@ export interface MenuItemProps extends VibeComponentProps { | |||
| splitMenuItem?: boolean; | |||
| "aria-label"?: AriaAttributes["aria-label"]; | |||
| submenuPosition?: SubmenuPosition; | |||
| tooltipId?: string; | |||
There was a problem hiding this comment.
Instead of adding an id, can be implemented using the id from tooltipProps
| @@ -56,6 +56,8 @@ export interface MenuItemProps extends VibeComponentProps { | |||
| splitMenuItem?: boolean; | |||
| "aria-label"?: AriaAttributes["aria-label"]; | |||
| submenuPosition?: SubmenuPosition; | |||
| tooltipId?: string; | |||
| shouldShowTooltip?: boolean; | |||
There was a problem hiding this comment.
Do we need this prop? there is already logic in the component for this here.
| tooltipId={tooltipProps?.id} | ||
| shouldShowTooltip={!!shouldShowTooltip} |
There was a problem hiding this comment.
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.
| @@ -121,6 +123,7 @@ const MenuItem: VibeComponent<MenuItemProps | MenuItemTitleComponentProps> & { | |||
| content={shouldShowTooltip ? finalTooltipContent : null} | |||
| position={tooltipPosition} | |||
| showDelay={tooltipShowDelay} | |||
| childId={baseMenuProps.id} | |||
There was a problem hiding this comment.
Tooltip has an id prop do we need to add a childId?
| @@ -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)}> | |||
There was a problem hiding this comment.
Would the id be enough? why need to add childId?
https://monday.monday.com/boards/5751390280/pulses/5751647586
Resolves #
It will let screen readers reach disabled items and be aware of the disable reason