From be1f47a293ed3e8d159618ca8eeedbda4c9b7f92 Mon Sep 17 00:00:00 2001 From: marcoskolodny Date: Tue, 3 Oct 2023 23:41:35 +0200 Subject: [PATCH] use item indices instead of labels for focusing logic --- src/__stories__/menu-story.tsx | 14 +++--- src/checkbox.tsx | 7 +-- src/menu.tsx | 92 +++++++++++++++++++--------------- 3 files changed, 63 insertions(+), 50 deletions(-) diff --git a/src/__stories__/menu-story.tsx b/src/__stories__/menu-story.tsx index cdf2f06ba7..fb132ad34a 100644 --- a/src/__stories__/menu-story.tsx +++ b/src/__stories__/menu-story.tsx @@ -34,9 +34,9 @@ export const Default: StoryComponent = ({ icon, checkbox, }) => { - const [valuesState, setValuesState] = React.useState>([]); + const [valuesState, setValuesState] = React.useState>([]); - const setValues = (val: string) => { + const setValues = (val: number) => { if (valuesState.includes(val)) { setValuesState(valuesState.filter((value) => value !== val)); } else { @@ -82,18 +82,18 @@ export const Default: StoryComponent = ({ {[...Array(menuOptionsCount).keys()].map((optionIndex) => ( { + onPress={(item) => { if (checkbox) { - setValues(value); + setValues(item); } else { - alert({title: value, message: 'pressed'}); + alert({title: `Item ${item + 1}`, message: 'pressed'}); } }} {...(checkbox && { controlType: 'checkbox' as const, - checked: valuesState.includes(`Option ${optionIndex + 1}`), + checked: valuesState.includes(optionIndex), })} Icon={icon ? IconLightningRegular : undefined} /> diff --git a/src/checkbox.tsx b/src/checkbox.tsx index 30a64117e2..008b01b964 100644 --- a/src/checkbox.tsx +++ b/src/checkbox.tsx @@ -8,6 +8,7 @@ import classnames from 'classnames'; import {getPrefixedDataAttributes} from './utils/dom'; import * as styles from './checkbox.css'; import {vars} from './skins/skin-contract.css'; +import {combineRefs} from './utils/common'; import type {DataAttributes} from './utils/types'; @@ -83,7 +84,7 @@ type ChildrenProps = { dataAttributes?: DataAttributes; }; -const Checkbox: React.FC = (props) => { +const Checkbox = React.forwardRef((props, ref) => { const labelId = useAriaId(props['aria-labelledby']); const ariaLabel = props['aria-label']; const hasExternalLabel = ariaLabel || props['aria-labelledby']; @@ -132,7 +133,7 @@ const Checkbox: React.FC = (props) => { } }} tabIndex={disabled ? undefined : 0} - ref={focusableRef} + ref={combineRefs(ref, focusableRef)} className={disabled ? styles.checkboxContainerDisabled : styles.checkboxContainer} aria-label={ariaLabel} aria-labelledby={ariaLabel ? undefined : labelId} @@ -166,6 +167,6 @@ const Checkbox: React.FC = (props) => { )} ); -}; +}); export default Checkbox; diff --git a/src/menu.tsx b/src/menu.tsx index 1140a24828..d10233fce1 100644 --- a/src/menu.tsx +++ b/src/menu.tsx @@ -23,28 +23,38 @@ import type {DataAttributes, IconProps} from './utils/types'; const MENU_TRANSITION_DURATION_IN_MS = 120; type MenuContextType = { - focusedValue: string | null; + focusedItem: number | null; isMenuOpen: boolean; - setFocusedValue: (value: string | null) => void; + setFocusedItem: (item: number | null) => void; closeMenu: () => void; }; const MenuContext = React.createContext({ - focusedValue: null, + focusedItem: null, isMenuOpen: false, - setFocusedValue: () => {}, + setFocusedItem: () => {}, closeMenu: () => {}, }); const useMenuContext = (): MenuContextType => React.useContext(MenuContext); +const getMenuItems = (menu: HTMLElement | null): Array => + menu ? Array.from(menu.querySelectorAll('[role=menuitem],[role=menuitemcheckbox]')) : []; + +const getItemIndexInMenu = (menu: HTMLElement | null, item: HTMLElement | null): number | null => { + if (!item) { + return null; + } + const itemIndex = getMenuItems(menu).indexOf(item); + return itemIndex < 0 ? null : itemIndex; +}; + interface MenuItemBaseProps { label: string; Icon?: React.FC; destructive?: boolean; disabled?: boolean; - 'aria-label'?: string; - onPress: (label: string) => void; + onPress: (item: number) => void; } interface MenuItemWithoutControlProps extends MenuItemBaseProps { @@ -68,23 +78,29 @@ export const MenuItem: React.FC = ({ controlType, checked, }) => { - const {focusedValue, setFocusedValue, closeMenu, isMenuOpen} = useMenuContext(); + const {focusedItem, setFocusedItem, closeMenu, isMenuOpen} = useMenuContext(); + const itemRef = React.useRef(null); + const contentColor = destructive ? vars.colors.textLinkDanger : vars.colors.neutralHigh; + const item = itemRef?.current; + const menu: HTMLElement | null = item?.closest('[role=menu]') || null; + const itemIndex = getItemIndexInMenu(menu, item); + const renderContent = () => controlType === 'checkbox' ? ( { - if (isMenuOpen) { - onPress(label); + if (isMenuOpen && itemIndex !== null) { + onPress(itemIndex); closeMenu(); } }} disabled={disabled} role="menuitemcheckbox" - aria-label={label} render={({controlElement}) => ( @@ -105,15 +121,15 @@ export const MenuItem: React.FC = ({ /> ) : ( { - if (isMenuOpen) { - onPress(label); + if (isMenuOpen && itemIndex !== null) { + onPress(itemIndex); closeMenu(); } }} disabled={disabled} role="menuitem" - aria-label={label} >
@@ -134,10 +150,10 @@ export const MenuItem: React.FC = ({
setFocusedValue(disabled ? null : label)} - onMouseLeave={() => setFocusedValue(null)} + onMouseMove={() => setFocusedItem(disabled ? null : itemIndex)} + onMouseLeave={() => setFocusedItem(null)} > {renderContent()}
@@ -195,7 +211,7 @@ export const Menu: React.FC = ({ const [isMenuOpen, setIsMenuOpen] = React.useState(false); const [target, setTarget] = React.useState(null); const [menu, setMenu] = React.useState(null); - const [focusedValue, setFocusedValue] = React.useState(null); + const [focusedItem, setFocusedItem] = React.useState(null); const [isOpenedwithKeyboard, setIsOpenedwithKeyboard] = React.useState(false); const menuRef = React.useRef(null); @@ -273,42 +289,38 @@ export const Menu: React.FC = ({ close: () => setIsMenuOpen(false), }; - const getMenuItems = React.useCallback( - (): Array => - menu ? Array.from(menu.querySelectorAll('[role=menuitem],[role=menuitemcheckbox]')) : [], - [menu] - ); - const setFirstFocusableItem = React.useCallback(() => { - const items = getMenuItems(); - const nextItem = items.findIndex((value) => !value.getAttribute('aria-disabled')); - setFocusedValue(nextItem < 0 ? null : items[nextItem].getAttribute('aria-label')); - }, [getMenuItems]); + const items = getMenuItems(menu); + const nextItem = items.findIndex((item) => !item.getAttribute('aria-disabled')); + setFocusedItem(nextItem < 0 ? null : nextItem); + }, [menu]); const setNextFocusableItem = React.useCallback( (reverse?: boolean) => { - const items = getMenuItems(); + const items = getMenuItems(menu); if (reverse) { items.reverse(); } - const focusedId = items.findIndex((item) => item.getAttribute('aria-label') === focusedValue); + const currentItem = + focusedItem === null ? -1 : reverse ? items.length - 1 - focusedItem : focusedItem; let nextItem = items.findIndex( - (value, index) => !value.getAttribute('aria-disabled') && index > focusedId + (item, index) => !item.getAttribute('aria-disabled') && index > currentItem ); if (nextItem === -1) { - nextItem = items.findIndex((value) => !value.getAttribute('aria-disabled')); + nextItem = items.findIndex((item) => !item.getAttribute('aria-disabled')); } - setFocusedValue(nextItem < 0 ? null : items[nextItem].getAttribute('aria-label')); + const nextFocusedItem = reverse && nextItem !== -1 ? items.length - 1 - nextItem : nextItem; + setFocusedItem(nextFocusedItem < 0 ? null : nextFocusedItem); items[nextItem]?.focus(); }, - [focusedValue, getMenuItems] + [focusedItem, menu] ); React.useEffect(() => { if (!isMenuOpen) { - setFocusedValue(null); + setFocusedItem(null); } else if (isOpenedwithKeyboard && menu) { setFirstFocusableItem(); setIsOpenedwithKeyboard(false); @@ -337,9 +349,9 @@ export const Menu: React.FC = ({ case SPACE: case ENTER: cancelEvent(e); - getMenuItems() - .find((item) => item.getAttribute('aria-label') === focusedValue) - ?.click(); + if (focusedItem !== null) { + getMenuItems(menu)[focusedItem].click(); + } break; case TAB: cancelEvent(e); @@ -391,7 +403,7 @@ export const Menu: React.FC = ({ classNames={styles.menuTransitionClasses} mountOnEnter unmountOnExit - onExited={() => target?.focus()} + onExit={() => target?.focus()} > { @@ -434,8 +446,8 @@ export const Menu: React.FC = ({ setIsMenuOpen(false), }} >