feat: #10121 Implemented the Table Column Header Focus

[jsmitrah]

Closes #10121

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

• Pass the props as initialFocus="columnheader" to the table.
• Press the Tab button to focus the table column header.

🧢 Your Project:

[snowystinger]

Thanks, this looks pretty good. Will bring it to the team and see how we feel about the prop/name etc

[jsmitrah]

Thanks @snowystinger, I'll wait for the team feedback

[LFDanLu]

minor point, but do we really want to make this v3 change or is it enough have it available in RAC for now. We could consider exposing it in S2 as well, but only if there is an ask for it

[snowystinger]

I'd be ok with restricting the scope and having it only in RAC for now

[LFDanLu]

I noticed that arrow down doesn't seem to work when you are focused on the columns now, it should move focus to the rows below

[jsmitrah]

@LFDanLu , now the arrow down is focusing on the columns.

[LFDanLu]

@jsmitrah thanks for the fix, tested locally and it seems to work well now. Mind removing the prop addition in packages/@adobe/react-spectrum/src/table/TableView.tsx and then moving those tests to RAC if needed?

[jsmitrah]

@LFDanLu Thanks for the feedback. I have removed the prop and moved those tests into RAC.

[snowystinger]

some minor cleanup

[jsmitrah]

@snowystinger Cleaned up the code and made the suggested changes

[LFDanLu]

@snowystinger brought up a good point that this may affect areas like

react-spectrum/packages/react-aria/src/dnd/useDroppableCollection.ts

Lines 661 to 671 in 14b74f3

	case 'PageDown': {
	if (keyboardDelegate.getKeyPageBelow) {
	let target = localState.state.target;
	if (!target) {
	target = nextValidTarget(null, types, drag.allowedDropOperations, getNextTarget);
	} else {
	// If on the root, go to the item a page below the top. Otherwise a page below the current item.
	let targetKey = keyboardDelegate.getFirstKey?.();
	if (target.type === 'item') {
	targetKey = target.key;
	}

and

react-spectrum/packages/react-aria/src/dnd/useDroppableCollection.ts

Lines 739 to 748 in 14b74f3

	// If at the top already, switch to the root. Otherwise navigate a page up.
	if (target.key === keyboardDelegate.getFirstKey?.()) {
	target = {
	type: 'root'
	};
	} else {
	let nextKey: Key | null | undefined = keyboardDelegate.getKeyPageAbove(target.key);
	let dropPosition = target.dropPosition;
	if (nextKey == null) {
	nextKey = keyboardDelegate.getFirstKey?.();

IMO either those call sites need to be updated accordingly to look for the first row key, or we should change this column case to specifically only trigger for normal keyboard navigation some how

[snowystinger]

We should have tests for these as well, or point them out if they already exist, I'm a little surprised none broke but we may not have good coverage on this

[jsmitrah]

@LFDanLu and @snowystinger Thanks for pointing out the useDroppableCollection issue. Returning the column header from getFirstKey() was affecting DnD keyboard navigation. I added a getFirstItemKey helper to skip non-item elements and find the first row. I also updated the DnD call sites to use it. DnD keyboard navigation now works correctly, and all DnD and Table tests are passing.

[snowystinger]

This is starting to worry me that we're making a breaking change

[jsmitrah]

Thanks for the feedback. I looked into both points a bit more

I agree that we should avoid a breaking change! The reason I added getFirstItemKey is to make the DnD code use an actual item key. DnD expects drop target to be either item or root, so the column header returned by getFirstKey() shouldn't be passed to it, and this helper function will skip non-item nodes and find the first valid item instead.

So this supports columnheader as the initial focus for Tables without changing the shared APIs or passing an invalid key into the DnD logic.

[snowystinger]

Maybe instead of it being global, it should be an optional argument to getFirstKey? I have no idea if that'll be better

[jsmitrah]

Keeping it on TableKeyboardDelegate is the safer option, and the useSelectableCollection is shared by Lists, Grids, and Trees, so it doesn’t know about initialFocus. Passing it through getFirstKey() would require changes to the shared KeyboardDelegate API and the generic hook. Keeping it as an option on the Table delegate keeps this change limited to Tables.