Merge pull request desktop#16315 from desktop/windows-app-menu-screen-reader-fixes

Windows app menu screen reader fixes

Author: tidy-dev

app/src/ui/app-menu/app-menu-bar-button.tsx

@@ -202,9 +202,11 @@ export class AppMenuBarButton extends React.Component<
         onMouseEnter={this.onMouseEnter}
         onKeyDown={this.onKeyDown}
         tabIndex={-1}
-        role="menuitem"
+        buttonRole="menuitem"
+        buttonAriaHaspopup="menu"
       >
         <MenuListItem
+          menuItemId={`app-menu-${item.label}`}
           item={item}
           highlightAccessKey={this.props.highlightMenuAccessKey}
           renderAcceleratorText={false}
@@ -255,7 +257,7 @@ export class AppMenuBarButton extends React.Component<
     if (this.isMenuOpen) {
       this.props.onClose(this.props.menuItem, source)
     } else {
-      this.props.onOpen(this.props.menuItem)
+      this.props.onOpen(this.props.menuItem, true)
     }
   }
 
@@ -274,6 +276,7 @@ export class AppMenuBarButton extends React.Component<
         state={menuState}
         enableAccessKeyNavigation={this.props.enableAccessKeyNavigation}
         autoHeight={true}
+        ariaLabelledby={`app-menu-${this.props.menuItem.label}`}
       />
     )
   }

app/src/ui/app-menu/app-menu.tsx

@@ -49,6 +49,9 @@ interface IAppMenuProps {
    * @default false
    */
   readonly autoHeight?: boolean
+
+  /** The id of the element that serves as the menu's accessibility label */
+  readonly ariaLabelledby: string
 }
 
 export interface IKeyboardCloseSource {
@@ -85,22 +88,6 @@ function menuPaneClassNameFromId(id: string) {
 }
 
 export class AppMenu extends React.Component<IAppMenuProps, {}> {
-  /**
-   * The index of the menu pane that should receive focus after the
-   * next render. Default value is -1. This field is cleared after
-   * each successful focus operation.
-   */
-  private focusPane: number = -1
-
-  /**
-   * A mapping between pane index (depth) and actual MenuPane instances.
-   * This is used in order to (indirectly) call the focus method on the
-   * underlying List instances.
-   *
-   * See focusPane and ensurePaneFocus
-   */
-  private paneRefs: MenuPane[] = []
-
   /**
    * A numeric reference to a setTimeout timer id which is used for
    * opening and closing submenus after a delay.
@@ -109,29 +96,6 @@ export class AppMenu extends React.Component<IAppMenuProps, {}> {
    */
   private expandCollapseTimer: number | null = null
 
-  public constructor(props: IAppMenuProps) {
-    super(props)
-    this.focusPane = props.state.length - 1
-    this.receiveProps(null, props)
-  }
-
-  private receiveProps(
-    currentProps: IAppMenuProps | null,
-    nextProps: IAppMenuProps
-  ) {
-    if (nextProps.openedWithAccessKey) {
-      // We only want to react to the openedWithAccessKey prop once, either
-      // when it goes from false to true or when we receive it as our first
-      // prop. By doing it this way we save ourselves having to go through
-      // the dispatcher and updating the value once we've received it.
-      if (!currentProps || !currentProps.openedWithAccessKey) {
-        // Since we were opened with an access key we auto set focus to the
-        // last pane opened.
-        this.focusPane = nextProps.state.length - 1
-      }
-    }
-  }
-
   private onItemClicked = (
     depth: number,
     item: MenuItem,
@@ -154,9 +118,6 @@ export class AppMenu extends React.Component<IAppMenuProps, {}> {
       this.props.dispatcher.setAppMenuState(menu =>
         menu.withOpenedMenu(item, sourceIsAccessKey)
       )
-      if (source.kind === 'keyboard') {
-        this.focusPane = depth + 1
-      }
     } else if (item.type !== 'separator') {
       // Send the close event before actually executing the item so that
       // the menu can restore focus to the previously selected element
@@ -185,7 +146,6 @@ export class AppMenu extends React.Component<IAppMenuProps, {}> {
           menu.withClosedMenu(this.props.state[depth])
         )
 
-        this.focusPane = depth - 1
         event.preventDefault()
       }
     } else if (event.key === 'ArrowRight') {
@@ -196,7 +156,6 @@ export class AppMenu extends React.Component<IAppMenuProps, {}> {
         this.props.dispatcher.setAppMenuState(menu =>
           menu.withOpenedMenu(selectedItem, true)
         )
-        this.focusPane = depth + 1
         event.preventDefault()
       }
     }
@@ -255,12 +214,6 @@ export class AppMenu extends React.Component<IAppMenuProps, {}> {
     }
   }
 
-  private onMenuPaneRef = (pane: MenuPane | null) => {
-    if (pane) {
-      this.paneRefs[pane.props.depth] = pane
-    }
-  }
-
   private onPaneMouseEnter = (depth: number) => {
     this.clearExpandCollapseTimer()
 
@@ -275,8 +228,6 @@ export class AppMenu extends React.Component<IAppMenuProps, {}> {
       // This ensures that the selection to this menu is reset.
       this.props.dispatcher.setAppMenuState(m => m.withDeselectedMenu(paneMenu))
     }
-
-    this.focusPane = depth
   }
 
   private onKeyDown = (event: React.KeyboardEvent<HTMLElement>) => {
@@ -294,7 +245,6 @@ export class AppMenu extends React.Component<IAppMenuProps, {}> {
     return (
       <MenuPane
         key={key}
-        ref={this.onMenuPaneRef}
         className={className}
         depth={depth}
         items={menu.items}
@@ -305,6 +255,7 @@ export class AppMenu extends React.Component<IAppMenuProps, {}> {
         onSelectionChanged={this.onSelectionChanged}
         enableAccessKeyNavigation={this.props.enableAccessKeyNavigation}
         onClearSelection={this.onClearSelection}
+        ariaLabelledby={this.props.ariaLabelledby}
       />
     )
   }
@@ -313,10 +264,6 @@ export class AppMenu extends React.Component<IAppMenuProps, {}> {
     const menus = this.props.state
     const panes = menus.map((m, depth) => this.renderMenuPane(depth, m))
 
-    // Clear out any old references we might have to panes that are
-    // no longer displayed.
-    this.paneRefs = this.paneRefs.slice(0, panes.length)
-
     return (
       // eslint-disable-next-line jsx-a11y/no-static-element-interactions
       <div id="app-menu-foldout" onKeyDown={this.onKeyDown}>
@@ -325,32 +272,6 @@ export class AppMenu extends React.Component<IAppMenuProps, {}> {
     )
   }
 
-  /**
-   * Called after mounting or re-rendering and ensures that the
-   * appropriate (if any) MenuPane list receives keyboard focus.
-   */
-  private ensurePaneFocus() {
-    if (this.focusPane >= 0) {
-      const pane = this.paneRefs[this.focusPane]
-      if (pane) {
-        pane.focus()
-        this.focusPane = -1
-      }
-    }
-  }
-
-  public componentWillReceiveProps(nextProps: IAppMenuProps) {
-    this.receiveProps(this.props, nextProps)
-  }
-
-  public componentDidMount() {
-    this.ensurePaneFocus()
-  }
-
-  public componentDidUpdate() {
-    this.ensurePaneFocus()
-  }
-
   public componentWillUnmount() {
     this.clearExpandCollapseTimer()
   }

app/src/ui/app-menu/menu-list-item.tsx

@@ -10,6 +10,12 @@ import { getPlatformSpecificNameOrSymbolForModifier } from '../../lib/menu-item'
 interface IMenuListItemProps {
   readonly item: MenuItem
 
+  /**
+   * A unique identifier for the menu item. On use is  to link it to the menu
+   * for accessibility labelling.
+   */
+  readonly menuItemId?: string
+
   /**
    * Whether or not to highlight the access key of a menu item (if one exists).
    *
@@ -150,14 +156,16 @@ export class MenuListItem extends React.Component<IMenuListItemProps, {}> {
     })
 
     return (
-      // eslint-disable-next-line jsx-a11y/click-events-have-key-events, jsx-a11y/interactive-supports-focus
+      // eslint-disable-next-line jsx-a11y/click-events-have-key-events
       <div
+        id={this.props.menuItemId}
         className={className}
         onMouseEnter={this.onMouseEnter}
         onMouseLeave={this.onMouseLeave}
         onClick={this.onClick}
         ref={this.wrapperRef}
         role="menuitem"
+        tabIndex={-1}
       >
         {this.getIcon(item)}
         <div className="label">

app/src/ui/app-menu/menu-pane.tsx

@@ -89,11 +89,12 @@ interface IMenuPaneProps {
    * will be called when the user's pointer device leaves a menu item.
    */
   readonly onClearSelection: (depth: number) => void
+
+  /** The id of the element that serves as the menu's accessibility label */
+  readonly ariaLabelledby: string
 }
 
 export class MenuPane extends React.Component<IMenuPaneProps> {
-  private paneRef = React.createRef<HTMLDivElement>()
-
   private onRowClick = (
     item: MenuItem,
     event: React.MouseEvent<HTMLDivElement>
@@ -215,9 +216,9 @@ export class MenuPane extends React.Component<IMenuPaneProps> {
         className={className}
         onMouseEnter={this.onMouseEnter}
         onKeyDown={this.onKeyDown}
-        ref={this.paneRef}
         tabIndex={-1}
         role="menu"
+        aria-labelledby={this.props.ariaLabelledby}
       >
         {this.props.items
           .filter(x => x.visible)
@@ -236,10 +237,6 @@ export class MenuPane extends React.Component<IMenuPaneProps> {
       </div>
     )
   }
-
-  public focus() {
-    this.paneRef.current?.focus()
-  }
 }
 
 const supportedKeys = [

app/src/ui/lib/aria-types.ts

@@ -0,0 +1,3 @@
+import { HTMLAttributes } from 'react'
+
+export type AriaHasPopupType = HTMLAttributes<HTMLElement>['aria-haspopup']

app/src/ui/lib/button.tsx

@@ -2,6 +2,7 @@ import * as React from 'react'
 import classNames from 'classnames'
 import { Tooltip, TooltipDirection } from './tooltip'
 import { createObservableRef } from './observable-ref'
+import { AriaHasPopupType } from './aria-types'
 
 export interface IButtonProps {
   /**
@@ -74,9 +75,41 @@ export interface IButtonProps {
    */
   readonly tabIndex?: number
 
+  /**
+   * ARIA roles provide semantic meaning to content, allowing screen readers and
+   * other tools to present and support interaction with object in a way that is
+   * consistent with user expectations of that type of object. ARIA roles can be
+   * used to describe elements that don't natively exist in HTML or exist but
+   * don't yet have full browser support.
+   *
+   * By default, many semantic elements in HTML have a role; for example, <input
+   * type="radio"> has the "radio" role. Non-semantic elements in HTML do not
+   * have a role; <div> and <span> without added semantics return null. The role
+   * attribute can provide semantics.
+   */
   readonly role?: string
+
+  /**
+   * The aria-expanded attribute is set on an element to indicate if a control
+   * is expanded or collapsed, and whether or not the controlled elements are
+   * displayed or hidden.
+   *
+   * There are several widgets that can be expanded and collapsed, including
+   * menus, dialogs, and accordion panels. Each of these objects, in turn, has
+   * an interactive element that controls their opening and closing. The
+   * aria-expanded attribute is applied to this focusable, interactive control
+   * that toggles the visibility of the object.
+   */
   readonly ariaExpanded?: boolean
 
+  /** An aria attribute indicates the availability and type of interactive popup
+   * element that can be triggered by the element on which the attribute is set
+   *
+   * Notes: The value true is the same as menu. Any other value, including an
+   * empty string or other role, is treated as if false were set.
+   * */
+  readonly ariaHaspopup?: AriaHasPopupType
+
   /**
    * Typically the contents of a button serve the purpose of describing the
    * buttons use. However, ariaLabel can be used if the contents do not suffice.
@@ -142,6 +175,7 @@ export class Button extends React.Component<IButtonProps, {}> {
         aria-expanded={this.props.ariaExpanded}
         aria-disabled={disabled ? 'true' : undefined}
         aria-label={this.props.ariaLabel}
+        aria-haspopup={this.props.ariaHaspopup}
       >
         {tooltip && (
           <Tooltip

app/src/ui/toolbar/button.tsx

@@ -7,6 +7,7 @@ import { Button } from '../lib/button'
 import { clamp } from '../../lib/clamp'
 import { createObservableRef } from '../lib/observable-ref'
 import { Tooltip, TooltipDirection, TooltipTarget } from '../lib/tooltip'
+import { AriaHasPopupType } from '../lib/aria-types'
 
 /** The button style. */
 export enum ToolbarButtonStyle {
@@ -108,6 +109,7 @@ export interface IToolbarButtonProps {
 
   readonly role?: string
   readonly ariaExpanded?: boolean
+  readonly ariaHaspopup?: AriaHasPopupType
 
   /**
    * Whether to only show the tooltip when the tooltip target overflows its
@@ -221,6 +223,7 @@ export class ToolbarButton extends React.Component<IToolbarButtonProps, {}> {
           tabIndex={this.props.tabIndex}
           role={this.props.role}
           ariaExpanded={this.props.ariaExpanded}
+          ariaHaspopup={this.props.ariaHaspopup}
         >
           {progress}
           {icon}

app/src/ui/toolbar/dropdown.tsx

@@ -10,6 +10,7 @@ import classNames from 'classnames'
 import FocusTrap from 'focus-trap-react'
 import { Options as FocusTrapOptions } from 'focus-trap'
 import { TooltipTarget } from '../lib/tooltip'
+import { AriaHasPopupType } from '../lib/aria-types'
 
 export type DropdownState = 'open' | 'closed'
 
@@ -168,6 +169,7 @@ export interface IToolbarDropdownProps {
 
   readonly role?: string
   readonly buttonRole?: string
+  readonly buttonAriaHaspopup?: AriaHasPopupType
 
   /** Classes to be appended to `ToolbarButton` component */
   readonly buttonClassName?: string
@@ -448,6 +450,7 @@ export class ToolbarDropdown extends React.Component<
             this.props.onlyShowTooltipWhenOverflowed
           }
           isOverflowed={this.props.isOverflowed}
+          ariaHaspopup={this.props.buttonAriaHaspopup}
         >
           {this.props.children}
           {this.props.dropdownStyle !== ToolbarDropdownStyle.MultiOption &&