feat: Add colspan to Table

[oyvinmar]

Closes #5334, closes #5862

This change adds support for colSpan to the table. The changes can be reviewed commit by commit. The first commit adds colSpan support; the second adds aria-col-index, which makes keyboard navigation slightly simpler but adds some complexity to updating the cells with the correct column index.

Some implementation details/concerns/questions:

• I'm unsure if this is the best way to update the grid nodes with the attributes. If there's a better way, please let me know.
• aria-col-index is added to every cell, even in rows where colspan isn't used. I could probably look this up before adding it, but I'm worried that could be an expensive operation.
• I've not added any tests yet (or updated documentation). I would like to add some for the keyboard delegate but want to make sure I'm on the right path first.

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

I've provided Storybook examples, which I have used to test the changes. They should be findable by searching for «colspan».

🧢 Your Project:

We are evaluating react-aria-components for a new implementation of a table provided by our internal design system.

[snowystinger]

Thanks for the PR! Had a quick look, it looks like the right direction to go, I'll bring it up with the team later this week.

Some notes for the team:

Looks like VO support isn't great for colspans, announcement is only "x of n" where "n" is less than overall number of columns. It doesn't mention anything about how many columns it covers or what the title/owning column is. I tested it against the MDN docs as well, appears to be the same, so this may be expected. It behaves the same with the virtualized RSP table as well.

We'll need to do more testing with the other screen readers.

[nwidynski]

@oyvinmar I've recently worked a lot on the grid edit mode, so I'm hoping to be able to be of help with some of your questions.

I'm unsure if this is the best way to update the grid nodes with the attributes.

Usually @react-aria/collections access pattern for node props is via the collection. You can view this pattern being applied in the SelectionManager, but it would look something like state.collection.getItem(node.key)?.props.colSpan.

The value of this gets automatically populated on the first render, when the collection is built. That is also why you don't see most of the cell props inside of the GridCellProps or TableCellProps interface(s).

aria-col-index is added to every cell, even in rows where colspan isn't used. I could probably look this up before adding it, but I'm worried that could be an expensive operation.

Via the before-mentioned pattern you can trivially check for the existence of a specific prop, since only then it will exist in the prop object of the node item. This is also not a DOM operation so no worries on that front.

I would like to add some for the keyboard delegate but want to make sure I'm on the right path first.

Afaik, the colSpan attribute is only supported on td and th elements per W3C spec. From a quick check through the codebase, i can only find one reference to the column node type inside grid hooks, so it's likely useTableCell & TableKeyboardDelegate are better suited to accomodate this behavior.

EDIT: Saw that @snowystinger also provided feedback on VO support for this already, so I'm sure the core maintainers will get back to you shortly with more specific guidance. Thanks again for the PR 👍

[oyvinmar]

Thanks for the fast feedback! I've changed to setting the grid node attributes in the collections and only include aria-col-index when the row has a colSpan cell (and when virtualized).

[snowystinger]

We briefly discussed this today. Again, thank you so much for contributing this feature. It looks like this is the correct direction. Adding tests is a good next step while we review the code.

One thing we'd like to add in the logic is some validation:
num cols === sum of col spans
and throw if this isn't true

[danielbrodin]

Will this also add support for colspan on Column for cases where you want a subheading in the table with scope="colgroup"? Don't think the Column component has support for colgroup yet, but if it gets it colspan would be needed.

[snowystinger]

Will this also add support for colspan on Column for cases where you want a subheading in the table with scope="colgroup"? Don't think the Column component has support for colgroup yet, but if it gets it colspan would be needed.

column group support will have to come later, we don't have an API for it yet and I'd prefer not to add more to this pr

colspan is only added on Cell in this PR, so it's not on Column

[snowystinger]

In general this looks awesome, I only have a few comments. Thanks for adding tests and doing the React Spectrum implementation.

Question to team: Should we track column keys for repetitive arrow key up/down so that you stay in the same column until you intentionally change columns?
Go to /?path=/story/tableview--table-cell-col-span-with-various-spans-example, press Tab, ArrowRight, then ArrowDown x3, you'll have focus on the cell in Col1 instead of Col2.

[snowystinger]

can colspan match the capitalisation of the GridCellProps' colSpan?

[oyvinmar]

It should be a fairly straightforward to change, but I think it would introduce a breaking change in table header (useTableColumnHeader): https://react-spectrum.adobe.com/react-aria/useTable.html#table-header

[snowystinger]

Ah, then probably we should go the other direction to match.

[snowystinger]

Ok, spent a little longer looking at this, I think we can match the colSpan if we keep colspan on the node at the same time, and we can deprecate the old one. Then the hooks should return the appropriate values so that people don't need to do that step with hooks themselves

https://github.com/adobe/react-spectrum/blob/main/packages/%40react-types/grid/src/index.d.ts#L31

change to

  /** 
     * The number of columns spanned by this cell. Use `colSpan` instead.
     * @deprecated
     */
  colspan?: number,
  /** 
     * The number of columns spanned by this cell.
     */
  colSpan?: number,

I like having it be colSpan as well because that's the react attribute too.

[oyvinmar]

I have renamed it now and duplicated the assignments to ensure backward compatibility.

[henkkasoft]

It's really great that this has been implemented, thank you very much! We are looking forward to start using this as soon as possible.

[snowystinger]

awesome, thank you so much for the update, I'll hopefully get a second person on the team to review soon