From 5b94ea0990b0a242196fe7aa46cd474d1f2b171a Mon Sep 17 00:00:00 2001 From: drita Date: Tue, 27 Feb 2024 02:04:52 +0300 Subject: [PATCH 1/7] feat: add aria attributes to menu components --- .../menu/src/menu-divider/menu-divider.js | 2 +- .../src/menu-item/__tests__/menu-item.test.js | 45 +++++++++++++++++++ components/menu/src/menu-item/menu-item.js | 4 ++ .../menu/src/menu-item/menu-item.stories.js | 1 + .../menu/src/menu/__tests__/menu.test.js | 34 ++++++++++++++ components/menu/src/menu/menu.js | 2 +- 6 files changed, 86 insertions(+), 2 deletions(-) create mode 100644 components/menu/src/menu-item/__tests__/menu-item.test.js create mode 100644 components/menu/src/menu/__tests__/menu.test.js diff --git a/components/menu/src/menu-divider/menu-divider.js b/components/menu/src/menu-divider/menu-divider.js index c3f1da6aa3..400cd51044 100644 --- a/components/menu/src/menu-divider/menu-divider.js +++ b/components/menu/src/menu-divider/menu-divider.js @@ -4,7 +4,7 @@ import PropTypes from 'prop-types' import React from 'react' const MenuDivider = ({ className, dataTest, dense }) => ( -
  • +
  • - -) +import React, { + Children, + cloneElement, + isValidElement, + useEffect, + useState, + useRef, +} from 'react' + +const Menu = ({ children, className, dataTest, dense }) => { + const [selectedIndex, setSelectedIndex] = useState(-1) + const menuRef = useRef(null) + + useEffect(() => { + if (!menuRef.current) { + return + } + const menu = menuRef.current + const childrenArray = Children.toArray(children) + + const handleIndex = (index, action) => { + let current_position = index + if (current_position === -1) { + return current_position + } + const current_element = childrenArray[current_position] + if ( + ['MenuSectionHeader', 'MenuDivider'].includes( + current_element.type?.name + ) || + current_element.props.disabled + ) { + current_position = action === 'up' ? index - 1 : index + 1 + } + + if (current_position > childrenArray.length - 1) { + current_position = 0 + } else if (current_position < 0) { + current_position = childrenArray.length - 1 + } + + return current_position + } + + const handleDirection = (event) => { + let active = selectedIndex + const { key } = event + + if (key === 'ArrowUp') { + if (active > 0) { + active = active - 1 + active = handleIndex(active, 'up') + setSelectedIndex(active) + } + } + if (key === 'ArrowDown') { + if (active < childrenArray.length - 1) { + active = active + 1 + active = handleIndex(active, 'down') + setSelectedIndex(active) + } + } + } + + const handleActionKeys = (event) => { + if (selectedIndex > -1) { + const { target } = event + const child = target.children[selectedIndex] + if (child.onclick) { + child.click() + } else if (child.childNodes[0].onclick) { + child.childNodes[0].click() + } + } + } + + const handleKeyDown = (event) => { + event.preventDefault() + + if (event.key === ' ' || event.key === 'Enter') { + handleActionKeys(event) + } + if (event.key.startsWith('Arrow')) { + handleDirection(event) + } + } + + menu.addEventListener('keydown', handleKeyDown) + + return () => { + menu.removeEventListener('keydown', handleKeyDown) + } + }, [children, selectedIndex]) + + return ( +
      + {Children.map(children, (child, index) => { + return isValidElement(child) + ? cloneElement(child, { + dense: + typeof child.props.dense === 'boolean' + ? child.props.dense + : dense, + hideDivider: + typeof child.props.hideDivider !== 'boolean' && + index === 0 + ? true + : child.props.hideDivider, + selected: + index === selectedIndex && !child.props.disabled, + }) + : child + })} + + +
    + ) +} Menu.defaultProps = { dataTest: 'dhis2-uicore-menulist', From c439dec6520b6317cb59ab8b4e7280ad954655d5 Mon Sep 17 00:00:00 2001 From: Diana Nanyanzi Date: Tue, 30 Apr 2024 07:36:34 +0300 Subject: [PATCH 3/7] feat: add focus and submenu navigation --- .../src/menu-item/__tests__/menu-item.test.js | 11 +- components/menu/src/menu-item/menu-item.js | 37 ++- .../menu/src/menu/__tests__/menu.test.js | 2 +- components/menu/src/menu/menu.js | 259 ++++++++++-------- 4 files changed, 189 insertions(+), 120 deletions(-) diff --git a/components/menu/src/menu-item/__tests__/menu-item.test.js b/components/menu/src/menu-item/__tests__/menu-item.test.js index db10e94947..f9428441f0 100644 --- a/components/menu/src/menu-item/__tests__/menu-item.test.js +++ b/components/menu/src/menu-item/__tests__/menu-item.test.js @@ -10,8 +10,9 @@ describe('Menu Component', () => { ) const menuItem = wrapper.find({ 'data-test': menuItemDataTest }) expect(menuItem.prop('role')).toBe('menuitem') - expect(menuItem.prop('aria-disabled')).toBe(false) - expect(menuItem.prop('role')).not.toBe('menuitemcheckbox') + // expect(menuItem.prop('aria-disabled')).toBe(false) + // expect(menuItem.prop('role')).not.toBe('menuitemcheckbox') + // }) it('Disabled menu item has aria disabled attribute', () => { @@ -25,8 +26,8 @@ describe('Menu Component', () => { ) const menuItem = wrapper.find({ 'data-test': menuItemDataTest }) expect(menuItem.prop('role')).toBe('menuitem') - expect(menuItem.prop('role')).not.toBe('menuitemcheckbox') - expect(menuItem.prop('aria-disabled')).toBe(true) + // expect(menuItem.prop('role')).not.toBe('menuitemcheckbox') + // expect(menuItem.prop('aria-disabled')).toBe(true) }) it('Toggle-able menu item has menuitemcheckbox role', () => { @@ -40,6 +41,6 @@ describe('Menu Component', () => { ) const menuItem = wrapper.find({ 'data-test': menuItemDataTest }) expect(menuItem.prop('role')).toBe('menuitemcheckbox') - expect(menuItem.prop('aria-disabled')).toBe(false) + // expect(menuItem.prop('aria-disabled')).toBe(false) }) }) diff --git a/components/menu/src/menu-item/menu-item.js b/components/menu/src/menu-item/menu-item.js index 72205b932c..61302c5877 100644 --- a/components/menu/src/menu-item/menu-item.js +++ b/components/menu/src/menu-item/menu-item.js @@ -3,7 +3,7 @@ import { Portal } from '@dhis2-ui/portal' import { IconChevronRight24 } from '@dhis2/ui-icons' import cx from 'classnames' import PropTypes from 'prop-types' -import React, { useRef } from 'react' +import React, { useRef, useEffect } from 'react' import { FlyoutMenu } from '../index.js' import styles from './menu-item.styles.js' @@ -40,11 +40,19 @@ const MenuItem = ({ showSubMenu, toggleSubMenu, suffix, + tabIndex, checkbox, - selected, + radio, + checked, }) => { const menuItemRef = useRef() + useEffect(() => { + if (active && tabIndex === 0) { + menuItemRef.current.childNodes[0].focus() + } + }, [active, tabIndex]) + return ( <>
  • {icon && {icon}} @@ -106,7 +124,10 @@ MenuItem.defaultProps = { MenuItem.propTypes = { active: PropTypes.bool, + /* For using menuitem as checkbox */ checkbox: PropTypes.bool, + /* Value of checkbox */ + checked: PropTypes.bool, chevron: PropTypes.bool, /** * Nested menu items can become submenus. @@ -124,11 +145,13 @@ MenuItem.propTypes = { icon: PropTypes.node, /** Text in the menu item */ label: PropTypes.node, - selected: PropTypes.bool, + /* For using menuitem as radio */ + radio: PropTypes.bool, /** When true, nested menu items are shown in a Popper */ showSubMenu: PropTypes.bool, /** A supporting element shown at the end of the menu item */ suffix: PropTypes.node, + tabIndex: PropTypes.number, /** For using menu item as a link */ target: PropTypes.string, /** On click, this function is called (without args) */ diff --git a/components/menu/src/menu/__tests__/menu.test.js b/components/menu/src/menu/__tests__/menu.test.js index 9af77433dd..b273dabea5 100644 --- a/components/menu/src/menu/__tests__/menu.test.js +++ b/components/menu/src/menu/__tests__/menu.test.js @@ -20,7 +20,7 @@ describe('Menu Component', () => { const menuElement = wrapper.find({ 'data-test': menuDataTest }) const menuItem = wrapper.find({ 'data-test': menuItemDataTest }) const menuDivider = wrapper.find({ 'data-test': dividerDataTest }) - expect(menuItem.prop('role')).toBe('menuitem') + expect(menuItem.firstChild.prop('role')).toBe('menuitem') expect(menuDivider.prop('role')).toBe('separator') expect(menuElement.prop('role')).toBe('menu') }) diff --git a/components/menu/src/menu/menu.js b/components/menu/src/menu/menu.js index cea8aa2495..896c596ee4 100644 --- a/components/menu/src/menu/menu.js +++ b/components/menu/src/menu/menu.js @@ -3,134 +3,179 @@ import React, { Children, cloneElement, isValidElement, + useCallback, useEffect, - useState, useRef, + useState, } from 'react' -const Menu = ({ children, className, dataTest, dense }) => { - const [selectedIndex, setSelectedIndex] = useState(-1) - const menuRef = useRef(null) +const getMenu = (menuitem) => { + let menu = menuitem + let role = menuitem.getAttribute('role') - useEffect(() => { - if (!menuRef.current) { - return + while (menu && role !== 'menu' && role !== 'menubar') { + menu = menu.parentNode + if (menu) { + role = menu.getAttribute('role') } - const menu = menuRef.current - const childrenArray = Children.toArray(children) + } - const handleIndex = (index, action) => { - let current_position = index - if (current_position === -1) { - return current_position - } - const current_element = childrenArray[current_position] - if ( - ['MenuSectionHeader', 'MenuDivider'].includes( - current_element.type?.name - ) || - current_element.props.disabled - ) { - current_position = action === 'up' ? index - 1 : index + 1 - } - - if (current_position > childrenArray.length - 1) { - current_position = 0 - } else if (current_position < 0) { - current_position = childrenArray.length - 1 - } - - return current_position - } + return menu +} - const handleDirection = (event) => { - let active = selectedIndex - const { key } = event +const Menu = ({ children, className, dataTest, dense }) => { + const [focusedIndex, setFocusedIndex] = useState(0) + const menuRef = useRef(null) + const [showSubMenu, setShowSubMenu] = useState(false) + // track last parent menu item + // const [parentMenuItem, setParentMenuItem] = useState() - if (key === 'ArrowUp') { - if (active > 0) { - active = active - 1 - active = handleIndex(active, 'up') - setSelectedIndex(active) - } - } - if (key === 'ArrowDown') { - if (active < childrenArray.length - 1) { - active = active + 1 - active = handleIndex(active, 'down') - setSelectedIndex(active) - } - } + const handleFocus = (e) => { + if (menuRef.current === e.target) { + menuRef.current?.firstChild?.childNodes[0]?.focus() } - - const handleActionKeys = (event) => { - if (selectedIndex > -1) { - const { target } = event - const child = target.children[selectedIndex] - if (child.onclick) { - child.click() - } else if (child.childNodes[0].onclick) { - child.childNodes[0].click() - } + } + + const setNextIndex = ({ arrayLength, index }) => { + const newIndex = index === arrayLength - 1 ? 0 : index + 1 + + return newIndex + } + + const setPrevIndex = ({ arrayLength, index }) => { + const newIndex = index === 0 ? arrayLength - 1 : index - 1 + + return newIndex + } + + const handleKeyDown = useCallback( + (event) => { + const childrenArray = Children.toArray(children) + const childrenLength = childrenArray.length + + const activeEl = document.activeElement + + switch (event.key) { + case 'ArrowUp': + event.preventDefault() + setFocusedIndex((prevIndex) => + setPrevIndex({ + arrayLength: childrenLength, + index: prevIndex, + }) + ) + break + case 'ArrowDown': + event.preventDefault() + setFocusedIndex((prevIndex) => + setNextIndex({ + arrayLength: childrenLength, + index: prevIndex, + }) + ) + break + case 'ArrowRight': + event.preventDefault() + if (activeEl.getAttribute('aria-haspopup')) { + setShowSubMenu(true) + activeEl.setAttribute('aria-expanded', true) + } + + break + case 'ArrowLeft': + event.preventDefault() + setShowSubMenu(false) + // popper + getMenu(activeEl).parentNode.parentNode.style.display = '' + // activeEl.setAttribute('aria-expanded', false) + + break + case 'Enter': + case ' ': + event.preventDefault() + + if (activeEl.onclick) { + activeEl.click() + } + + // link + if (activeEl.href) { + window.open( + activeEl.href, + activeEl.target ? activeEl.target : '_blank' + ) + } + + break + default: + return } - } - - const handleKeyDown = (event) => { - event.preventDefault() + }, + [children] + ) - if (event.key === ' ' || event.key === 'Enter') { - handleActionKeys(event) - } - if (event.key.startsWith('Arrow')) { - handleDirection(event) - } + useEffect(() => { + if (!menuRef && !menuRef.current) { + return } + const menu = menuRef.current + menu.addEventListener('focus', handleFocus) menu.addEventListener('keydown', handleKeyDown) return () => { menu.removeEventListener('keydown', handleKeyDown) + menu.removeEventListener('focus', handleFocus) } - }, [children, selectedIndex]) + }, [handleKeyDown]) return ( -
      - {Children.map(children, (child, index) => { - return isValidElement(child) - ? cloneElement(child, { - dense: - typeof child.props.dense === 'boolean' - ? child.props.dense - : dense, - hideDivider: - typeof child.props.hideDivider !== 'boolean' && - index === 0 - ? true - : child.props.hideDivider, - selected: - index === selectedIndex && !child.props.disabled, - }) - : child - })} - - -
    + <> +
      + {Children.map(children, (child, index) => { + return isValidElement(child) ? ( + cloneElement(child, { + dense: + typeof child.props.dense === 'boolean' + ? child.props.dense + : dense, + hideDivider: + typeof child.props.hideDivider !== 'boolean' && + index === 0 + ? true + : child.props.hideDivider, + active: focusedIndex === index, + tabIndex: focusedIndex === index ? 0 : -1, + showSubMenu: child.props.children + ? showSubMenu + : null, + }) + ) : ( +
    • + {child} +
      • + ) + })} + + +
    + ) } From b9ba8e7eaf8bf941bde5317ddae171589eb9945b Mon Sep 17 00:00:00 2001 From: Diana Nanyanzi Date: Thu, 2 May 2024 04:42:05 +0300 Subject: [PATCH 4/7] feat: refactor keyboard functionality on menu and menu item components --- .../menu/src/flyout-menu/flyout-menu.js | 23 +- components/menu/src/menu-item/menu-item.js | 65 +++-- .../menu/src/menu-item/menu-item.stories.js | 1 + components/menu/src/menu/menu.js | 238 +++++++++--------- 4 files changed, 178 insertions(+), 149 deletions(-) diff --git a/components/menu/src/flyout-menu/flyout-menu.js b/components/menu/src/flyout-menu/flyout-menu.js index c3b4035635..4305db9965 100644 --- a/components/menu/src/flyout-menu/flyout-menu.js +++ b/components/menu/src/flyout-menu/flyout-menu.js @@ -1,6 +1,12 @@ import { colors, elevations, spacers } from '@dhis2/ui-constants' import PropTypes from 'prop-types' -import React, { Children, cloneElement, isValidElement, useState } from 'react' +import React, { + Children, + cloneElement, + isValidElement, + useState, + // useEffect, +} from 'react' import { Menu } from '../index.js' const FlyoutMenu = ({ @@ -11,12 +17,25 @@ const FlyoutMenu = ({ maxHeight, maxWidth, }) => { - const [openedSubMenu, setOpenedSubMenu] = useState(null) + const [openedSubMenu, setOpenedSubMenu] = useState(0) const toggleSubMenu = (index) => { const toggleValue = index === openedSubMenu ? null : index setOpenedSubMenu(toggleValue) } + // useEffect(() => { + // if (openedSubMenu) { + // menuItemRef.current.childNodes[0].focus() + // } + // }, [openedSubMenu]) + + // console.log(document.activeElement, "flyout menu active element") + // // const handleKeydown = () => { + + // // } + + console.log(openedSubMenu, 'opened submenu') + return (
    diff --git a/components/menu/src/menu-item/menu-item.js b/components/menu/src/menu-item/menu-item.js index 61302c5877..f376be1be8 100644 --- a/components/menu/src/menu-item/menu-item.js +++ b/components/menu/src/menu-item/menu-item.js @@ -3,7 +3,7 @@ import { Portal } from '@dhis2-ui/portal' import { IconChevronRight24 } from '@dhis2/ui-icons' import cx from 'classnames' import PropTypes from 'prop-types' -import React, { useRef, useEffect } from 'react' +import React, { forwardRef, useRef } from 'react' import { FlyoutMenu } from '../index.js' import styles from './menu-item.styles.js' @@ -22,37 +22,34 @@ const createOnClickHandler = onClick && onClick({ value }, evt) toggleSubMenu && toggleSubMenu() } -const MenuItem = ({ - href, - onClick, - children, - target, - icon, - className, - destructive, - disabled, - dense, - active, - dataTest, - chevron, - value, - label, - showSubMenu, - toggleSubMenu, - suffix, - tabIndex, - checkbox, - radio, - checked, -}) => { +const MenuItem = forwardRef(function MenuItem( + { + href, + onClick, + children, + target, + icon, + className, + destructive, + disabled, + dense, + active, + dataTest, + chevron, + value, + label, + showSubMenu, + toggleSubMenu, + suffix, + tabIndex, + checkbox, + radio, + checked, + }, + ref +) { const menuItemRef = useRef() - useEffect(() => { - if (active && tabIndex === 0) { - menuItemRef.current.childNodes[0].focus() - } - }, [active, tabIndex]) - return ( <>
  • {icon && {icon}} @@ -116,7 +115,7 @@ const MenuItem = ({ )} ) -} +}) MenuItem.defaultProps = { dataTest: 'dhis2-uicore-menuitem', diff --git a/components/menu/src/menu-item/menu-item.stories.js b/components/menu/src/menu-item/menu-item.stories.js index e986810200..a25b55628b 100644 --- a/components/menu/src/menu-item/menu-item.stories.js +++ b/components/menu/src/menu-item/menu-item.stories.js @@ -134,6 +134,7 @@ export const ToggleMenuItem = (args) => { icon={icon} label="A toggle menu item" checkbox={true} + checked={on} />
    • ) diff --git a/components/menu/src/menu/menu.js b/components/menu/src/menu/menu.js index 896c596ee4..30a93a8c0f 100644 --- a/components/menu/src/menu/menu.js +++ b/components/menu/src/menu/menu.js @@ -3,179 +3,189 @@ import React, { Children, cloneElement, isValidElement, - useCallback, - useEffect, useRef, useState, + useEffect, + useCallback, } from 'react' -const getMenu = (menuitem) => { - let menu = menuitem - let role = menuitem.getAttribute('role') - - while (menu && role !== 'menu' && role !== 'menubar') { - menu = menu.parentNode - if (menu) { - role = menu.getAttribute('role') - } - } +function setNextIndex({ arrayLength, index }) { + return index === arrayLength - 1 ? 0 : index + 1 +} - return menu +function setPrevIndex({ arrayLength, index }) { + return index === 0 ? arrayLength - 1 : index - 1 } const Menu = ({ children, className, dataTest, dense }) => { - const [focusedIndex, setFocusedIndex] = useState(0) + const [focusedIndex, setFocusedIndex] = useState(-1) const menuRef = useRef(null) - const [showSubMenu, setShowSubMenu] = useState(false) - // track last parent menu item - // const [parentMenuItem, setParentMenuItem] = useState() - - const handleFocus = (e) => { - if (menuRef.current === e.target) { - menuRef.current?.firstChild?.childNodes[0]?.focus() - } - } + const itemsRefs = useRef([]) - const setNextIndex = ({ arrayLength, index }) => { - const newIndex = index === arrayLength - 1 ? 0 : index + 1 + const findFocusableItems = () => { + const obj = {} - return newIndex + if (itemsRefs.current?.length) { + itemsRefs.current.map((ref, index) => { + return (obj[index] = ref) + }) + } + return obj } - const setPrevIndex = ({ arrayLength, index }) => { - const newIndex = index === 0 ? arrayLength - 1 : index - 1 + const focusFirstFocusableItem = useCallback((e) => { + const focusableItems = findFocusableItems() - return newIndex - } + if (e.target === menuRef.current) { + setFocusedIndex(~~Object.keys(focusableItems)[0]) + Object.values(focusableItems)[0].focus() + } + }, []) + + const setNextFocusableIndex = useCallback(({ elemIndex, action }) => { + const focusableItems = findFocusableItems() + const focusableIndicesArray = Object.keys(focusableItems).map( + (i) => ~~i + ) + const position = focusableIndicesArray.indexOf(elemIndex) + + if (position != -1) { + if (action === 'next') { + return focusableIndicesArray[ + setNextIndex({ + arrayLength: focusableIndicesArray.length, + index: position, + }) + ] + } else { + return focusableIndicesArray[ + setPrevIndex({ + arrayLength: focusableIndicesArray.length, + index: position, + }) + ] + } + } else { + return focusableIndicesArray[0] + } + }, []) const handleKeyDown = useCallback( (event) => { - const childrenArray = Children.toArray(children) - const childrenLength = childrenArray.length - - const activeEl = document.activeElement - switch (event.key) { case 'ArrowUp': event.preventDefault() setFocusedIndex((prevIndex) => - setPrevIndex({ - arrayLength: childrenLength, - index: prevIndex, + setNextFocusableIndex({ + elemIndex: prevIndex, + action: 'previous', }) ) break case 'ArrowDown': event.preventDefault() setFocusedIndex((prevIndex) => - setNextIndex({ - arrayLength: childrenLength, - index: prevIndex, + setNextFocusableIndex({ + elemIndex: prevIndex, + action: 'next', }) ) break case 'ArrowRight': event.preventDefault() - if (activeEl.getAttribute('aria-haspopup')) { - setShowSubMenu(true) - activeEl.setAttribute('aria-expanded', true) + if (event.target.hasAttribute('aria-haspopup')) { + event.target.click() } - break case 'ArrowLeft': event.preventDefault() - setShowSubMenu(false) - // popper - getMenu(activeEl).parentNode.parentNode.style.display = '' - // activeEl.setAttribute('aria-expanded', false) - break - case 'Enter': case ' ': + case 'Enter': event.preventDefault() - - if (activeEl.onclick) { - activeEl.click() - } - - // link - if (activeEl.href) { - window.open( - activeEl.href, - activeEl.target ? activeEl.target : '_blank' - ) - } - + event.target.click() break default: return } }, - [children] + [setNextFocusableIndex] ) + useEffect(() => { + if (!itemsRefs.current?.length) { + return + } + itemsRefs.current[focusedIndex]?.focus() + }, [focusedIndex]) + useEffect(() => { if (!menuRef && !menuRef.current) { return } const menu = menuRef.current - menu.addEventListener('focus', handleFocus) + menu.addEventListener('focus', focusFirstFocusableItem) menu.addEventListener('keydown', handleKeyDown) return () => { menu.removeEventListener('keydown', handleKeyDown) - menu.removeEventListener('focus', handleFocus) + menu.removeEventListener('focus', focusFirstFocusableItem) } - }, [handleKeyDown]) + }, [focusFirstFocusableItem, handleKeyDown]) return ( - <> -
      - {Children.map(children, (child, index) => { - return isValidElement(child) ? ( - cloneElement(child, { - dense: - typeof child.props.dense === 'boolean' - ? child.props.dense - : dense, - hideDivider: - typeof child.props.hideDivider !== 'boolean' && - index === 0 - ? true - : child.props.hideDivider, - active: focusedIndex === index, - tabIndex: focusedIndex === index ? 0 : -1, - showSubMenu: child.props.children - ? showSubMenu - : null, - }) - ) : ( -
    • - {child} -
      • - ) - })} - - -
    - +
      + {Children.map(children, (child, index) => { + return isValidElement(child) + ? cloneElement(child, { + dense: + typeof child.props.dense === 'boolean' + ? child.props.dense + : dense, + hideDivider: + typeof child.props.hideDivider !== 'boolean' && + index === 0 + ? true + : child.props.hideDivider, + tabIndex: focusedIndex === index ? 0 : -1, + active: focusedIndex === index, + ref: (node) => { + const role = node?.getAttribute('role') + if ( + [ + 'menuitem', + 'menuitemradio', + 'menuitemcheckbox', + ].includes(role) + ) { + return (itemsRefs.current[index] = node) + } else { + return null + } + }, + }) + : child + })} + + +
    ) } From fb7571a278a79643bc56b2ec9e9866655bb20b22 Mon Sep 17 00:00:00 2001 From: Diana Nanyanzi Date: Sun, 5 May 2024 02:08:34 +0300 Subject: [PATCH 5/7] chore: improve menu and menu item tests --- .../src/menu-item/__tests__/menu-item.test.js | 30 +++++++++----- .../menu/src/menu/__tests__/menu.test.js | 41 ++++++++++++++++--- 2 files changed, 54 insertions(+), 17 deletions(-) diff --git a/components/menu/src/menu-item/__tests__/menu-item.test.js b/components/menu/src/menu-item/__tests__/menu-item.test.js index f9428441f0..3c847d6c7b 100644 --- a/components/menu/src/menu-item/__tests__/menu-item.test.js +++ b/components/menu/src/menu-item/__tests__/menu-item.test.js @@ -3,19 +3,18 @@ import React from 'react' import { MenuItem } from '../menu-item.js' describe('Menu Component', () => { - it('Default menu item has aria role', () => { + it('Default menu item has role', () => { const menuItemDataTest = 'data-test-menu-item' const wrapper = mount( ) const menuItem = wrapper.find({ 'data-test': menuItemDataTest }) - expect(menuItem.prop('role')).toBe('menuitem') - // expect(menuItem.prop('aria-disabled')).toBe(false) - // expect(menuItem.prop('role')).not.toBe('menuitemcheckbox') - // + expect(menuItem.childAt(0).prop('role')).toBe('menuitem') + expect(menuItem.childAt(0).prop('aria-disabled')).toBe(undefined) + expect(menuItem.childAt(0).prop('aria-label')).toBe('Menu item') }) - it('Disabled menu item has aria disabled attribute', () => { + it('Disabled menu item has aria-disabled attribute', () => { const menuItemDataTest = 'data-test-menu-item' const wrapper = mount( { /> ) const menuItem = wrapper.find({ 'data-test': menuItemDataTest }) - expect(menuItem.prop('role')).toBe('menuitem') - // expect(menuItem.prop('role')).not.toBe('menuitemcheckbox') - // expect(menuItem.prop('aria-disabled')).toBe(true) + + expect(menuItem.childAt(0).prop('role')).toBe('menuitem') + expect(menuItem.childAt(0).prop('aria-disabled')).toBe(true) + expect(menuItem.childAt(0).prop('aria-label')).toBe( + 'Disabled menu item' + ) }) it('Toggle-able menu item has menuitemcheckbox role', () => { @@ -37,10 +39,16 @@ describe('Menu Component', () => { dataTest={menuItemDataTest} label="Toggle-able menu item" checkbox={true} + checked={false} /> ) const menuItem = wrapper.find({ 'data-test': menuItemDataTest }) - expect(menuItem.prop('role')).toBe('menuitemcheckbox') - // expect(menuItem.prop('aria-disabled')).toBe(false) + + expect(menuItem.childAt(0).prop('role')).not.toBe('menuitem') + expect(menuItem.childAt(0).prop('role')).toBe('menuitemcheckbox') + expect(menuItem.childAt(0).prop('aria-checked')).toBe(false) + expect(menuItem.childAt(0).prop('aria-label')).toBe( + 'Toggle-able menu item' + ) }) }) diff --git a/components/menu/src/menu/__tests__/menu.test.js b/components/menu/src/menu/__tests__/menu.test.js index b273dabea5..bde4a49e05 100644 --- a/components/menu/src/menu/__tests__/menu.test.js +++ b/components/menu/src/menu/__tests__/menu.test.js @@ -6,29 +6,58 @@ import { MenuSectionHeader } from '../../menu-section-header/menu-section-header import { Menu } from '../menu.js' describe('Menu Component', () => { - it('Menu has aria attributes', () => { + it('Menu and menu items have aria attributes', () => { const menuDataTest = 'data-test-menu' const menuItemDataTest = 'data-test-menu-item' const dividerDataTest = 'data-test-menu-divider' const wrapper = mount( - - + + ) const menuElement = wrapper.find({ 'data-test': menuDataTest }) const menuItem = wrapper.find({ 'data-test': menuItemDataTest }) const menuDivider = wrapper.find({ 'data-test': dividerDataTest }) - expect(menuItem.firstChild.prop('role')).toBe('menuitem') - expect(menuDivider.prop('role')).toBe('separator') + expect(menuElement.prop('role')).toBe('menu') + + expect(menuItem.childAt(0).props().role).toBe('menuitem') + expect(menuItem.childAt(0).prop('aria-label')).toBe('Menu item') + expect(menuDivider.prop('role')).toBe('separator') }) - it('Empty menu has aria attributes', () => { + it('Empty menu has role menu', () => { const menuDataTest = 'data-test-menu' const wrapper = mount() const menuElement = wrapper.find({ 'data-test': menuDataTest }) expect(menuElement.prop('role')).toBe('menu') }) + + it('Submenu has relevant aria attributes', () => { + const showSubMenu = false + const wrapper = mount( + + jest.fn()} + label="Parent of submenus" + > + + + + ) + + const menuItem = wrapper.find({ role: 'menuitem' }) + + expect(menuItem.prop('aria-haspopup')).toBe('menu') + expect(menuItem.prop('aria-expanded')).toBe(false) + expect(menuItem.prop('aria-expanded')).toBe(false) + expect(menuItem.prop('aria-label')).toBe('Parent of submenus') + expect(wrapper.find({ label: 'Test submenu child' }).exists()).toBe( + false + ) + // todo: simulate click to show submenu + }) }) From d35ffb2715fda0225197fea3d06b19242da95879 Mon Sep 17 00:00:00 2001 From: Diana Nanyanzi Date: Mon, 6 May 2024 08:13:34 +0300 Subject: [PATCH 6/7] feat: enable left and right arrow functionality --- .../menu/src/flyout-menu/flyout-menu.js | 23 +---------- components/menu/src/menu-item/menu-item.js | 13 ++++++- components/menu/src/menu/menu.js | 38 +++++++++++++++---- components/popper/src/popper.js | 3 ++ components/popper/types/index.d.ts | 1 + 5 files changed, 48 insertions(+), 30 deletions(-) diff --git a/components/menu/src/flyout-menu/flyout-menu.js b/components/menu/src/flyout-menu/flyout-menu.js index 4305db9965..c3b4035635 100644 --- a/components/menu/src/flyout-menu/flyout-menu.js +++ b/components/menu/src/flyout-menu/flyout-menu.js @@ -1,12 +1,6 @@ import { colors, elevations, spacers } from '@dhis2/ui-constants' import PropTypes from 'prop-types' -import React, { - Children, - cloneElement, - isValidElement, - useState, - // useEffect, -} from 'react' +import React, { Children, cloneElement, isValidElement, useState } from 'react' import { Menu } from '../index.js' const FlyoutMenu = ({ @@ -17,25 +11,12 @@ const FlyoutMenu = ({ maxHeight, maxWidth, }) => { - const [openedSubMenu, setOpenedSubMenu] = useState(0) + const [openedSubMenu, setOpenedSubMenu] = useState(null) const toggleSubMenu = (index) => { const toggleValue = index === openedSubMenu ? null : index setOpenedSubMenu(toggleValue) } - // useEffect(() => { - // if (openedSubMenu) { - // menuItemRef.current.childNodes[0].focus() - // } - // }, [openedSubMenu]) - - // console.log(document.activeElement, "flyout menu active element") - // // const handleKeydown = () => { - - // // } - - console.log(openedSubMenu, 'opened submenu') - return (
    diff --git a/components/menu/src/menu-item/menu-item.js b/components/menu/src/menu-item/menu-item.js index f376be1be8..bd25590776 100644 --- a/components/menu/src/menu-item/menu-item.js +++ b/components/menu/src/menu-item/menu-item.js @@ -90,6 +90,10 @@ const MenuItem = forwardRef(function MenuItem( aria-disabled={disabled} aria-checked={checkbox && checked} aria-label={label} + aria-owns={ + children && + `popper-${label.split(' ').join('-').toLowerCase()}` + } > {icon && {icon}} @@ -108,7 +112,14 @@ const MenuItem = forwardRef(function MenuItem(
    • {children && showSubMenu && ( - + {children} diff --git a/components/menu/src/menu/menu.js b/components/menu/src/menu/menu.js index 30a93a8c0f..9d7a56b877 100644 --- a/components/menu/src/menu/menu.js +++ b/components/menu/src/menu/menu.js @@ -33,14 +33,19 @@ const Menu = ({ children, className, dataTest, dense }) => { return obj } - const focusFirstFocusableItem = useCallback((e) => { - const focusableItems = findFocusableItems() - - if (e.target === menuRef.current) { - setFocusedIndex(~~Object.keys(focusableItems)[0]) - Object.values(focusableItems)[0].focus() - } - }, []) + const focusFirstFocusableItem = useCallback( + (e) => { + const focusableItems = findFocusableItems() + + if (e.target === menuRef.current) { + if (focusedIndex === -1) { + setFocusedIndex(~~Object.keys(focusableItems)[0]) + // Object.values(focusableItems)[0].focus() + } + } + }, + [focusedIndex] + ) const setNextFocusableIndex = useCallback(({ elemIndex, action }) => { const focusableItems = findFocusableItems() @@ -99,6 +104,20 @@ const Menu = ({ children, className, dataTest, dense }) => { break case 'ArrowLeft': event.preventDefault() + document + .querySelector( + `[aria-owns='${event.target.parentNode.parentNode.parentNode.parentNode.getAttribute( + 'id' + )}']` + ) + .focus() + document + .querySelector( + `[aria-owns='${event.target.parentNode.parentNode.parentNode.parentNode.getAttribute( + 'id' + )}']` + ) + .click() break case ' ': case 'Enter': @@ -156,6 +175,9 @@ const Menu = ({ children, className, dataTest, dense }) => { : child.props.hideDivider, tabIndex: focusedIndex === index ? 0 : -1, active: focusedIndex === index, + showSubMenu: child.props.children + ? child.props.showSubMenu + : null, ref: (node) => { const role = node?.getAttribute('role') if ( diff --git a/components/popper/src/popper.js b/components/popper/src/popper.js index 453e3a067f..308b425c62 100644 --- a/components/popper/src/popper.js +++ b/components/popper/src/popper.js @@ -16,6 +16,7 @@ const flipPlacement = (placement) => { } const Popper = ({ + id, children, className, dataTest, @@ -51,6 +52,7 @@ const Popper = ({ return (
    export interface PopperProps { + id?: string /** * Content inside the Popper */ From 9416c2f0ce34ffdb80b7f643692e0aa80f4a5caf Mon Sep 17 00:00:00 2001 From: Diana Nanyanzi Date: Mon, 6 May 2024 08:30:28 +0300 Subject: [PATCH 7/7] feat: refactor functionality to open and close submenus --- .../menu/src/flyout-menu/flyout-menu.js | 18 ++++- components/menu/src/menu-item/menu-item.js | 34 +++++++++- components/menu/src/menu/menu.js | 68 +++++++------------ 3 files changed, 72 insertions(+), 48 deletions(-) diff --git a/components/menu/src/flyout-menu/flyout-menu.js b/components/menu/src/flyout-menu/flyout-menu.js index c3b4035635..43b3cd6b23 100644 --- a/components/menu/src/flyout-menu/flyout-menu.js +++ b/components/menu/src/flyout-menu/flyout-menu.js @@ -1,6 +1,13 @@ import { colors, elevations, spacers } from '@dhis2/ui-constants' import PropTypes from 'prop-types' -import React, { Children, cloneElement, isValidElement, useState } from 'react' +import React, { + Children, + cloneElement, + isValidElement, + useEffect, + useRef, + useState, +} from 'react' import { Menu } from '../index.js' const FlyoutMenu = ({ @@ -16,9 +23,16 @@ const FlyoutMenu = ({ const toggleValue = index === openedSubMenu ? null : index setOpenedSubMenu(toggleValue) } + const ref = useRef(null) + + useEffect(() => { + if (ref.current && ref.current?.children[0]) { + ref.current.children[0].focus() + } + }, []) return ( -
    +
    {Children.map(children, (child, index) => isValidElement(child) diff --git a/components/menu/src/menu-item/menu-item.js b/components/menu/src/menu-item/menu-item.js index bd25590776..f8c1035de0 100644 --- a/components/menu/src/menu-item/menu-item.js +++ b/components/menu/src/menu-item/menu-item.js @@ -3,7 +3,7 @@ import { Portal } from '@dhis2-ui/portal' import { IconChevronRight24 } from '@dhis2/ui-icons' import cx from 'classnames' import PropTypes from 'prop-types' -import React, { forwardRef, useRef } from 'react' +import React, { forwardRef, useCallback, useRef, useState } from 'react' import { FlyoutMenu } from '../index.js' import styles from './menu-item.styles.js' @@ -49,6 +49,32 @@ const MenuItem = forwardRef(function MenuItem( ref ) { const menuItemRef = useRef() + const [openSubMenu, setOpenSubmenu] = useState(false) + const [subMenu, setSubMenu] = useState([]) + + const handleKeyDown = useCallback( + (event) => { + setSubMenu(document.querySelectorAll('[data-submenu-open=true]')) + switch (event.key) { + case 'ArrowRight': + event.preventDefault() + if (event.target.getAttribute('aria-haspopup')) { + setOpenSubmenu(true) + } + break + case 'ArrowLeft': + event.preventDefault() + if (subMenu.length) { + subMenu[subMenu.length - 1].focus() + } + setOpenSubmenu(false) + break + default: + return + } + }, + [subMenu] + ) return ( <> @@ -86,7 +112,7 @@ const MenuItem = forwardRef(function MenuItem( : 'menuitem' } aria-haspopup={children && 'menu'} - aria-expanded={showSubMenu} + aria-expanded={showSubMenu || openSubMenu} aria-disabled={disabled} aria-checked={checkbox && checked} aria-label={label} @@ -94,6 +120,8 @@ const MenuItem = forwardRef(function MenuItem( children && `popper-${label.split(' ').join('-').toLowerCase()}` } + data-submenu-open={children ? openSubMenu : null} + onKeyDown={handleKeyDown} > {icon && {icon}} @@ -110,7 +138,7 @@ const MenuItem = forwardRef(function MenuItem( - {children && showSubMenu && ( + {children && (showSubMenu || openSubMenu) && ( { return obj } - const focusFirstFocusableItem = useCallback( + const handleFocus = useCallback( (e) => { const focusableItems = findFocusableItems() if (e.target === menuRef.current) { if (focusedIndex === -1) { setFocusedIndex(~~Object.keys(focusableItems)[0]) - // Object.values(focusableItems)[0].focus() } } }, @@ -96,33 +95,12 @@ const Menu = ({ children, className, dataTest, dense }) => { }) ) break - case 'ArrowRight': - event.preventDefault() - if (event.target.hasAttribute('aria-haspopup')) { - event.target.click() - } - break - case 'ArrowLeft': - event.preventDefault() - document - .querySelector( - `[aria-owns='${event.target.parentNode.parentNode.parentNode.parentNode.getAttribute( - 'id' - )}']` - ) - .focus() - document - .querySelector( - `[aria-owns='${event.target.parentNode.parentNode.parentNode.parentNode.getAttribute( - 'id' - )}']` - ) - .click() - break case ' ': case 'Enter': event.preventDefault() - event.target.click() + if (event.target.getAttribute('aria-disabled') === null) { + event.target.click() + } break default: return @@ -144,14 +122,14 @@ const Menu = ({ children, className, dataTest, dense }) => { } const menu = menuRef.current - menu.addEventListener('focus', focusFirstFocusableItem) + menu.addEventListener('focus', handleFocus) menu.addEventListener('keydown', handleKeyDown) return () => { menu.removeEventListener('keydown', handleKeyDown) - menu.removeEventListener('focus', focusFirstFocusableItem) + menu.removeEventListener('focus', handleFocus) } - }, [focusFirstFocusableItem, handleKeyDown]) + }, [handleFocus, handleKeyDown]) return (
      { showSubMenu: child.props.children ? child.props.showSubMenu : null, - ref: (node) => { - const role = node?.getAttribute('role') - if ( - [ - 'menuitem', - 'menuitemradio', - 'menuitemcheckbox', - ].includes(role) - ) { - return (itemsRefs.current[index] = node) - } else { - return null - } - }, + ...(child.props.dataTest && + child.props.dataTest.includes('menuitem') + ? { + ref: (node) => { + const role = node?.getAttribute('role') + if ( + [ + 'menuitem', + 'menuitemradio', + 'menuitemcheckbox', + ].includes(role) + ) { + return (itemsRefs.current[index] = + node) + } + }, + } + : {}), }) : child })}