Skip to content

Conversation

@LFDanLu
Copy link
Member

@LFDanLu LFDanLu commented Mar 15, 2023

Closes

✅ Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

Test the various table section stories. For DnD, look for the "complex drag between tables (sections)" story in the Drag and Drop -> Util Handlers section

Known issues:

  • Reorder operations break if dropping onto a drop position adjacent to the dragged item
  • Reorder operations lose focus to the body

🧢 Your Project:

RSP

…thout

note: this is pretty broken, awaiting feedback from spectrum since it seems like there might be some cases that need to be handled
commited them so they wouldnt just be in my stash
using this information to get rid of the extra wrapping row group when there are sections
parent key of the section is the body but this key isnt in the keymap
renders extra whitespace for some reason
toggling selection on and off in a table with sections behaves strangely
pausing here since collectionbuilder will be changed in the future
…to a section

as per feedback from spectrum design meeting
…xist

also make ListView section have title as a required prop
@rspbot
Copy link

rspbot commented Apr 24, 2023

LFDanLu added 4 commits April 24, 2023 15:32
includes an api update to keyboard delegate methods and table/grid hooks return the keyboard delegate now
fixes case where row column header was a valid drop target, empty section drop indicator, erroneous skipping of after drop position when going from empty section to previous row drop position
this means we dont have to export keyboardDelegate from useTable/useGrid and instead we can have keyboard DnD keyboard navigation centralized in useDroppableCollection by providing itemFilter to the keyAbove/Below getters in Listlayout which ListView and TableView use. Also fixes various bugs I discovered such as focusing a non-valid drop target if the last focused key in the droppable collection was a column header and other bugs with pageup/down during keyboard DnD
@rspbot
Copy link

rspbot commented Apr 25, 2023

@rspbot
Copy link

rspbot commented Apr 26, 2023

LFDanLu added 2 commits April 26, 2023 17:06
this lead to weird behavior where you actually created new sections. Only dropping ON the section should be allowed, the other drop positions should come from after the preceeding row or before the following row in the section
@rspbot
Copy link

rspbot commented Apr 27, 2023

@rspbot
Copy link

rspbot commented Apr 28, 2023

@rspbot
Copy link

rspbot commented May 2, 2023

@rspbot
Copy link

rspbot commented May 2, 2023

Comment on lines +27 to +28
/** A delegate object that implements behavior for keyboard focus movement. */
keyboardDelegate?: KeyboardDelegate<unknown>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update to useDropIndicator api, see discussion here for background.

Comment on lines 70 to 72
// TODO: I've changed it so it will announce "insert after"/"insert before" target if the drop position is between the target and a following/preceeding section
// instead of announcing "insert between target and NEXT/PREV_SECTION_ROW". Gather opinions
let keyBefore = keyboardDelegate != null ? keyboardDelegate.getKeyAbove(target.key) : collection.getKeyBefore(target.key);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

open to opinions here

Copy link
Member Author

@LFDanLu LFDanLu May 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update so it announces the section name as well, make it announce as "before item A in section BLAH"

Comment on lines +46 to +51
// VoiceOver on MacOS doesn't announce TableView/ListView sections when navigating with arrow keys so we do this ourselves
// TODO: NVDA announces the section title when navigating into it with arrow keys as "SECTION TITLE grouping" by defualt, so removing isMac() doubles up on the section title announcement
// a bit. However, this does add an announcement for the number of rows in a section which might be
// Mobile screen readers don't cause this announcement to fire until focus happens on a row via double tap which is pretty strange
useUpdateEffect(() => {
if (isMac() && focusedItem != null && selectionManager.isFocused && sectionKey !== lastSection.current) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Open to opinions here if people feel like we wanna make this custom announcement in other non-Mac screenreaders

Comment on lines +50 to +52
// TODO: Chrome VO states "empty row group", doesn't happen in Safari VO. Doesn't happen in NVDA or TalkBack
role: 'rowgroup',
'aria-labelledby': headerId
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As stated here, Chrome VO doesn't have the best experience when navigating with arrows keys. Using control + option + arrow keys doesn't run into this problem and may be the preferred way that VO users will navigate through the table in general though. Will require some more exploration with different aria configurations but IMO this is ok for a first iteration (e.g. trying aria-posinset, aria-level, and aria-setsize instead of aria-rowindex resulted in a subpar experience)


// Override TS for TableSection to require title prop.
const SpectrumTableSection = TableSection as <T>(props: SpectrumTableSectionProps<T>) => JSX.Element;
export {SpectrumTableSection as TableSection};
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note we are exporting a TableSection and not reusing the Section collection. This feels like it makes more sense since all of Table's collection components are table specific anyways

}

getKeyAbove(key: Key): Key | null {
getKeyAbove(key: Key, itemFilter: (item: Node<T>) => boolean = item => item.type === 'item'): Key | null {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note the API change here to the getKey methods for ListLayout. This is so we can include sections as valid return keys from these methods ONLY during drag and drop. I'm a bit torn if modifying ListLayout is the best place to do this, but it does allow ListView's DnD to use the same code this way since they both provide a layout to useDroppableCollection.

Alternatives:

  • create a Table DnD specific keyboard delegate
    • Where would this live? Is it too specific? Opted not to do this for these reasons
  • Modifying the Table/GridKeyboardDelegate getKey getters instead
    • Would need the delegate to be returned by useTable, changes felt odd. Code for this can be found before this commit aka in 96f4921

// Skip persisted rows that overlap with visible cells.
while (persistedRowIndices && persistIndex < persistedRowIndices.length && persistedRowIndices[persistIndex] < i) {
} else {
// Note:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A variety of notes here to remind myself of the format of various pieces of data, happy to remove if people feel like it isn't helpful

Comment on lines +484 to +487
// TODO: discuss how best to indicate that a row/item is not selectable without making it disabled
// Some general item prop (isInert, notSelectable)? isSectionHeader might be too specific.
// Maybe not an item prop? Maybe I shouldn't be making the section header an actual row/item in the collection
if (!item || (item.type === 'cell' && !this.allowsCellSelection) || item.props?.isSectionHeader) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Open question, maybe it would be useful to have a generic prop that marks a node as non-selectable? Perhaps a non-factor when moving to RAC collections


export interface KeyboardDelegate {
export interface KeyboardDelegate<T> {
// TODO: what do we think about this update to the keyboard delegate api?
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned previously, changes in this PR include a update to the options provided to KeyboardDelegate's getKey getters

Copy link
Member Author

@LFDanLu LFDanLu May 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try doing T extends object instead to make this a non-breaking change for someone who made their own keyboard delegate

EDIT: Maybe I could make it unknown instead? Or change it so it is typeFilter: (type) => boolean? Is it actually a breaking change? Wouldn't someone implementing a KeyboardDelegate need a generic for their collection typing?

@LFDanLu LFDanLu changed the title (WIP) Sections in table Sections in table May 2, 2023
@rspbot
Copy link

rspbot commented May 2, 2023

export {
TableHeader,
TableBody,
Section,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keep this in case someone was using it

LFDanLu added 2 commits May 3, 2023 17:59
as per discussion with team, would be nice to have some context for insertion indicators immediatly before or after a section. Also addressed some review comments
@rspbot
Copy link

rspbot commented May 4, 2023

@rspbot
Copy link

rspbot commented May 4, 2023

## API Changes

unknown top level export { type: 'identifier', name: 'Column' }
unknown top level export { type: 'identifier', name: 'TableSection' }
unknown top level export { type: 'identifier', name: 'Column' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
undefined already in set
undefined already in set
undefined already in set
undefined already in set
GridRowProps already in set

@react-aria/autocomplete

AriaSearchAutocompleteOptions

 AriaSearchAutocompleteOptions<T> {
   inputRef: RefObject<HTMLInputElement>
-  keyboardDelegate?: KeyboardDelegate
+  keyboardDelegate?: KeyboardDelegate<T>
   listBoxRef: RefObject<HTMLElement>
   popoverRef: RefObject<HTMLDivElement>
 }

it changed:

  • useSearchAutocomplete

@react-aria/combobox

AriaComboBoxOptions

 AriaComboBoxOptions<T> {
   buttonRef?: RefObject<Element>
   inputRef: RefObject<HTMLInputElement>
-  keyboardDelegate?: KeyboardDelegate
+  keyboardDelegate?: KeyboardDelegate<T>
   listBoxRef: RefObject<HTMLElement>
   popoverRef: RefObject<Element>
 }

it changed:

  • useComboBox

@react-aria/dnd

DroppableCollectionOptions

 DroppableCollectionOptions {
   dropTargetDelegate: DropTargetDelegate
-  keyboardDelegate: KeyboardDelegate
+  keyboardDelegate: KeyboardDelegate<unknown>
 }

it changed:

  • useDroppableCollection

DropIndicatorProps

 DropIndicatorProps {
+  keyboardDelegate?: KeyboardDelegate<unknown>
   target: DropTarget
 }

it changed:

  • useDropIndicator

@react-aria/grid

useGrid

changed by:

  • GridProps
 useGrid<T> {
-  props: GridProps
+  props: GridProps<T>
   state: GridState<T, GridCollection<T>>
   ref: RefObject<HTMLElement>
   returnVal: undefined
 }

GridProps

-GridProps {
-  focusMode?: 'row' | 'cell' = 'row'
-  getRowText?: (Key) => string = (key) => state.collection.getItem(key)?.textValue
-  isVirtualized?: boolean
-  keyboardDelegate?: KeyboardDelegate
-  onCellAction?: (Key) => void
-  onRowAction?: (Key) => void
-  scrollRef?: RefObject<HTMLElement>
-}
+

it changed:

  • useGrid

GridRowProps

 GridRowProps<T> {
   isVirtualized?: boolean
-  node: Node<T>
+  node: GridNode<T>
   shouldSelectOnPressUp?: boolean
 }

it changed:

  • useGridRow

useGridSectionAnnouncement

-
+useGridSectionAnnouncement<T> {
+  state: GridSelectionState<T>
+  returnVal: undefined
+}

undefined

-
+GridProps<T> {
+  focusMode?: 'row' | 'cell' = 'row'
+  getRowText?: (Key) => string = (key) => state.collection.getItem(key)?.textValue
+  isVirtualized?: boolean
+  keyboardDelegate?: KeyboardDelegate<T>
+  onCellAction?: (Key) => void
+  onRowAction?: (Key) => void
+  scrollRef?: RefObject<HTMLElement>
+}

@react-aria/gridlist

AriaGridListOptions

 AriaGridListOptions<T> {
   disabledBehavior?: DisabledBehavior
   isVirtualized?: boolean
-  keyboardDelegate?: KeyboardDelegate
+  keyboardDelegate?: KeyboardDelegate<T>
   onAction?: (Key) => void
 }

it changed:

  • useGridList

@react-aria/listbox

AriaListBoxProps

 AriaListBoxOptions<T> {
   isVirtualized?: boolean
-  keyboardDelegate?: KeyboardDelegate
+  keyboardDelegate?: KeyboardDelegate<T>
   shouldFocusOnHover?: boolean
   shouldSelectOnPressUp?: boolean
   shouldUseVirtualFocus?: boolean
 }

@react-aria/menu

AriaMenuOptions

 AriaMenuOptions<T> {
   isVirtualized?: boolean
-  keyboardDelegate?: KeyboardDelegate
+  keyboardDelegate?: KeyboardDelegate<T>
 }

it changed:

  • useMenu

@react-aria/select

AriaSelectOptions

 AriaSelectOptions<T> {
-  keyboardDelegate?: KeyboardDelegate
+  keyboardDelegate?: KeyboardDelegate<T>
 }

it changed:

  • useSelect

@react-aria/selection

useSelectableCollection

changed by:

  • AriaSelectableCollectionOptions
-useSelectableCollection {
-  options: AriaSelectableCollectionOptions
-  returnVal: undefined
-}
+

useSelectableList

changed by:

  • AriaSelectableListOptions
-useSelectableList {
-  props: AriaSelectableListOptions
-  returnVal: undefined
-}
+

AriaSelectableCollectionOptions

-AriaSelectableCollectionOptions {
-  allowsTabNavigation?: boolean
-  autoFocus?: boolean | FocusStrategy = false
-  disallowEmptySelection?: boolean = false
-  disallowSelectAll?: boolean = false
-  disallowTypeAhead?: boolean = false
-  isVirtualized?: boolean
-  keyboardDelegate: KeyboardDelegate
-  ref: RefObject<HTMLElement>
-  scrollRef?: RefObject<HTMLElement>
-  selectOnFocus?: boolean = false
-  selectionManager: MultipleSelectionManager
-  shouldFocusWrap?: boolean = false
-  shouldUseVirtualFocus?: boolean
-}
+

it changed:

  • useSelectableCollection

AriaSelectableListOptions

-AriaSelectableListOptions {
-  allowsTabNavigation?: boolean
-  autoFocus?: boolean | FocusStrategy = false
-  collection: Collection<Node<unknown>>
-  disabledKeys: Set<Key>
-  disallowEmptySelection?: boolean = false
-  disallowTypeAhead?: boolean = false
-  isVirtualized?: boolean
-  keyboardDelegate?: KeyboardDelegate
-  ref?: RefObject<HTMLElement>
-  selectOnFocus?: boolean = false
-  selectionManager: MultipleSelectionManager
-  shouldFocusWrap?: boolean = false
-  shouldUseVirtualFocus?: boolean
-}
+

it changed:

  • useSelectableList

AriaTypeSelectOptions

 AriaTypeSelectOptions {
-  keyboardDelegate: KeyboardDelegate
+  keyboardDelegate: KeyboardDelegate<unknown>
   onTypeSelect?: (Key) => void
   selectionManager: MultipleSelectionManager
 }

it changed:

  • useTypeSelect

undefined

-
+useSelectableCollection<T> {
+  options: AriaSelectableCollectionOptions<T>
+  returnVal: undefined
+}

undefined

-
+useSelectableList<T> {
+  props: AriaSelectableListOptions<T>
+  returnVal: undefined
+}

undefined

-
+AriaSelectableCollectionOptions<T> {
+  allowsTabNavigation?: boolean
+  autoFocus?: boolean | FocusStrategy = false
+  disallowEmptySelection?: boolean = false
+  disallowSelectAll?: boolean = false
+  disallowTypeAhead?: boolean = false
+  isVirtualized?: boolean
+  keyboardDelegate: KeyboardDelegate<T>
+  ref: RefObject<HTMLElement>
+  scrollRef?: RefObject<HTMLElement>
+  selectOnFocus?: boolean = false
+  selectionManager: MultipleSelectionManager
+  shouldFocusWrap?: boolean = false
+  shouldUseVirtualFocus?: boolean
+}

undefined

-
+AriaSelectableListOptions<T> {
+  allowsTabNavigation?: boolean
+  autoFocus?: boolean | FocusStrategy = false
+  collection: Collection<Node<unknown>>
+  disabledKeys: Set<Key>
+  disallowEmptySelection?: boolean = false
+  disallowTypeAhead?: boolean = false
+  isVirtualized?: boolean
+  keyboardDelegate?: KeyboardDelegate<T>
+  ref?: RefObject<HTMLElement>
+  selectOnFocus?: boolean = false
+  selectionManager: MultipleSelectionManager
+  shouldFocusWrap?: boolean = false
+  shouldUseVirtualFocus?: boolean
+}

@react-aria/table

GridRowProps

 GridRowProps<T> {
   isVirtualized?: boolean
-  node: Node<T>
+  node: GridNode<T>
   shouldSelectOnPressUp?: boolean
 }

it changed:

  • useGridRow

useTableSection

changed by:

  • AriaTableSectionProps
  • TableSectionAria
-
+useTableSection<T> {
+  props: AriaTableSectionProps<T>
+  state: TableState<T>
+  returnVal: undefined
+}

AriaTableSectionProps

-
+AriaTableSectionProps<T> {
+  isVirtualized?: boolean
+  node: GridNode<T>
+}

it changed:

  • useTableSection

TableSectionAria

-
+TableSectionAria {
+  gridCellProps: DOMAttributes
+  rowGroupProps: DOMAttributes
+  rowProps: DOMAttributes
+}

it changed:

  • useTableSection

@react-stately/layout

ListLayout

 ListLayout<T> {
   allowDisabledKeyFocus: boolean
   buildChild: (Node<T>, number, number) => LayoutNode
   buildCollection: () => Array<LayoutNode>
   buildItem: (Node<T>, number, number) => LayoutNode
   buildNode: (Node<T>, number, number) => LayoutNode
   buildSection: (Node<T>, number, number) => LayoutNode
   collection: Collection<Node<T>>
   constructor: (ListLayoutOptions<T>) => void
   disabledKeys: Set<Key>
   getContentSize: () => void
   getDropTargetFromPoint: (number, number, (DropTarget) => boolean) => DropTarget
   getFinalLayoutInfo: (LayoutInfo) => void
-  getFirstKey: () => Key | null
+  getFirstKey: (any, any, (Node<T>) => boolean) => Key | null
   getInitialLayoutInfo: (LayoutInfo) => void
-  getKeyAbove: (Key) => Key | null
-  getKeyBelow: (Key) => Key | null
+  getKeyAbove: (Key, (Node<T>) => boolean) => Key | null
+  getKeyBelow: (Key, (Node<T>) => boolean) => Key | null
   getKeyForSearch: (string, Key) => Key | null
-  getKeyPageAbove: (Key) => Key | null
-  getKeyPageBelow: (Key) => Key | null
-  getLastKey: () => Key | null
+  getKeyPageAbove: (Key, (Node<T>) => boolean) => Key | null
+  getKeyPageBelow: (Key, (Node<T>) => boolean) => Key | null
+  getLastKey: (any, any, (Node<T>) => boolean) => Key | null
   getLayoutInfo: (Key) => void
   getVisibleLayoutInfos: (Rect) => void
   isLoading: boolean
   isValid: (Node<T>, number) => void
   updateItemSize: (Key, Size) => void
   updateLayoutNode: (Key, LayoutInfo, LayoutInfo) => void
   validate: (InvalidationContext<Node<T>, unknown>) => void
 }
 

TableLayout

 TableLayout<T> {
   addVisibleLayoutInfos: (Array<LayoutInfo>, LayoutNode, Rect) => void
   binarySearch: (Array<LayoutNode>, Point, 'x' | 'y') => void
   buildBody: (number) => LayoutNode
   buildCell: (GridNode<T>, number, number) => LayoutNode
   buildCollection: () => Array<LayoutNode>
   buildColumn: (GridNode<T>, number, number) => LayoutNode
   buildHeader: () => LayoutNode
   buildHeaderRow: (GridNode<T>, number, number) => LayoutNode
   buildNode: (GridNode<T>, number, number) => LayoutNode
   buildPersistedIndices: () => void
   buildRow: (GridNode<T>, number, number) => LayoutNode
+  buildSection: (Node<T>, number, number) => LayoutNode
   collection: TableCollection<T>
   columnLayout: TableColumnLayout<T>
   columnWidths: Map<Key, number>
   constructor: (TableLayoutOptions<T>) => void
   endResize: () => void
   getColumnMaxWidth: (Key) => number
   getColumnMinWidth: (Key) => number
   getColumnWidth: (Key) => number
   getDropTargetFromPoint: (number, number, (DropTarget) => boolean) => DropTarget
   getEstimatedHeight: (GridNode<T>, number, number, number) => void
   getInitialLayoutInfo: (LayoutInfo) => void
   getRenderedColumnWidth: (GridNode<T>) => void
   getResizerPosition: () => Key
   getVisibleLayoutInfos: (Rect) => void
   isLoading: any
   lastCollection: TableCollection<T>
   lastPersistedKeys: Set<Key>
   persistedIndices: Map<Key, Array<number>>
   resizingColumn: Key | null
   setChildHeights: (Array<LayoutNode>, number) => void
   startResize: (Key) => void
   stickyColumnIndices: Array<number>
   uncontrolledColumns: Map<Key, GridNode<unknown>>
   uncontrolledWidths: Map<Key, ColumnSize>
   updateResizedColumns: (Key, number) => Map<Key, ColumnSize>
   wasLoading: any
 }
 

@react-stately/table

TableSection

-
+TableSection<T> {
+  props: TableSectionProps<T>
+  returnVal: undefined
+}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants