[DataGrid] Spread forwardedProps onto grid element

[samwato]

Closes #13522

Looking at the historical changes, the aria-* and data-* props were originally on the GridRoot component when it had the role of "grid". This PR moves the rootProps.forwardedProps back to that element which has moved into the GridMainContainer component.

Demo: https://codesandbox.io/p/sandbox/github/samwato/codesandbox/tree/main/mui-x-issue-13522

• I have followed (at least) the PR section of the contributing guide.

[mui-bot]

Deploy preview: https://deploy-preview-14197--material-ui-x.netlify.app/

Generated by 🚫 dangerJS against 0f8d876

[samwato]

@michelengelen Any reviews on this would be great.

[romgrk]

Not sure what to think of this one. The change makes sense, but I think this could also be considered a breaking change at this point.

@mui/xgrid Thoughts?

[arminmeh]

Update: strikeout because of #8525
I think that it is more useful to move the generated aria attributes up and keep the ability for the users to add props to the root container.
In both cases the change should be treated as a breaking change since people may rely on the position of these attributes for styling, event listeners, tests, etc.
The change should be introduced behind an experimental flag (same as here)
Another example

[romgrk]

I've read the issue again and I think this could also classify as a bug. It's a breaking behavior that wasn't intended nor documented in the changelog, and it causes a11y issues. I think I'd be fine with merging the PR.

[arminmeh]

I think that the fact that even in this PR a test had to be updated to target the right element shows that we should treat it as a breaking change.

To update on my previous comment

I think that it is more useful to move the generated aria attributes up and keep the ability for the users to add props to the root container.

Actually, the only way to deal with this is like in this PR (moving forwarded props down), because role cannot be moved up (#8525)

[romgrk]

I guess it would also be possible to introduce a new prop that targets the [role=grid] element.

By the way the prop is marked as undocumented, we should remove that @ignore if it's already being used by external users:

mui-x/packages/x-data-grid/src/models/props/DataGridProps.ts

Lines 389 to 392 in c0463fd

	/**
	* Forwarded props for the Data Grid root element.
	* @ignore - do not document.
	*/

[samwato]

For context, I'm still on v6 as the migration to v7 breaks tests duo to the forwardedProps change and doesn't comply with our screen reader testing.

It is unfortunate that the changes in this PR are a breaking change on top of the v6 to v7 breaking change, but sounds like the right direction to go.

[MBilalShafi]

I guess it would also be possible to introduce a new prop that targets the [role=grid] element.

Agreed, to avoid the breaking change, we could add a temporary separate prop like forwardedPropsV8 for v7 that targets the [role=grid] element.
In v8, we could remove the prop and spread the forwarded props to the [role=grid] element by default.

[romgrk]

But it could also be a different prop like forwardedPropsGrid. Is there a use-case for both?

[MBilalShafi]

Is there a use-case for both?

There might be, I haven't seen it yet though. For simplicity, maybe start with only the grid element and aim to reintroduce it on the root element if there's a clear need. Wdyt?

[romgrk]

I'm fine with that.

[MBilalShafi]

Thank you @samwato for the effort on this PR. Would you mind updating the PR as discussed in the comment above?
I can also take over the PR from here if you want. 🙏

[samwato]

Thank you @samwato for the effort on this PR. Would you mind updating the PR as discussed in the comment above? I can also take over the PR from here if you want. 🙏

@MBilalShafi No problem, If you could take over the changes, that would be great. We can close this PR and start again if that's easier.

[romgrk]

I would favor adding forwardedPropsGrid instead of forwardedPropsV8, even if we're going to rename/remove them later.

[oliviertassinari]

Break API convention / describeConformance(), extra props should land on the root element, it's not the case https://deploy-preview-14197--material-ui-x.netlify.app/x/react-data-grid/

[MBilalShafi]

Break API convention / describeConformance(), extra props should land on the root element, it's not the case deploy-preview-14197--material-ui-x.netlify.app/x/react-data-grid

@oliviertassinari

As I understand WAI-ARIA Authoring Practices, extra attributes like aria-label, aria-describedby, etc. should be applied to the element with role="grid" for the screen readers to properly associate these attributes with the grid functionality.

I understand that we have an API convention to spread the extra props on to the DOM root element, this case seems to be an exception though as we can't move role attribute up.

What do you think about considering the logical root element (.MuiDataGrid-main/role="grid") as root rather than the actual DOM root (.MuiDataGrid-root) and spread the props on it?

An additional prop forwardedPropsRoot could be used alongside to support forwarding props on actual DOM root (.MuiDataGrid-root)

[oliviertassinari]

@MBilalShafi I think this is where the slotProps prop steps in. Feeling in control as a developer feels more important than ease of use. It also makes the case to make it possible to decompose the data grid.

[cherniavskii]

I think this is where the slotProps prop steps in.

I agree. This is the least confusing option.
Now, the naming:

1. slotProps={{ main: {} }} – following the main CSS class we're already using
2. slotProps={{ grid: {} }} – following the aria role.
3. ...Other?

I'm in favor of option 1.
@MBilalShafi What do you think?

[MBilalShafi]

1. slotProps={{ main: {} }} – following the main CSS class we're already using
2. slotProps={{ grid: {} }} – following the aria role.

@cherniavskii
Sounds good. However, I wonder if it would keep the same confusion (#13522) related to the screen readers. Would the users passing additional props directly to the <DataGrid /> instead of using slotProps expect them to be spread to the role="grid" element rather than .root?

Another option could be to follow a similar route as #10409 (comment), i.e. stop forwarding additional props forwarded on the <DataGrid /> component, and introduce slotProps for both the elements. i.e slotProps={{ root: {}, main/grid: {} }}

Summing up, we have the following options to choose from.

1. Spread the additional props to the root element (.MuiDataGrid-root) and introduce a slot prop (slotProps={{ main/grid: {} }}) to allow passing additional props to the role="grid" element.
2. Reverse of 1. Spread the additional props to the grid element (.MuiDataGrid-main/role="grid") and introduce a slot prop (slotProps={{ root: {} }}) to allow passing additional props to the .root element.
3. Don't forward additional props passed to the <DataGrid /> component at all, allow to do it via the slotProps only. (slotProps={{ root: {}, main/grid: {} }})

If I get it right, your suggestion is option 1, I proposed option 2, it seems option 3 could be the least confusing middle-ground. Wdyt?

[cherniavskii]

Don't forward additional props passed to the component at all

This only applies to data- and aria- attributes, but not className and sx, right?

[MBilalShafi]

This only applies to data- and aria- attributes, but not className and sx, right?

Yes, only the forwarded props prefixed with aria- and data-.

[cherniavskii]

Then option 3 sounds good to me 👍🏻

[samwato]

The simplicity of just providing the 'aria-' prop to the component and it just works with screen readers is nice. But option 3 allows flexibility to enable any use case so sounds good 👍

[oliviertassinari]

Now, the naming:
1.slotProps={{ main: {} }} – following the main CSS class we're already using
2.slotProps={{ grid: {} }} – following the aria role.
3 ...Other?

For sure 1. no question, we have to keep the pattern consistent (class name slot name) for people to learn things once. If the aria role is important, then we can argue that main should be renamed to grid, but it's a different, for later, change.

3. Don't forward additional props passed to the component at all, allow to do it via the slotProps only. (slotProps={{ root: {}, main/grid: {} }})

If we mean we can get rid of all of this:

mui-x/packages/x-data-grid/src/internals/utils/useProps.ts

Line 19 in 99ce15c

for (let i = 0; i < keys.length; i += 1) {

it sounds like a performance win 👍. I guess it's also performance as to why we don't spread other props to the root. There are so many props supported, that it would add bundle-size bloat.

With compound components, we could improve the DX later on, while still making the current one-component API behavior easy to predict.

[MBilalShafi]

Raised a follow-up PR based on the discussion above: #15870

When that is merged, we could close this one safely. Thank you everyone for your input 🙇