[DataGrid] Support data attributes

[romgrk]

Closes #8605

This PR adds support for custom data attributes on the grid root element, which is useful for automation, testing & some external libraries.

[mui-bot]

Netlify deploy preview

Netlify deploy preview: https://deploy-preview-8845--material-ui-x.netlify.app/

Updated pages

No updates.

These are the results for the performance tests:

Test case	Unit	Min	Max	Median	Mean	σ
Filter 100k rows	ms	654.8	1,018.1	705.4	828.02	135.743
Sort 100k rows	ms	613.8	1,373.8	1,373.8	961.54	293.94
Select 100k rows	ms	172.5	355.5	308.8	290.12	66.311
Deselect 100k rows	ms	198.4	312.8	231.1	248.44	39.445

Generated by 🚫 dangerJS against 2c7578f

[romgrk]

I've added a test for this in the community version, but technically speaking the same test should be added in the pro & premium packages to cover the feature fully. But that wouldn't be very DRY. Suggestions welcome.

[romgrk]

The changes here are going to conflict with #8942.

[romgrk]

This is ready for review. There is a CI failure, but it seems unrelated to this PR.

[m4theushw]

I'm not sure if we're in the right direction. The MUI components, by convention, spread the remaining props to the root element, we don't do that. If we go with the approach implemented here we still won't support passing an id value or using other attributes, e.g. hidden. I think we should enhance the DataGridProps interface to also allow any DIV attribute, then pass the attributes we don't use to the root element.

[romgrk]

IIUC, you're saying we should do something like this?

[m4theushw]

Yes, take all props, filter out those that are used internally (e.g. pagination), and spread to the root element the remaining ones (e.g. data-*). It would be nice if we didn't have to duplicate the list of props and leverage the DataGridProps interface but I don't know if it's possible.

[romgrk]

Note for myself:
https://github.com/kimamula/ts-transformer-keys
https://stackoverflow.com/a/59774983/6303229

[romgrk]

I've looked around and using the DataGridProps interface isn't super trivial, because we need to take into account that there is also DataGridProProps and DataGridPremiumProps, and all of these are composite parametric types built dynamically. So the ts-transformer-keys above won't work (parametric types). And I feel like defining an array of accepted keys for each of those types is going to be tedious and error-prone.

How about we define the list of HTML props (eg. id, hidden) & prop patterns (eg data-*) we accept, and restrict ourselves to forwarding those?

[romgrk]

Ping @m4theushw, any opinion on the comment above?

[m4theushw]

I'm not a big fan of creating a list of HTML attributes to forward. The MUI components have this convention of forwarding the rest and I think it would be nice if we supported it too. I'm more on the side of having a list of DataGrid attributes to not forward. If it's not possible to enforce via TS that every prop must be filtered out from the rest then I think we need to accept it and be cautious when adding new props. Maybe a ESLint could be built to check this but it might be overkill. Don't take my opinion as the only approach to follow, I'm open for the opinions from other members.

[romgrk]

I prefer the solution currently implemented in this PR because it has a good complexity/maintainability cost vs benefit. We have received requests to support data-attributes, from a pragmatic POV I think we should limit ourselves to supporting that in this PR. I'll wait to hear comments from the rest of the team before adding props arrays for all the DataGrid components.

[cherniavskii]

The trade-off looks good to me 👍

[MBilalShafi]

Looks good to me.

Should we use the groupDataProps middleware to also accommodate aria-* props? (Rename it to groupCustomProps or something like that.)

Currently we hard code aria-label and aria-labelledby but probably it's an opportunity to make it generic too, as more aria-* props like aria-description could be passed.

Example pain point: #9075

[romgrk]

Makes sense to deal with aria- here. I've updated the function to accept aria- props. I've left aria-label and aria-labelledby in because otherwise our linting/testing fails, but we do accept any aria prop now.

[MBilalShafi]

otherwise our linting/testing fails

Since we now support aria-* attributes by default, would doing something like this resolve the linting errors you mentioned (and possibly add a little bit better IDE auto-completion, for aria-* attributes)?

diff --git a/packages/grid/x-data-grid/src/models/props/DataGridProps.ts b/packages/grid/x-data-grid/src/models/props/DataGridProps.ts
index 006af94b0..2936f2233 100644
--- a/packages/grid/x-data-grid/src/models/props/DataGridProps.ts
+++ b/packages/grid/x-data-grid/src/models/props/DataGridProps.ts
@@ -116,7 +116,7 @@ export interface DataGridPropsWithComplexDefaultValueBeforeProcessing {
  * The controlled model do not have a default value at the prop processing level, so they must be defined in `DataGridOtherProps`
  * TODO: add multiSortKey
  */
-export interface DataGridPropsWithDefaultValues {
+export interface DataGridPropsWithDefaultValues extends React.AriaAttributes {
   /**
    * If `true`, the grid height is dynamic and follow the number of rows in the grid.
    * @default false
@@ -677,14 +677,6 @@ export interface DataGridPropsWithoutDefaultValue<R extends GridValidRowModel =
    * @param {GridCallbackDetails} details Additional details for this callback.
    */
   onSortModelChange?: (model: GridSortModel, details: GridCallbackDetails) => void;
-  /**
-   * The label of the grid.
-   */
-  'aria-label'?: string;
-  /**
-   * The id of the element containing a label for the grid.
-   */
-  'aria-labelledby'?: string;
   /**
    * Set of columns of type [[GridColDef[]]].
    */

[MBilalShafi]

Shouldn't this be in DataGridPropsWithoutDefaultValue?

[MBilalShafi]

Just a side note:

I am amazed this is not actually possible (context), but it'd have be great to only allow aria-* and data-* hyphen-cased props for better type coverage. (Lack of that is the very reason this issue came up to begin with, as the user passed data-* props, typescript did not complain, even though we didn't pass the props down to the dom element)

This probably supports @m4theushw's argument of destructuring { ...rest } as technically all hyphen-cased props are allowed by the TS but are not actually passed down to the DOM element. (possibly for those props which contain a hyphen propName.includes('-'), it will cover data-* and aria-* by default).

I am not sure though how practical is it to go in that direction right now.

[romgrk]

React.AriaAttributes

I considered using it but it didn't like the solution, because it contains props such as aria-checked which make absolutely no sense on the datagrid. I feel like it's fine to accept everything (pros: future-proof, completeness) but not document that we accept everything. I'm open to using it if there is a strong feeling that it's a better solution.'

Destructuring

I'm not sure I see how it would help with the issues here. I don't think we can get a perfect (typed) solution here.

[romgrk]

There is a failing test but it seems unrelated to this PR. If no one else has comments, I'll merge as it is before the release.