Merge pull request #17499 from desktop/better-diff-expand-button-focus-management

Better diff expand button focus management

app/src/ui/diff/side-by-side-diff-row.tsx

@@ -142,11 +142,11 @@ interface ISideBySideDiffRowProps {
   /** Called when the user changes the hide whitespace in diffs setting. */
   readonly onHideWhitespaceInDiffChanged: (checked: boolean) => void
 
-  /* This tracks the last expanded hunk index so that we can refocus the expander after rerender */
-  readonly lastExpandedHunk: {
-    index: number
-    expansionType: DiffHunkExpansionType
-  } | null
+  readonly onHunkExpansionRef: (
+    hunkIndex: number,
+    expansionType: DiffHunkExpansionType,
+    element: HTMLButtonElement | null
+  ) => void
 }
 
 interface ISideBySideDiffRowState {
@@ -424,9 +424,7 @@ export class SideBySideDiffRow extends React.Component<
           className="hunk-expansion-handle"
           style={{ width: this.lineGutterWidth }}
         >
-          <button onContextMenu={this.props.onContextMenuExpandHunk}>
-            <span></span>
-          </button>
+          <div className="hunk-expansion-placeholder" />
         </div>
       )
     }
@@ -436,25 +434,6 @@ export class SideBySideDiffRow extends React.Component<
       expansionType
     )
 
-    /**
-     * For accessibility, when a button is focused, it should maintain focus.
-     * This sets the autofocus of the button if the last expanded button at the
-     * position was the same type. The +1 is to handle the last hunk index which
-     * is one off, and if there are two hunks with the same expansion types on
-     * after each other we just want the first one and autofocus will go to the
-     * first one automatically.
-     *
-     * Other notes: the expand up buttons already worked. This is
-     * for expand all and expand down buttons.
-     */
-    const { lastExpandedHunk } = this.props
-    const focusButton =
-      lastExpandedHunk !== null &&
-      expansionType === lastExpandedHunk.expansionType
-        ? hunkIndex === lastExpandedHunk.index ||
-          hunkIndex === lastExpandedHunk.index + 1
-        : false
-
     return (
       <div
         className="hunk-expansion-handle selectable hoverable"
@@ -465,8 +444,8 @@ export class SideBySideDiffRow extends React.Component<
           onContextMenu={this.props.onContextMenuExpandHunk}
           tooltip={elementInfo.title}
           toolTipDirection={TooltipDirection.SOUTH}
-          autoFocus={focusButton}
           ariaLabel={elementInfo.title}
+          onButtonRef={this.getOnHunkExpansionRef(hunkIndex, expansionType)}
         >
           <Octicon symbol={elementInfo.icon} />
         </Button>
@@ -474,6 +453,12 @@ export class SideBySideDiffRow extends React.Component<
     )
   }
 
+  private getOnHunkExpansionRef =
+    (hunkIndex: number, expansionType: DiffHunkExpansionType) =>
+    (button: HTMLButtonElement | null) => {
+      this.props.onHunkExpansionRef(hunkIndex, expansionType, button)
+    }
+
   private renderHunkHeaderGutter(
     hunkIndex: number,
     expansionType: DiffHunkExpansionType

app/src/ui/diff/side-by-side-diff.tsx

@@ -62,6 +62,7 @@ import {
 } from './text-diff-expansion'
 import { IMenuItem } from '../../lib/menu-item'
 import { HiddenBidiCharsWarning } from './hidden-bidi-chars-warning'
+import { findDOMNode } from 'react-dom'
 import escapeRegExp from 'lodash/escapeRegExp'
 
 const DefaultRowHeight = 20
@@ -204,7 +205,7 @@ interface ISideBySideDiffState {
 
   /** This tracks the last expanded hunk index so that we can refocus the expander after rerender */
   readonly lastExpandedHunk: {
-    index: number
+    hunkIndex: number
     expansionType: DiffHunkExpansionType
   } | null
 }
@@ -227,6 +228,8 @@ export class SideBySideDiff extends React.Component<
   private textSelectionStartRow: number | undefined = undefined
   private textSelectionEndRow: number | undefined = undefined
 
+  private readonly hunkExpansionRefs = new Map<string, HTMLButtonElement>()
+
   public constructor(props: ISideBySideDiffProps) {
     super(props)
 
@@ -408,6 +411,92 @@ export class SideBySideDiff extends React.Component<
         }
       }
     }
+
+    if (this.state.lastExpandedHunk !== prevState.lastExpandedHunk) {
+      this.focusAfterLastExpandedHunkChange()
+    }
+  }
+
+  private focusListElement = () => {
+    const diffNode = findDOMNode(this.virtualListRef.current)
+    const diff = diffNode instanceof HTMLElement ? diffNode : null
+    diff?.focus()
+  }
+
+  /**
+   * This handles app focus after a user has clicked on an diff expansion
+   * button. With the exception of the top expand up button, the expansion
+   * buttons disappear after clicking and by default the focus moves to the app
+   * body. This is not ideal for accessibilty as a keyboard user must then tab
+   * all the way back to the diff to continut to interact with it.
+   *
+   * If an expansion button of the type clicked is available, we focus it.
+   * Otherwise, we try to find the next closest expansion button and focus that.
+   * If no expansion buttons available, we focus the diff container. This makes
+   * it so if a user expands down and can expand down further, they will
+   * automatically be focused on the next expand down.
+   *
+   * Other context:
+   * - When a user clicks on a diff expansion button, the
+   * lastExpandedHunk state is updated. In the componentDidUpdate, we detect
+   * that change in order to call this after the new expansion buttons have
+   * rendered. The rendered expansion buttons are stored in a map.
+   * - A hunk index may have multiple expansion buttons (up and down) so it does
+   *   not uniquely identify a button.
+   */
+  private focusAfterLastExpandedHunkChange() {
+    if (this.state.lastExpandedHunk === null) {
+      return
+    }
+
+    // No expansion buttons? Focus the diff
+    if (this.hunkExpansionRefs.size === 0) {
+      this.focusListElement()
+      return
+    }
+
+    const expansionHunkKeys = Array.from(this.hunkExpansionRefs.keys()).sort()
+    const { hunkIndex, expansionType } = this.state.lastExpandedHunk
+    const lastExpandedKey = `${hunkIndex}-${expansionType}`
+
+    // If there is a new hunk expansion button of same type in same place as the
+    // last, focus it
+    const lastExpandedHunkButton = this.hunkExpansionRefs.get(lastExpandedKey)
+    if (lastExpandedHunkButton) {
+      lastExpandedHunkButton.focus()
+      return
+    }
+
+    function getHunkKeyIndex(key: string) {
+      return parseInt(key.split('-').at(0) || '', 10)
+    }
+
+    // No?, Then try to focus the next closest hunk in tab order
+    const closestInTabOrder = expansionHunkKeys.find(
+      key => getHunkKeyIndex(key) >= hunkIndex
+    )
+
+    if (closestInTabOrder) {
+      const closetHunkButton = this.hunkExpansionRefs.get(closestInTabOrder)
+      closetHunkButton?.focus()
+      return
+    }
+
+    // No? Then try to focus the next closest hunk in reverse tab order
+    const closestInReverseTabOrder = expansionHunkKeys
+      .reverse()
+      .find(key => getHunkKeyIndex(key) <= hunkIndex)
+
+    if (closestInReverseTabOrder) {
+      const closetHunkButton = this.hunkExpansionRefs.get(
+        closestInReverseTabOrder
+      )
+      closetHunkButton?.focus()
+      return
+    }
+
+    // We should never get here, but just in case focus something!
+    this.focusListElement()
   }
 
   private canExpandDiff() {
@@ -583,13 +672,26 @@ export class SideBySideDiff extends React.Component<
             }
             beforeClassNames={beforeClassNames}
             afterClassNames={afterClassNames}
-            lastExpandedHunk={this.state.lastExpandedHunk}
+            onHunkExpansionRef={this.onHunkExpansionRef}
           />
         </div>
       </CellMeasurer>
     )
   }
 
+  private onHunkExpansionRef = (
+    hunkIndex: number,
+    expansionType: DiffHunkExpansionType,
+    button: HTMLButtonElement | null
+  ) => {
+    const key = `${hunkIndex}-${expansionType}`
+    if (button === null) {
+      this.hunkExpansionRefs.delete(key)
+    } else {
+      this.hunkExpansionRefs.set(key, button)
+    }
+  }
+
   private getRowHeight = (row: { index: number }) => {
     return listRowsHeightCache.rowHeight(row) ?? DefaultRowHeight
   }
@@ -925,7 +1027,7 @@ export class SideBySideDiff extends React.Component<
       return
     }
 
-    this.setState({ lastExpandedHunk: { index: hunkIndex, expansionType } })
+    this.setState({ lastExpandedHunk: { hunkIndex, expansionType } })
 
     const kind = expansionType === DiffHunkExpansionType.Down ? 'down' : 'up'