Skip to content

fix: Fix Table disabled state + Safari loadMore and scroll Combobox selected item into view when opening via click #8224

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
May 15, 2025

Conversation

LFDanLu
Copy link
Member

@LFDanLu LFDanLu commented May 12, 2025

caught by chromatic

✅ 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:

🧢 Your Project:

RSP

@rspbot
Copy link

rspbot commented May 12, 2025

snowystinger
snowystinger previously approved these changes May 12, 2025
…fari

position absolute/relative is not really well supported in Safari making the loading sentinel not scroll into view when scrolling the table
@LFDanLu
Copy link
Member Author

LFDanLu commented May 13, 2025

https://www.chromatic.com/build?appId=5f0dd5ad2b5fc10022a2e320&number=948, can ignore the one failure, that is known and unrelated to this PR

@rspbot
Copy link

rspbot commented May 13, 2025

@LFDanLu LFDanLu changed the title fix: Fix select all checkbox disabled state and other empty state issues when loading sentinel is provided fix: Fix select all checkbox disabled state when loading sentinel is provided and Safari loadMore when non-virtualized May 13, 2025
@LFDanLu LFDanLu changed the title fix: Fix select all checkbox disabled state when loading sentinel is provided and Safari loadMore when non-virtualized fix: Fix Table disabled state + Safari loadMore and scroll Combobox selected item into view when opening via click May 13, 2025
Copy link
Member Author

@LFDanLu LFDanLu May 13, 2025

Choose a reason for hiding this comment

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

For now I'm skipping these tests until we do more testing and confirm if we want to focus + scroll the ComboBox selected item into view when opening the drop down via click. These tests originate from #1014 but I don't remember/can't find any reason why we don't want this behavior other than a214902. The NVDA problem shouldn't apply since we still clear focus when the input field is modified, but will test

Copy link
Member Author

Choose a reason for hiding this comment

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

One potential weird point may be if you have a selected key and allowsCustomValue, where then if you backspace once the dropdown will open and an item will be focused, preventing a subsequent ENTER keypress from submitting your custom value

@rspbot
Copy link

rspbot commented May 13, 2025

devongovett
devongovett previously approved these changes May 14, 2025
@@ -64,7 +64,7 @@ export function useTableSelectAllCheckbox<T>(state: TableState<T>): TableSelectA
checkboxProps: {
'aria-label': stringFormatter.format(selectionMode === 'single' ? 'select' : 'selectAll'),
isSelected: isSelectAll,
isDisabled: selectionMode !== 'multiple' || state.collection.size === 0,
isDisabled: selectionMode !== 'multiple' || (state.collection.size === 0 || (state.collection.rows.length === 1 && state.collection.rows[0].type === 'loader')),
Copy link
Member

Choose a reason for hiding this comment

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

😐

@@ -1874,7 +1874,7 @@ describe('Table', () => {
expect(loaderRow).toHaveTextContent('spinner');

let sentinel = tree.getByTestId('loadMoreSentinel');
expect(sentinel.parentElement).toHaveAttribute('inert');
expect(sentinel.parentElement.parentElement).toHaveAttribute('inert');
Copy link
Member

Choose a reason for hiding this comment

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

seems brittle, can we just use sentinel.closest('[inert]') is truthy?

Copy link
Member Author

Choose a reason for hiding this comment

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

sure, was mainly concerned about that possibly matching a higher level inert element and still passing, but I'll see about limiting it to the body

@rspbot
Copy link

rspbot commented May 15, 2025

@rspbot
Copy link

rspbot commented May 15, 2025

@rspbot
Copy link

rspbot commented May 15, 2025

@rspbot
Copy link

rspbot commented May 15, 2025

## API Changes

react-aria-components

/react-aria-components:DropIndicator

 DropIndicator {
-  activateButtonRef?: RefObject<FocusableElement | null>
   children?: ReactNode | ((DropIndicatorRenderProps & {
     defaultChildren: ReactNode | undefined
 })) => ReactNode
   className?: string | ((DropIndicatorRenderProps & {
 })) => string
   style?: CSSProperties | ((DropIndicatorRenderProps & {
     defaultStyle: CSSProperties
 })) => CSSProperties | undefined
   target: DropTarget
 }

/react-aria-components:TreeItemRenderProps

 TreeItemRenderProps {
-  allowsDragging?: boolean
   hasChildItems: boolean
   id: Key
   isDisabled: boolean
-  isDragging?: boolean
-  isDropTarget?: boolean
   isExpanded: boolean
   isFocusVisible: boolean
   isFocusVisibleWithin: boolean
   isFocused: boolean
   isPressed: boolean
   isSelected: boolean
   level: number
   selectionBehavior: SelectionBehavior
   selectionMode: SelectionMode
   state: TreeState<unknown>
 }

/react-aria-components:TreeItemContentRenderProps

 TreeItemContentRenderProps {
-  allowsDragging?: boolean
   hasChildItems: boolean
   id: Key
   isDisabled: boolean
-  isDragging?: boolean
-  isDropTarget?: boolean
   isExpanded: boolean
   isFocusVisible: boolean
   isFocusVisibleWithin: boolean
   isFocused: boolean
   isPressed: boolean
   isSelected: boolean
   level: number
   selectionBehavior: SelectionBehavior
   selectionMode: SelectionMode
   state: TreeState<unknown>
 }

/react-aria-components:DragAndDropOptions

 DragAndDropOptions {
   acceptedDragTypes?: 'all' | Array<string | symbol> = 'all'
   dropTargetDelegate?: DropTargetDelegate
   getAllowedDropOperations?: () => Array<DropOperation>
   getDropOperation?: (DropTarget, DragTypes, Array<DropOperation>) => DropOperation
   getItems?: (Set<Key>) => Array<DragItem> = () => []
   isDisabled?: boolean
   onDragEnd?: (DraggableCollectionEndEvent) => void
   onDragMove?: (DraggableCollectionMoveEvent) => void
   onDragStart?: (DraggableCollectionStartEvent) => void
   onDrop?: (DroppableCollectionDropEvent) => void
-  onDropActivate?: (DroppableCollectionActivateEvent) => void
   onDropEnter?: (DroppableCollectionEnterEvent) => void
   onDropExit?: (DroppableCollectionExitEvent) => void
   onInsert?: (DroppableCollectionInsertDropEvent) => void
   onItemDrop?: (DroppableCollectionOnItemDropEvent) => void
-  onMove?: (DroppableCollectionReorderEvent) => void
   onReorder?: (DroppableCollectionReorderEvent) => void
   onRootDrop?: (DroppableCollectionRootDropEvent) => void
   renderDragPreview?: (Array<DragItem>) => JSX.Element
   renderDropIndicator?: (DropTarget) => JSX.Element
 }

/react-aria-components:DropIndicatorProps

 DropIndicatorProps {
-  activateButtonRef?: RefObject<FocusableElement | null>
   children?: ReactNode | ((DropIndicatorRenderProps & {
     defaultChildren: ReactNode | undefined
 })) => ReactNode
   className?: string | ((DropIndicatorRenderProps & {
 })) => string
   style?: CSSProperties | ((DropIndicatorRenderProps & {
     defaultStyle: CSSProperties
 })) => CSSProperties | undefined
   target: DropTarget
 }

@react-aria/dnd

/@react-aria/dnd:DroppableCollectionOptions

 DroppableCollectionOptions {
   acceptedDragTypes?: 'all' | Array<string | symbol> = 'all'
   dropTargetDelegate: DropTargetDelegate
   getDropOperation?: (DropTarget, DragTypes, Array<DropOperation>) => DropOperation
   keyboardDelegate: KeyboardDelegate
   onDrop?: (DroppableCollectionDropEvent) => void
-  onDropActivate?: (DroppableCollectionActivateEvent) => void
   onDropEnter?: (DroppableCollectionEnterEvent) => void
   onDropExit?: (DroppableCollectionExitEvent) => void
   onInsert?: (DroppableCollectionInsertDropEvent) => void
   onItemDrop?: (DroppableCollectionOnItemDropEvent) => void
-  onKeyDown?: (KeyboardEvent) => void
-  onMove?: (DroppableCollectionReorderEvent) => void
+  onKeyDown?: KeyboardEvents['onKeyDown']
   onReorder?: (DroppableCollectionReorderEvent) => void
   onRootDrop?: (DroppableCollectionRootDropEvent) => void
   shouldAcceptItemDrop?: (ItemDropTarget, DragTypes) => boolean
 }

/@react-aria/dnd:DroppableItemOptions

 DroppableItemOptions {
-  activateButtonRef?: RefObject<FocusableElement | null>
   target: DropTarget
 }

/@react-aria/dnd:DropIndicatorProps

 DropIndicatorProps {
-  activateButtonRef?: RefObject<FocusableElement | null>
   target: DropTarget
 }

@react-spectrum/dnd

/@react-spectrum/dnd:DragAndDropOptions

 DragAndDropOptions {
   acceptedDragTypes?: 'all' | Array<string | symbol> = 'all'
   getAllowedDropOperations?: () => Array<DropOperation>
   getDropOperation?: (DropTarget, DragTypes, Array<DropOperation>) => DropOperation
   getItems?: (Set<Key>) => Array<DragItem> = () => []
   onDragEnd?: (DraggableCollectionEndEvent) => void
   onDragMove?: (DraggableCollectionMoveEvent) => void
   onDragStart?: (DraggableCollectionStartEvent) => void
   onDrop?: (DroppableCollectionDropEvent) => void
-  onDropActivate?: (DroppableCollectionActivateEvent) => void
   onDropEnter?: (DroppableCollectionEnterEvent) => void
   onDropExit?: (DroppableCollectionExitEvent) => void
   onInsert?: (DroppableCollectionInsertDropEvent) => void
   onItemDrop?: (DroppableCollectionOnItemDropEvent) => void
-  onMove?: (DroppableCollectionReorderEvent) => void
   onReorder?: (DroppableCollectionReorderEvent) => void
   onRootDrop?: (DroppableCollectionRootDropEvent) => void
   renderPreview?: (Set<Key>, Key) => JSX.Element
   shouldAcceptItemDrop?: (ItemDropTarget, DragTypes) => boolean

@react-stately/dnd

/@react-stately/dnd:DroppableCollectionStateOptions

 DroppableCollectionStateOptions {
   acceptedDragTypes?: 'all' | Array<string | symbol> = 'all'
   collection: Collection<Node<unknown>>
   getDropOperation?: (DropTarget, DragTypes, Array<DropOperation>) => DropOperation
   isDisabled?: boolean
   onDrop?: (DroppableCollectionDropEvent) => void
   onDropEnter?: (DroppableCollectionEnterEvent) => void
   onDropExit?: (DroppableCollectionExitEvent) => void
   onInsert?: (DroppableCollectionInsertDropEvent) => void
   onItemDrop?: (DroppableCollectionOnItemDropEvent) => void
-  onMove?: (DroppableCollectionReorderEvent) => void
   onReorder?: (DroppableCollectionReorderEvent) => void
   onRootDrop?: (DroppableCollectionRootDropEvent) => void
   selectionManager: MultipleSelectionManager
   shouldAcceptItemDrop?: (ItemDropTarget, DragTypes) => boolean

@LFDanLu LFDanLu added this pull request to the merge queue May 15, 2025
Merged via the queue into main with commit 671f0b4 May 15, 2025
30 checks passed
@LFDanLu LFDanLu deleted the table_chromatic_fixes branch May 15, 2025 21:18
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.

5 participants