[WIP] Tabs remove clone element

[ZeeshanTamboli]

[mui-bot]

Netlify deploy preview

https://deploy-preview-46333--material-ui.netlify.app/

Bundle size report

Bundle	Parsed Size	Gzip Size
@mui/material	🔺+500B(+0.10%)	🔺+226B(+0.15%)
@mui/lab	🔺+165B(+0.12%)	🔺+86B(+0.21%)
@mui/system	0B(0.00%)	0B(0.00%)
@mui/utils	0B(0.00%)	0B(0.00%)

Details of bundle changes

Generated by :no_entry_sign: dangerJS against 9a305833621ebcfb4ae9fe4f1bea1a4bf0ff5d4d

[ZeeshanTamboli]

@DiegoAndai

When we release this, we should do it under a minor version just to be cautious about changes that might be perceived as breaking.

Agreed 👍

Did you add test for all the issues this would close?

Not for every case individually, but I believe this test covers the behavior broadly across the scenarios. Let me know if not.

[DiegoAndai]

@ZeeshanTamboli I think we should add additional tests for:

• https://github.com/mui/material-ui/issues/27947
• https://github.com/mui/material-ui/issues/34740
• https://github.com/mui/material-ui/issues/38516

(The one we already have covers https://github.com/mui/material-ui/issues/46265)

[ZeeshanTamboli]

@ZeeshanTamboli I think this change (225cafd) needs to be reverted.

In previous implementation, when onChange is added to both Tabs and Tab, onChange of Tabs always overrides onChange of Tab, check here for implementation =>

https://github.com/mui/material-ui/blob/4ec315ebc9fa6672b473c3c82b245d683ea71114/packages/mui-material/src/Tabs/Tabs.js#L809

In this version, onChange of Tabs doesn't override onChange of Tab thus resulting in broken behaviour of Tabs when both Tabs and Tab has onChange handler.

Check this example built on top of this PR: https://stackblitz.com/edit/ry4fan5c?file=src%2FDemo.tsx, try to change tabs it doesn't work.

I'm aware this is extremely niche use case, not sure if it's really a valid use case, but just wanted to bring it your attention

👍 Reverted. I think there is no breaking change then cc @DiegoAndai

[DiegoAndai]

@ZeeshanTamboli I agree with @sai6855 that onChange behavior shouldn't change: when onChange is added to both Tabs and Tab, onChange of Tabs overrides the one on Tab. We should keep that, but we shouldn't remove the prop from Tab. Does this make sense?

[ZeeshanTamboli]

@ZeeshanTamboli I agree with @sai6855 that onChange behavior shouldn't change: when onChange is added to both Tabs and Tab, onChange of Tabs overrides the one on Tab. We should keep that, but we shouldn't remove the prop from Tab. Does this make sense?

@DiegoAndai I think we can safely remove the onChange prop from Tab.

In both the current version (master) and this PR:

• ✅ If onChange is provided only to <Tabs>, it’s called.
• ❌ If onChange is provided only to <Tab>, it’s not called. The undefined onChange from Tabs takes priority. And also <Tab> cannot be used as a standalone component without <Tabs>.
• ✅ If provided to both, the one from <Tabs> takes priority.

You can see this behavior is consistent in both versions:

• Master: https://stackblitz.com/edit/ry4fan5c-3goxgph6
• This PR: https://stackblitz.com/edit/ry4fan5c-r5tnuyfm

[ZeeshanTamboli]

@ZeeshanTamboli I think we should add additional tests for:

• [Tabs] Tabs doesn't work with Tab as a custom component #27947
• [Tabs] Conditionally rendering of Tab not working properly #34740
• [Divider][Tabs] Using divider with tab causes 4 console warnings in Chrome DevTools #38516

(The one we already have covers #46265)

@DiegoAndai Addressed in https://github.com/mui/material-ui/pull/46333/commits/f47c38ee9c23bfd617e9197f0a9d581a4dc11a9e

[ZeeshanTamboli]

We should think about this a bit more. I am struggle on what's the structure should be, ideally I want:

<Tabs>
  <TabList>
    <Tab>
    …
  </TabList>
  <TabPanel>…</TabPanel>
  <TabPanel>…</TabPanel>
  <TabPanel>…</TabPanel>
</Tabs>

@siriwatknp I think we can do this. We can use the context from Tabs by setting idPrefix in the Tabs context. Then, we can apply aria-controls and id on each Tab, and aria-labelledby and id on eachTabPanels.

Then we can remove the need of the TabContext component from Lab altogether and move TabList and TabPanel components to Material UI.

I think we can even remove TabList. Just have Tabs, Tab and TabPanel with full accessibility support.

[siriwatknp]

@siriwatknp I think we can do this. We can use the context from Tabs by setting idPrefix in the Tabs context. Then, we can apply aria-controls and id on each Tab, and aria-labelledby and id on eachTabPanels.

Then we can remove the need of the TabContext component from Lab altogether and move TabList and TabPanel components to Material UI.

I think we can even remove TabList. Just have Tabs, Tab and TabPanel with full accessibility support.

In terms of logic I agree, but for the layout, what's your plan to make it work without a breaking change?

The existing Tabs passes the children inside the scroller, so this would not work:

<Tabs>
  <Tab></Tab>
  <Tab></Tab>
  <Tab></Tab>
  <TabPanel></TabPanel>
  <TabPanel></TabPanel>
  <TabPanel></TabPanel>
</Tabs>

[ZeeshanTamboli]

I think we need to be clear on https://mui.com/material-ui/react-tabs/#experimental-api.

In my opinion, we should communicate that user can move away from the lab to this new context API.

We should think about this a bit more. I am struggle on what's the structure should be, ideally I want:

<Tabs>
  <TabList>
    <Tab>
    …
  </TabList>
  <TabPanel>…</TabPanel>
  <TabPanel>…</TabPanel>
  <TabPanel>…</TabPanel>
</Tabs>

but not sure if this is possible given the existing Tabs we have.

Probably the best option is to use new prop to change the layout:

<Tabs
  panels={
    <div>
      <TabPanel>…</TabPanel>
      <TabPanel>…</TabPanel>
      <TabPanel>…</TabPanel>
    </div>
  }
>
  <Tab>…</Tab>
</Tabs>

@ZeeshanTamboli here is my proposal ZeeshanTamboli#3

@siriwatknp Thanks for sharing the proposal. There are a lot of file changes so I didn't see it thoroughly.

That said, I think this PR should focus only on fixing the issues with Tabs and Tab, as mentioned in the description.

Supporting TabPanel directly inside Tabs (because now it has the Context) would require changing the API (like your suggested layout), which makes sense but might be better handled in a separate PR — or ideally in a future major version where we can fully and freely refactor the Tabs structure. For now, the Lab components still work well for TabPanel.

This PR is just about improving the core behavior when using Tabs and Tab alone.

[siriwatknp]

This PR is just about improving the core behavior when using Tabs and Tab alone.

Okay sounds good. let's keep the context internal and revise the context field names. Please check my suggestion.

[ZeeshanTamboli]

@DiegoAndai @siriwatknp Ready for further review.

[ZeeshanTamboli]

Closing this PR since fixing the SSR issue would require making the value prop mandatory, which would be a breaking change.