Merge pull request #16977 from desktop/list-focus-styles-clone-repo-fix

Fix "Clone a Repository" repository list focus issues

app/src/ui/app.tsx

@@ -2839,6 +2839,11 @@ export class App extends React.Component<IAppProps, IAppState> {
       top: 0,
     }
 
+    /** The dropdown focus trap will stop focus event propagation we made need
+     * in some of our dialogs (noticed with Lists). Disabled this when dialogs
+     * are open */
+    const enableFocusTrap = this.state.currentPopup === null
+
     return (
       <ToolbarDropdown
         icon={icon}
@@ -2850,6 +2855,7 @@ export class App extends React.Component<IAppProps, IAppState> {
         onDropdownStateChanged={this.onRepositoryDropdownStateChanged}
         dropdownContentRenderer={this.renderRepositoryList}
         dropdownState={currentState}
+        enableFocusTrap={enableFocusTrap}
       />
     )
   }
@@ -2932,6 +2938,11 @@ export class App extends React.Component<IAppProps, IAppState> {
       aheadBehind
     )
 
+    /** The dropdown focus trap will stop focus event propagation we made need
+     * in some of our dialogs (noticed with Lists). Disabled this when dialogs
+     * are open */
+    const enableFocusTrap = this.state.currentPopup === null
+
     return (
       <PushPullButton
         dispatcher={this.props.dispatcher}
@@ -2952,6 +2963,7 @@ export class App extends React.Component<IAppProps, IAppState> {
         isDropdownOpen={isDropdownOpen}
         askForConfirmationOnForcePush={this.state.askForConfirmationOnForcePush}
         onDropdownStateChanged={this.onPushPullDropdownStateChanged}
+        enableFocusTrap={enableFocusTrap}
       />
     )
   }
@@ -3046,6 +3058,11 @@ export class App extends React.Component<IAppProps, IAppState> {
     const repository = selection.repository
     const { branchesState } = selection.state
 
+    /** The dropdown focus trap will stop focus event propagation we made need
+     * in some of our dialogs (noticed with Lists). Disabled this when dialogs
+     * are open */
+    const enableFocusTrap = this.state.currentPopup === null
+
     return (
       <BranchDropdown
         dispatcher={this.props.dispatcher}
@@ -3062,6 +3079,7 @@ export class App extends React.Component<IAppProps, IAppState> {
         }
         showCIStatusPopover={this.state.showCIStatusPopover}
         emoji={this.state.emoji}
+        enableFocusTrap={enableFocusTrap}
       />
     )
   }

app/src/ui/lib/filter-list.tsx

@@ -358,7 +358,9 @@ export class FilterList<T extends IFilterListItem> extends React.Component<
           rowCount={this.state.rows.length}
           rowRenderer={this.renderRow}
           rowHeight={this.props.rowHeight}
-          selectedRows={[this.state.selectedRow]}
+          selectedRows={
+            this.state.selectedRow === -1 ? [] : [this.state.selectedRow]
+          }
           onSelectedRowChanged={this.onSelectedRowChanged}
           onRowClick={this.onRowClick}
           onRowKeyDown={this.onRowKeyDown}

app/src/ui/lib/list/list.tsx

@@ -18,6 +18,7 @@ import { range } from '../../../lib/range'
 import { ListItemInsertionOverlay } from './list-item-insertion-overlay'
 import { DragData, DragType } from '../../../models/drag-drop'
 import memoizeOne from 'memoize-one'
+import { sendNonFatalException } from '../../../lib/helpers/non-fatal-exception'
 
 /**
  * Describe the first argument given to the cellRenderer,
@@ -1027,6 +1028,18 @@ export class List extends React.Component<IListProps, IListState> {
    * @param height - The height of the Grid as given by AutoSizer
    */
   private renderGrid(width: number, height: number) {
+    // It is possible to send an invalid array such as [-1] to this component,
+    // if you do, you get weird focus problems. We shouldn't be doing this.. but
+    // if we do, send a non fatal exception to tell us about it.
+    if (this.props.selectedRows[0] < 0) {
+      sendNonFatalException(
+        'The selected rows of the List.tsx contained a negative number.',
+        new Error(
+          `Invalid selected rows that contained a negative number passed to List component. This will cause keyboard navigation and focus problems.`
+        )
+      )
+    }
+
     // The currently selected list item is focusable but if there's no focused
     // item the list itself needs to be focusable so that you can reach it with
     // keyboard navigation and select an item.

app/src/ui/toolbar/branch-dropdown.tsx

@@ -63,6 +63,15 @@ interface IBranchDropdownProps {
 
   /** Map from the emoji shortcut (e.g., :+1:) to the image's local path. */
   readonly emoji: Map<string, string>
+
+  /** Whether the dropdown will trap focus or not. Defaults to true.
+   *
+   * Example of usage: If a dropdown is open and then a dialog subsequently, the
+   * focus trap logic will stop propagation of the focus event to the dialog.
+   * Thus, we want to disable this when dialogs are open since they will be
+   * using the dialog focus management.
+   */
+  readonly enableFocusTrap: boolean
 }
 
 /**
@@ -106,7 +115,7 @@ export class BranchDropdown extends React.Component<IBranchDropdownProps> {
   }
 
   public render() {
-    const { repositoryState } = this.props
+    const { repositoryState, enableFocusTrap } = this.props
     const { branchesState, checkoutProgress, changesState } = repositoryState
     const { tip } = branchesState
     const { conflictState } = changesState
@@ -196,6 +205,7 @@ export class BranchDropdown extends React.Component<IBranchDropdownProps> {
           onMouseEnter={this.onMouseEnter}
           onlyShowTooltipWhenOverflowed={true}
           isOverflowed={isDescriptionOverflowed}
+          enableFocusTrap={enableFocusTrap}
         >
           {this.renderPullRequestInfo()}
         </ToolbarDropdown>

app/src/ui/toolbar/dropdown.tsx

@@ -114,7 +114,13 @@ export interface IToolbarDropdownProps {
   /** The button's style. Defaults to `ToolbarButtonStyle.Standard`. */
   readonly style?: ToolbarButtonStyle
 
-  /** Whether the dropdown will trap focus or not. Defaults to true. */
+  /** Whether the dropdown will trap focus or not. Defaults to true.
+   *
+   * Example of usage: If a dropdown is open and then a dialog subsequently, the
+   * focus trap logic will stop propagation of the focus event to the dialog.
+   * Thus, we want to disable this when dialogs are open since they will be
+   * using the HTML build in dialog focus management.
+   */
   readonly enableFocusTrap?: boolean
 
   /**

app/src/ui/toolbar/push-pull-button.tsx

@@ -80,6 +80,15 @@ interface IPushPullButtonProps {
   /** Will the app prompt the user to confirm a force push? */
   readonly askForConfirmationOnForcePush: boolean
 
+  /** Whether the dropdown will trap focus or not. Defaults to true.
+   *
+   * Example of usage: If a dropdown is open and then a dialog subsequently, the
+   * focus trap logic will stop propagation of the focus event to the dialog.
+   * Thus, we want to disable this when dialogs are open since they will be
+   * using the dialog focus management.
+   */
+  readonly enableFocusTrap: boolean
+
   /**
    * An event handler for when the drop down is opened, or closed, by a pointer
    * event or by pressing the space or enter key while focused.
@@ -180,6 +189,7 @@ export class PushPullButton extends React.Component<IPushPullButtonProps> {
       dropdownStyle: ToolbarDropdownStyle.MultiOption,
       ariaLabel: 'Push, pull, fetch options',
       dropdownState: this.props.isDropdownOpen ? 'open' : 'closed',
+      enableFocusTrap: this.props.enableFocusTrap,
       onDropdownStateChanged: this.props.onDropdownStateChanged,
     }
   }