[Bug]: TabPanels throws an error when using conditional rendering

[danielannan17]

Package

@carbon/react

Browser

Chrome

Package version

v1.61.0

React version

v18.2.0

Description

The page doesn't render due to the error.

Reproduction/example

https://stackblitz.com/edit/github-g3gust?file=src%2FApp.jsx

Steps to reproduce

None, errors in the logs

Suggested Severity

None

Application/PAL

No response

Code of Conduct

• I agree to follow this project's Code of Conduct
• I checked the current issues for duplicate problems

[jesnajoseijk]

@alisonjoseph @danielannan17 Can I take up this task ?

[benjambo13]

Is there any update on this issue?

If its of any use, we have this patch as a workaround - Adding the child && before cloneElement:

Which I believe would this line in the code base - https://github.com/carbon-design-system/carbon/blob/main/packages/react/src/components/Tabs/Tabs.tsx#L1723

We currently have Carbon pinned so the patch can be applied which is not ideal. Thanks in advance.

[s100]

I would strongly discourage patching Carbon in this way as this patch is brittle and would potentially require manually re-applying on every patch bump of Carbon - it would also involve pinning to a specific version of Carbon which forces downstream consumers to do the same.

A simple workaround would be to replace code like this:

const App = ({ showTab3 }) => (
  <Tabs>
    <TabList>
      <Tab>1</Tab>
      <Tab>2</Tab>
      {showTab3 && <Tab>3</Tab>}
    </TabList>
    <TabPanels>
      <TabPanel>one</TabPanel>
      <TabPanel>two</TabPanel>
      {showTab3 && <TabPanel>three</TabPanel>}
    </TabPanels>
  </Tabs>
)

with something like this:

const App = ({ showTab3 }) => (
  <Tabs>
    <TabList>
      {[
        <Tab key='tab1'>1</Tab>,
        <Tab key='tab2'>2</Tab>,
        showTab3 && <Tab key='tab3'>3</Tab>
      ].filter(Boolean)}
    </TabList>
    <TabPanels>
      {[
        <TabPanel key='tabPanel1'>one</TabPanel>,
        <TabPanel key='tabPanel2'>two</TabPanel>,
        showTab3 && <TabPanel key='tabPanel3'>three</TabPanel>
      ].filter(Boolean)}
    </TabPanels>
  </Tabs>
)

[Gururajj77]

I would strongly discourage patching Carbon in this way as this patch is brittle and would potentially require manually re-applying on every patch bump of Carbon - it would also involve pinning to a specific version of Carbon which forces downstream consumers to do the same.

A simple workaround would be to replace code like this:

const App = ({ showTab3 }) => (
  <Tabs>
    <TabList>
      <Tab>1</Tab>
      <Tab>2</Tab>
      {showTab3 && <Tab>3</Tab>}
    </TabList>
    <TabPanels>
      <TabPanel>one</TabPanel>
      <TabPanel>two</TabPanel>
      {showTab3 && <TabPanel>three</TabPanel>}
    </TabPanels>
  </Tabs>
)

with something like this:

const App = ({ showTab3 }) => (
  <Tabs>
    <TabList>
      {[
        <Tab key='tab1'>1</Tab>,
        <Tab key='tab2'>2</Tab>,
        showTab3 && <Tab key='tab3'>3</Tab>
      ].filter(Boolean)}
    </TabList>
    <TabPanels>
      {[
        <TabPanel key='tabPanel1'>one</TabPanel>,
        <TabPanel key='tabPanel2'>two</TabPanel>,
        showTab3 && <TabPanel key='tabPanel3'>three</TabPanel>
      ].filter(Boolean)}
    </TabPanels>
  </Tabs>
)

Hi @tay1orjones , what you think of this implementation, currently the above mentioned implementation works without any issue on our component, but we do not have any documentation showing this.

[s100]

@Gururajj77 My statement "I would strongly discourage patching Carbon in this way" is directed towards @benjambo, who is using a package called patch-package to patch Carbon after npm install - this is very difficult to consume. Likewise, my suggested workaround is aimed at people who are experiencing this bug.

Exactly what you, the maintainers of Carbon, should do inside the Carbon code to fix this bug is entirely up to you. @benjambo13's patch may be exactly what you need.

[tay1orjones]

@benjambo13 Conditionally rendering tab and tabpanel elements can run you into accessibility problems (1, 2).

I know the stackblitz example is intentionally contrived to just show the issue you're having, but it's not a valid way to use these components from an accessibility perspective. Without the elements in the DOM, screenreaders probably will not be able to properly convey how many tabs there are. The aria-controls properties will not have valid id values since the elements don't exist. Movement of focus could be delayed or not work as expected.

Is there a reason why conditionally rendering the contents isn't a feasible option for you instead of conditionally rendering the tabpanel?

 <Tabs>
   <TabList aria-label={''}>
     <Tab>1</Tab>
   </TabList>
-  <TabPanels>{false && <TabPanel>Test</TabPanel>}</TabPanels>
+  <TabPanels><TabPanel>{false && Test}</TabPanel></TabPanels>
 </Tabs>

[s100]

In the OP we have a rigged scenario where

1. the number of rendered <Tab>s can disagree with the number of rendered <TabPanel>s (sometimes a <Tab> has no <TabPanel>), and
2. sometimes the number of <TabPanel>s can be 0.

I think we're probably all in agreement that neither of these are a good idea, and I wouldn't expect <Tabs> to actively support either of these scenarios.

However, I think it's reasonable to want to conditionally render or omit a <Tab>/<TabPanel> pair. In general, I would not expect these two pieces of JSX to render differently, or cause accessibility problems:

<List>
  <Entry>1</Entry>
  <Entry>2</Entry>
</List>

<List>
  <Entry>1</Entry>
  <Entry>2</Entry>
  {false}
</List>

[Gururajj77]

As we have discussed that we will not be considering this issue as it could run into accessibility violations, closing this.

[s100]

As we have discussed that we will not be considering this issue as it could run into accessibility violations, closing this.

This is untrue, the linked accessibility notes do not indicate that any kind of issue would be introduced by fixing this bug, as I discussed. Please reopen this.