Merge pull request #17311 from desktop/fix-keyb-nav-lists

Fix keyboard navigation of filter lists

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

@@ -173,6 +173,7 @@ interface IFilterListState<T extends IFilterListItem> {
   readonly rows: ReadonlyArray<IFilterListRow<T>>
   readonly selectedRow: number
   readonly filterValue: string
+  readonly filterValueChanged: boolean
 }
 
 /**
@@ -199,17 +200,15 @@ export class FilterList<T extends IFilterListItem> extends React.Component<
   public constructor(props: IFilterListProps<T>) {
     super(props)
 
-    this.state = createStateUpdate(props)
+    if (props.filterTextBox !== undefined) {
+      this.filterTextBox = props.filterTextBox
     }
 
-  public componentWillMount() {
-    if (this.props.filterTextBox !== undefined) {
-      this.filterTextBox = this.props.filterTextBox
-    }
+    this.state = createStateUpdate(props, null)
   }
 
   public componentWillReceiveProps(nextProps: IFilterListProps<T>) {
-    this.setState(createStateUpdate(nextProps))
+    this.setState(createStateUpdate(nextProps, this.state))
   }
 
   public componentDidUpdate(
@@ -276,6 +275,23 @@ export class FilterList<T extends IFilterListItem> extends React.Component<
     )
   }
 
+  public renderLiveContainer() {
+    if (!this.state.filterValueChanged) {
+      return null
+    }
+
+    const itemRows = this.state.rows.filter(row => row.kind === 'item')
+    const resultsPluralized = itemRows.length === 1 ? 'result' : 'results'
+    const screenReaderMessage = `${itemRows.length} ${resultsPluralized}`
+
+    return (
+      <AriaLiveContainer
+        message={screenReaderMessage}
+        trackedUserInput={this.state.filterValue}
+      />
+    )
+  }
+
   public renderFilterRow() {
     if (this.props.hideFilterRow === true) {
       return null
@@ -290,16 +306,10 @@ export class FilterList<T extends IFilterListItem> extends React.Component<
   }
 
   public render() {
-    const itemRows = this.state.rows.filter(row => row.kind === 'item')
-    const resultsPluralized = itemRows.length === 1 ? 'result' : 'results'
-    const screenReaderMessage = `${itemRows.length} ${resultsPluralized}`
-
     return (
       <div className={classnames('filter-list', this.props.className)}>
-        <AriaLiveContainer
-          message={screenReaderMessage}
-          trackedUserInput={this.state.filterValue}
-        />
+        {this.renderLiveContainer()}
+
         {this.props.renderPreList ? this.props.renderPreList() : null}
 
         {this.renderFilterRow()}
@@ -577,7 +587,8 @@ export function getText<T extends IFilterListItem>(
 }
 
 function createStateUpdate<T extends IFilterListItem>(
-  props: IFilterListProps<T>
+  props: IFilterListProps<T>,
+  state: IFilterListState<T> | null
 ) {
   const flattenedRows = new Array<IFilterListRow<T>>()
   const filter = (props.filterText || '').toLowerCase()
@@ -618,7 +629,17 @@ function createStateUpdate<T extends IFilterListItem>(
     selectedRow = flattenedRows.findIndex(i => i.kind === 'item')
   }
 
-  return { rows: flattenedRows, selectedRow, filterValue: filter }
+  // Stay true if already set, otherwise become true if the filter has content
+  const filterValueChanged = state?.filterValueChanged
+    ? true
+    : filter.length > 0
+
+  return {
+    rows: flattenedRows,
+    selectedRow,
+    filterValue: filter,
+    filterValueChanged,
+  }
 }
 
 function getItemFromRowIndex<T extends IFilterListItem>(

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

@@ -711,6 +711,16 @@ export class List extends React.Component<IListProps, IListState> {
     return this.props.canSelectRow ? this.props.canSelectRow(rowIndex) : true
   }
 
+  private getFirstSelectableRowIndexPath(): number | null {
+    for (let i = 0; i < this.props.rowCount; i++) {
+      if (this.canSelectRow(i)) {
+        return i
+      }
+    }
+
+    return null
+  }
+
   private addSelection(direction: SelectionDirection, source: SelectionSource) {
     if (this.props.selectedRows.length === 0) {
       return this.moveSelection(direction, source)
@@ -955,16 +965,22 @@ export class List extends React.Component<IListProps, IListState> {
     return customClasses.length === 0 ? undefined : customClasses.join(' ')
   }
 
-  private renderRow = (params: IRowRendererParams) => {
+  private getRowRenderer = (firstSelectableRowIndex: number | null) => {
+    return (params: IRowRendererParams) => {
+      const { selectedRows } = this.props
       const rowIndex = params.rowIndex
       const selectable = this.canSelectRow(rowIndex)
-    const selected = this.props.selectedRows.indexOf(rowIndex) !== -1
+      const selected = selectedRows.indexOf(rowIndex) !== -1
       const customClasses = this.getCustomRowClassNames(rowIndex)
 
       // An unselectable row shouldn't be focusable
       let tabIndex: number | undefined = undefined
       if (selectable) {
-      tabIndex = selected && this.props.selectedRows[0] === rowIndex ? 0 : -1
+        tabIndex =
+          (selected && selectedRows[0] === rowIndex) ||
+          (selectedRows.length === 0 && firstSelectableRowIndex === rowIndex)
+            ? 0
+            : -1
       }
 
       const row = this.props.rowRenderer(rowIndex)
@@ -1016,6 +1032,7 @@ export class List extends React.Component<IListProps, IListState> {
         />
       )
     }
+  }
 
   public render() {
     let content: JSX.Element[] | JSX.Element | null
@@ -1132,7 +1149,9 @@ export class List extends React.Component<IListProps, IListState> {
           }
           rowCount={this.props.rowCount}
           rowHeight={this.props.rowHeight}
-          cellRenderer={this.renderRow}
+          cellRenderer={this.getRowRenderer(
+            this.getFirstSelectableRowIndexPath()
+          )}
           onScroll={this.onScroll}
           scrollTop={this.props.setScrollTop}
           overscanRowCount={4}

app/src/ui/lib/list/section-list-selection.ts

@@ -71,12 +71,21 @@ export interface ISelectAllSource {
   readonly kind: 'select-all'
 }
 
+/**
+ * Interface describing a user initiated selection change event originating
+ * when focusing the list when it has no selection.
+ */
+export interface IFocusSource {
+  readonly kind: 'focus'
+}
+
 /** A type union of possible sources of a selection changed event */
 export type SelectionSource =
   | IMouseClickSource
   | IHoverSource
   | IKeyboardSource
   | ISelectAllSource
+  | IFocusSource
 
 /**
  * Determine the next selectable row, given the direction and a starting

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

@@ -742,6 +742,11 @@ export class SectionList extends React.Component<
     // focused list item if it scrolls back into view.
     if (!focusWithin) {
       this.focusRow = InvalidRowIndexPath
+    } else if (this.props.selectedRows.length === 0) {
+      const firstSelectableRowIndexPath = this.getFirstSelectableRowIndexPath()
+      if (firstSelectableRowIndexPath !== null) {
+        this.moveSelectionTo(firstSelectableRowIndexPath, { kind: 'focus' })
+      }
     }
   }
 
@@ -810,6 +815,22 @@ export class SectionList extends React.Component<
     return this.props.canSelectRow ? this.props.canSelectRow(rowIndex) : true
   }
 
+  private getFirstSelectableRowIndexPath(): RowIndexPath | null {
+    const { rowCount } = this.props
+
+    for (let section = 0; section < rowCount.length; section++) {
+      const rowCountInSection = rowCount[section]
+      for (let row = 0; row < rowCountInSection; row++) {
+        const indexPath = { section, row }
+        if (this.canSelectRow(indexPath)) {
+          return indexPath
+        }
+      }
+    }
+
+    return null
+  }
+
   private addSelection(direction: SelectionDirection, source: SelectionSource) {
     if (this.props.selectedRows.length === 0) {
       return this.moveSelection(direction, source)
@@ -1110,8 +1131,12 @@ export class SectionList extends React.Component<
     return customClasses.length === 0 ? undefined : customClasses.join(' ')
   }
 
-  private getRowRenderer = (section: number) => {
+  private getRowRenderer = (
+    section: number,
+    firstSelectableRowIndexPath: RowIndexPath | null
+  ) => {
     return (params: IRowRendererParams) => {
+      const { selectedRows } = this.props
       const indexPath: RowIndexPath = {
         section: section,
         row: params.rowIndex,
@@ -1119,16 +1144,17 @@ export class SectionList extends React.Component<
 
       const selectable = this.canSelectRow(indexPath)
       const selected =
-        this.props.selectedRows.findIndex(r =>
-          rowIndexPathEquals(r, indexPath)
-        ) !== -1
+        selectedRows.findIndex(r => rowIndexPathEquals(r, indexPath)) !== -1
       const customClasses = this.getCustomRowClassNames(indexPath)
 
       // An unselectable row shouldn't be focusable
       let tabIndex: number | undefined = undefined
       if (selectable) {
         tabIndex =
-          selected && rowIndexPathEquals(this.props.selectedRows[0], indexPath)
+          (selected && rowIndexPathEquals(selectedRows[0], indexPath)) ||
+          (selectedRows.length === 0 &&
+            firstSelectableRowIndexPath !== null &&
+            rowIndexPathEquals(firstSelectableRowIndexPath, indexPath))
             ? 0
             : -1
       }
@@ -1303,7 +1329,10 @@ export class SectionList extends React.Component<
           columnCount={1}
           rowCount={this.props.rowCount[section]}
           rowHeight={this.getRowHeight(section)}
-          cellRenderer={this.getRowRenderer(section)}
+          cellRenderer={this.getRowRenderer(
+            section,
+            this.getFirstSelectableRowIndexPath()
+          )}
           scrollTop={relativeScrollTop}
           overscanRowCount={4}
           style={{ ...params.style, width: '100%' }}

app/src/ui/lib/list/selection.ts

@@ -62,12 +62,21 @@ export interface ISelectAllSource {
   readonly kind: 'select-all'
 }
 
+/**
+ * Interface describing a user initiated selection change event originating
+ * when focusing the list when it has no selection.
+ */
+export interface IFocusSource {
+  readonly kind: 'focus'
+}
+
 /** A type union of possible sources of a selection changed event */
 export type SelectionSource =
   | IMouseClickSource
   | IHoverSource
   | IKeyboardSource
   | ISelectAllSource
+  | IFocusSource
 
 /**
  * Determine the next selectable row, given the direction and a starting

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

@@ -166,6 +166,7 @@ interface IFilterListState<T extends IFilterListItem> {
   readonly rows: ReadonlyArray<ReadonlyArray<IFilterListRow<T>>>
   readonly selectedRow: RowIndexPath
   readonly filterValue: string
+  readonly filterValueChanged: boolean
   // Indices of groups in the filtered list
   readonly groups: ReadonlyArray<number>
 }
@@ -180,17 +181,15 @@ export class SectionFilterList<
   public constructor(props: ISectionFilterListProps<T>) {
     super(props)
 
-    this.state = createStateUpdate(props)
+    if (props.filterTextBox !== undefined) {
+      this.filterTextBox = props.filterTextBox
     }
 
-  public componentWillMount() {
-    if (this.props.filterTextBox !== undefined) {
-      this.filterTextBox = this.props.filterTextBox
-    }
+    this.state = createStateUpdate(props, null)
   }
 
   public componentWillReceiveProps(nextProps: ISectionFilterListProps<T>) {
-    this.setState(createStateUpdate(nextProps))
+    this.setState(createStateUpdate(nextProps, this.state))
   }
 
   public componentDidUpdate(
@@ -257,6 +256,23 @@ export class SectionFilterList<
     )
   }
 
+  public renderLiveContainer() {
+    if (!this.state.filterValueChanged) {
+      return null
+    }
+
+    const itemRows = this.state.rows.flat().filter(row => row.kind === 'item')
+    const resultsPluralized = itemRows.length === 1 ? 'result' : 'results'
+    const screenReaderMessage = `${itemRows.length} ${resultsPluralized}`
+
+    return (
+      <AriaLiveContainer
+        trackedUserInput={this.state.filterValue}
+        message={screenReaderMessage}
+      />
+    )
+  }
+
   public renderFilterRow() {
     if (this.props.hideFilterRow === true) {
       return null
@@ -271,16 +287,10 @@ export class SectionFilterList<
   }
 
   public render() {
-    const itemRows = this.state.rows.flat().filter(row => row.kind === 'item')
-    const resultsPluralized = itemRows.length === 1 ? 'result' : 'results'
-    const screenReaderMessage = `${itemRows.length} ${resultsPluralized}`
-
     return (
       <div className={classnames('filter-list', this.props.className)}>
-        <AriaLiveContainer
-          trackedUserInput={this.state.filterValue}
-          message={screenReaderMessage}
-        />
+        {this.renderLiveContainer()}
+
         {this.props.renderPreList ? this.props.renderPreList() : null}
 
         {this.renderFilterRow()}
@@ -614,7 +624,8 @@ function getFirstVisibleRow<T extends IFilterListItem>(
 }
 
 function createStateUpdate<T extends IFilterListItem>(
-  props: ISectionFilterListProps<T>
+  props: ISectionFilterListProps<T>,
+  state: IFilterListState<T> | null
 ) {
   const rows = new Array<Array<IFilterListRow<T>>>()
   const filter = (props.filterText || '').toLowerCase()
@@ -664,7 +675,18 @@ function createStateUpdate<T extends IFilterListItem>(
     selectedRow = getFirstVisibleRow(rows)
   }
 
-  return { rows: rows, selectedRow, filterValue: filter, groups: groupIndices }
+  // Stay true if already set, otherwise become true if the filter has content
+  const filterValueChanged = state?.filterValueChanged
+    ? true
+    : filter.length > 0
+
+  return {
+    rows: rows,
+    selectedRow,
+    filterValue: filter,
+    filterValueChanged,
+    groups: groupIndices,
+  }
 }
 
 function getItemFromRowIndex<T extends IFilterListItem>(