diff --git a/packages/ui-menu/src/Menu/MenuItem/index.tsx b/packages/ui-menu/src/Menu/MenuItem/index.tsx index 1028396ddc..e9b8e29ecc 100644 --- a/packages/ui-menu/src/Menu/MenuItem/index.tsx +++ b/packages/ui-menu/src/Menu/MenuItem/index.tsx @@ -187,8 +187,6 @@ class MenuItem extends Component { return 'menuitemcheckbox' case 'radio': return 'menuitemradio' - case 'flyout': - return 'button' default: return 'menuitem' } diff --git a/packages/ui-menu/src/Menu/MenuItemGroup/__new-tests__/MenuItemGroup.test.tsx b/packages/ui-menu/src/Menu/MenuItemGroup/__new-tests__/MenuItemGroup.test.tsx index 41e42f27be..2c70867435 100644 --- a/packages/ui-menu/src/Menu/MenuItemGroup/__new-tests__/MenuItemGroup.test.tsx +++ b/packages/ui-menu/src/Menu/MenuItemGroup/__new-tests__/MenuItemGroup.test.tsx @@ -40,44 +40,11 @@ describe('', () => { ) - const group = container.querySelector("[id*='MenuItemGroup_']") - const groupMenu = screen.getByRole('menu') - + const group = container.querySelector("[class*='menuItemGroup']") expect(group).toBeInTheDocument() expect(group).toHaveTextContent('Menu Label') - - expect(groupMenu).toBeInTheDocument() - expect(groupMenu).toHaveTextContent('Item Text 1') - expect(groupMenu).toHaveTextContent('Item Text 2') - }) - - it('should set the role to "menu"', () => { - const { container } = render( - - Foo - Bar - - - ) - const menuItemGroup = container.querySelector( - "[class*='menuItemGroup__items']" - ) - - expect(menuItemGroup).toHaveAttribute('role', 'menu') - }) - - it('should set the list item role to "none"', () => { - render( - - Food - Bar - - ) - const menu = screen.getByRole('menu') - const menuListItem = menu.firstChild as HTMLElement - - expect(menuListItem.tagName).toBe('LI') - expect(menuListItem).toHaveAttribute('role', 'none') + expect(group).toHaveTextContent('Item Text 1') + expect(group).toHaveTextContent('Item Text 2') }) it('should default to children with type "radio"', () => { @@ -114,10 +81,8 @@ describe('', () => { ) - const menu = screen.getByRole('menu') const menuItems = screen.getAllByRole('menuitemradio') - expect(menu).toHaveAttribute('aria-disabled', 'true') expect(menuItems).toHaveLength(2) expect(menuItems[0]).toHaveAttribute('aria-disabled', 'true') expect(menuItems[1]).toHaveAttribute('aria-disabled', 'true') diff --git a/packages/ui-menu/src/Menu/MenuItemGroup/index.tsx b/packages/ui-menu/src/Menu/MenuItemGroup/index.tsx index 90cf521d91..4b62cffdad 100644 --- a/packages/ui-menu/src/Menu/MenuItemGroup/index.tsx +++ b/packages/ui-menu/src/Menu/MenuItemGroup/index.tsx @@ -82,11 +82,8 @@ class MenuItemGroup extends Component { selected: this.selectedFromChildren(props) || props.defaultSelected! } } - - this._labelId = props.deterministicId!('MenuItemGroup') } - private _labelId: string ref: Element | null = null handleRef = (el: Element | null) => { @@ -210,23 +207,18 @@ class MenuItemGroup extends Component { ++index const value = child.props.value || index - return ( -
  • - {' '} - {safeCloneElement(child, { - tabIndex: isTabbable && index === 0 ? 0 : -1, - controls, - value, - children: child.props.children, - type: allowMultiple ? 'checkbox' : 'radio', - ref: this.props.itemRef, - disabled: disabled || child.props.disabled, - selected: this.selected.indexOf(value) > -1, - onSelect: this.handleSelect, - onMouseOver - })}{' '} -
  • - ) + return safeCloneElement(child, { + tabIndex: isTabbable && index === 0 ? 0 : -1, + controls, + value, + children: child.props.children, + type: allowMultiple ? 'checkbox' : 'radio', + ref: this.props.itemRef, + disabled: disabled || child.props.disabled, + selected: this.selected.indexOf(value) > -1, + onSelect: this.handleSelect, + onMouseOver + }) } else { return child } @@ -239,18 +231,15 @@ class MenuItemGroup extends Component { - {this.renderLabel()} -
      {this.renderChildren()} -
    +
    ) } diff --git a/packages/ui-menu/src/Menu/MenuItemSeparator/index.tsx b/packages/ui-menu/src/Menu/MenuItemSeparator/index.tsx index e4ef40b94e..3a45fd21e6 100644 --- a/packages/ui-menu/src/Menu/MenuItemSeparator/index.tsx +++ b/packages/ui-menu/src/Menu/MenuItemSeparator/index.tsx @@ -66,6 +66,8 @@ class MenuItemSeparator extends Component { render() { const props = omitProps(this.props, MenuItemSeparator.allowedProps) + // role="separator" would fit better here, but it causes NVDA to stop the + // MenuItem count after it return (
    { _popover: Popover | null = null _trigger: MenuItem | (React.ReactInstance & { focus?: () => void }) | null = null - _menu: HTMLUListElement | null = null + _menu: HTMLElement | null = null _labelId = this.props.deterministicId!('Menu__label') _activeSubMenu?: Menu | null @@ -103,7 +103,7 @@ class Menu extends Component { ref: Element | null = null - handleRef = (el: HTMLUListElement | null) => { + handleRef = (el: HTMLElement | null) => { const { menuRef } = this.props this._menu = el if (typeof menuRef === 'function') { @@ -169,7 +169,7 @@ class Menu extends Component { } } - handleMenuKeyDown = (event: React.KeyboardEvent) => { + handleMenuKeyDown = (event: React.KeyboardEvent) => { const key = event && event.keyCode const { down, up, tab, left } = keycode.codes const pgdn = keycode.codes['page down'] @@ -350,71 +350,59 @@ class Menu extends Component { this.props.controls if (matchComponentTypes(child, ['MenuItem'])) { - return ( -
  • - {safeCloneElement(child, { - controls, - children: child.props.children, - disabled: disabled || child.props.disabled, - onFocus: this.handleMenuItemFocus, - onBlur: this.handleMenuItemBlur, - onSelect: this.handleMenuItemSelect, - onMouseOver: this.handleMenuItemMouseOver, - tabIndex: isTabbable ? 0 : -1 - })} -
  • - ) + return safeCloneElement(child, { + controls, + children: child.props.children, + disabled: disabled || child.props.disabled, + onFocus: this.handleMenuItemFocus, + onBlur: this.handleMenuItemBlur, + onSelect: this.handleMenuItemSelect, + onMouseOver: this.handleMenuItemMouseOver, + tabIndex: isTabbable ? 0 : -1 + }) } if (matchComponentTypes(child, ['MenuItemGroup'])) { - return ( -
  • - {safeCloneElement(child, { - label: child.props.label, - controls, - disabled: disabled || child.props.disabled, - onFocus: this.handleMenuItemFocus, - onBlur: this.handleMenuItemBlur, - onSelect: this.handleMenuItemSelect, - onMouseOver: this.handleMenuItemMouseOver, - isTabbable - })} -
  • - ) + return safeCloneElement(child, { + label: child.props.label, + controls, + disabled: disabled || child.props.disabled, + onFocus: this.handleMenuItemFocus, + onBlur: this.handleMenuItemBlur, + onSelect: this.handleMenuItemSelect, + onMouseOver: this.handleMenuItemMouseOver, + isTabbable + }) } if (matchComponentTypes(child, ['Menu'])) { const submenuDisabled = disabled || child.props.disabled - return ( -
  • - {safeCloneElement(child, { - type: 'flyout', - controls, - disabled: submenuDisabled, - onSelect: this.handleMenuItemSelect, - placement: 'end top', - offsetX: -5, - offsetY: 5, - withArrow: false, - onToggle: this.handleSubMenuToggle, - onDismiss: this.handleSubMenuDismiss, - trigger: ( - - {child.props.title || child.props.label} - - ) - })} -
  • - ) + return safeCloneElement(child, { + type: 'flyout', + controls, + disabled: submenuDisabled, + onSelect: this.handleMenuItemSelect, + placement: 'end top', + offsetX: -5, + offsetY: 5, + withArrow: false, + onToggle: this.handleSubMenuToggle, + onDismiss: this.handleSubMenuDismiss, + trigger: ( + + {child.props.title || child.props.label} + + ) + }) } return } @@ -434,7 +422,7 @@ class Menu extends Component { registerMenuItem: this.registerMenuItem }} > -
      { ref={this.handleRef} > {this.renderChildren()} -
    +
    ) } diff --git a/packages/ui-menu/src/Menu/props.ts b/packages/ui-menu/src/Menu/props.ts index abc1b3eecb..8907d405d1 100644 --- a/packages/ui-menu/src/Menu/props.ts +++ b/packages/ui-menu/src/Menu/props.ts @@ -111,15 +111,15 @@ type MenuOwnProps = { /** * Callback fired on the onKeyDown of the `` */ - onKeyDown?: (event: React.KeyboardEvent) => void + onKeyDown?: (event: React.KeyboardEvent) => void /** * Callback fired on the onKeyUp of the `` */ - onKeyUp?: (event: React.KeyboardEvent) => void + onKeyUp?: (event: React.KeyboardEvent) => void /** * A function that returns a reference to the `` */ - menuRef?: (el: HTMLUListElement | null) => void + menuRef?: (el: HTMLElement | null) => void /** * A function that returns a reference to the `` */