Repeater: Tab Flickering and Form State Loss on Tab Switch

[jerico-juegos]

Summary: Switching between tabs in the repeater layout causes flickering and loss of form state due to React unmounting/remounting behavior. This affects user experience and data entry reliability.

The original implementation renders only the active tab content:

{ items[ activeItem ] &&
  <Content
    key={ items[ activeItem ].key ?? activeItem }
    className='tf-repeater-tab-content'
  >
    { rowFields.map((control, i) => (
      <Row key={ control.name ?? i } className='tf-repeater-tab-row'>
        { renderItem(control, items[ activeItem ], activeItem) }
      </Row> )) }
  </Content> }

I tested rendering all tabs and toggling visibility via display: none.

Changes Tested Locally: assets/src/components/repeater/layout/tab/Tab.jsx

{ items && items.map((item, itemIndex) => (
  <Content
    key={ item.key ?? itemIndex }
    className='tf-repeater-tab-content'
    style={{ display: activeItem === itemIndex ? 'block' : 'none' }}
  >
    { rowFields.map((control, i) => (
      <Row key={ control.name ?? i } className='tf-repeater-tab-row'>
        { renderItem(control, item, itemIndex) }
      </Row> )) }
  </Content>
)) }

assets/src/components/base/tab/Tab.jsx

const Content = props => (
  <div 
    className={ 'tf-tab-content ' + (props.className ?? '') }
    style={ props.style }
  >
    { props.children }
  </div>
)

Observed Behavior After Patch:

• No flickering when switching tabs
• Input fields retain focus and values
• DOM remains stable

Questions:

• Is this a viable long-term solution?
• Are there performance concerns with rendering all tab contents?

[eliot-akira]

To see the difference, here's the suggested code change formatted with diff syntax highlight.

- { items[ activeItem ] &&
+ { items && items.map((item, itemIndex) => (
  <Content
-   key={ items[ activeItem ].key ?? activeItem }
+   key={ item.key ?? itemIndex }
    className='tf-repeater-tab-content'
+   style={{ display: activeItem === itemIndex ? 'block' : 'none' }}
  >
    { rowFields.map((control, i) => (
      <Row key={ control.name ?? i } className='tf-repeater-tab-row'>
-      { renderItem(control, items[ activeItem ], activeItem) }
+      { renderItem(control, item, itemIndex) }
      </Row> )) }
-  </Content> }
+  </Content>
+ )) }

From assets/src/components/repeater/layout/tab/Tab.jsx#L88

---

It's less efficient than rendering a single active tab on demand, but it may be an acceptable compromise for the simplicity of the solution. Otherwise, an alternative might be..

From the described behavior (loss of state on unmount) and the code (no state passed to rendered tab?), it sounds like the form fields are currently "uncontrolled".

• React docs: Sharing state between components - Controlled and uncontrolled

A way to retain state between tab switching is for the parent to pass the state (or a slice of it) down to the child components, which render "controlled" input fields whose values are restored after remount. But that may require a larger architectural change and cost of complexity. I'm not sure if it's practical for Repeater to keep track of all tab states.

---

A question came to mind, about rendering all tabs. What if there are conflicting duplicate input field names? I wonder how this situation is handled currently, and how it would behave after the suggested change.

@nicolas-jaussaud

[nicolas-jaussaud]

Thank you for creating the issue, and for the suggestions!

I made an attempt to switching to a controlled component in this PR, and it seems to fix the blinkering issue

I'm going to test it a little bit more later to be sure I didn't break anything and if everything still works I will merge it

A question came to mind, about rendering all tabs. What if there are conflicting duplicate input field names? I wonder how this situation is handled currently, and how it would behave after the suggested change.

I set it up to use a unique key instead, that is created for each new repeater item (here). We can use it as the component key for the content of each tab (I set it up here). So I think we should be safe for the tab content render

However it's definitely something that we should handle better. We don't check/handle that at all currently, and it will probably cause issues as the two controls will attempt to get/save their value from the same place

I guess we should at least sanitize the props we get from the user and remove duplicate controls if any (and maybe do a console.error to make the issue easier to debug)

[eliot-akira]

Nice, I see what you did here in the PR: all tabs are rendered but only the active tab renders its content. That efficiently solves the issue with maintaining tab state.

useEffect(() => {
  if ( props.isActive !== isActive ) setIsActive( props.isActive )
}, [ props.isActive ])

if ( ! isActive ) return <></>;

• Tab: Content - Switch to controlled component #53 (here)

You can just return instead of creating an empty fragment <></>. Since React 18 a component can return undefined.

• Allow components to render undefined facebook/react#21869

---

unique key..created for each new repeater item (here).. use it as the component key

Right, I see, this is a key for each tab component. React uses the key attribute to uniquely identify a component, mainly for efficiently re-rendering them in a list.

..We can use the special React attribute called key. When a key changes, React will create a new component instance rather than update the current one. Keys are usually used for dynamic lists.

-- https://reactjs.org/blog/2018/06/07/you-probably-dont-need-derived-state.html#recommendation-fully-uncontrolled-component-with-a-key

Ideally it's better to have a fixed deterministic key per item, instead of generating a new random key.

I think in this case we can use the tab index, since the key only needs to be unique among sibling elements in a list.

---

probably cause issues as the two controls will attempt to get/save their value

My question was about this, if there are multiple form input fields with the same name attribute. If all tabs' content is rendered, some fields might have conflicting names.

But for tabs in particular I see this won't be a problem, since with your solution only the active tab renders its content. So any duplicate field in a hidden tab will not affect it.

sanitize the props we get from the user and remove duplicate controls if any

That might be nice as a convenience - but probably not necessary since the current behavior is the same as regular HTML forms. Duplicate fields will just overwrite any existing value, and it's the responsibility of the user to ensure unique names. Maybe this is more suitable as part of the HTML linter in the template editor.

[nicolas-jaussaud]

You can just return instead of creating an empty fragment <></>. Since React 18 a component can return undefined.

Very cool thank you, I didn't know it was possible! I changed it in the last commit

I also merged the PR as I tested a bit more the changes and didn't notice new issues

Ideally it's better to have a fixed deterministic key per item, instead of generating a new random key.

I think in this case we can use the tab index, since the key only needs to be unique among sibling elements in a list.

Unfortunately we can't use the index directly

I was doing that at first, but because the index of each item might change (if we delete/clone a tab for example), one tab might end-up using the index that was previously used by another tab, which could prevent them from being re-rendered correctly

The good news is that, in theory, the random key we generate for each repeater item always stays the same, so it shouldn't be too bad. It's created for each new repeater item, and is then saved inside the repeater value to be sure it doesn't change

So, a repeater value will be an array like this:

[
  { key: '6401a27768211c', fieldName1: "something",  // ...  },
  { key: '6401a27f149614',  fieldName1: "something else", // ... }
]

The issue was that I couldn't use the index in repeater renders, but also didn't have a unique id I could use for the key, which is why I ended up generating one

What I do not like and I think was a bad idea is that this key is saved like it's a repeater field value. I could probably have found another way to deal with that. Something that I'm a bit worried is that it could be mistaken for a reliable way to identify a repeater item, while it's really just a way to prevent re-render issues

[eliot-akira]

the index of each item might change (if we delete/clone a tab for example)

I see what you mean, so the index can't be used to uniquely identify the same item.

the random key we generate for each repeater item always stays the same .. saved inside the repeater value

That makes sense, thanks for the explanation of how it works.

a way to prevent re-render issues

It is kind of unusual to save the temporarily generated key, used only for client rendering purpose, to be persisted in the database on server side. I guess the next time the repeater is rendered with those saved items, it uses the existing keys.

..I wonder if it would work if the keys are created on every component mount, and only used internally on client side, without having to save them in database to be reused across browser sessions. Well, maybe an idea for later.

---

could be mistaken for a reliable way to identify a repeater item

It reminds me of how in Advanced Custom Fields, it is necessary to provide a globally unique key for every field and field group that you register.

Each field group’s key and each field’s key must be unique. The key is a reference for ACF to find, save and load data. If 2 fields or 2 groups are added using the same key, the later will override the original.

https://www.advancedcustomfields.com/resources/register-fields-via-php/

(This relates to the topic earlier about handling duplicate field names. We can similarly mention it in the documentation about the need for unique field names.)

But that's a different meaning than the "keys" used to identify repeater items, which are dynamic on every save; compared to the "key" to identify the repeater field itself.

identify a repeater item

On the server side I suppose the intuitive way to access them would be to loop through repeater items and/or identify each by index position.

Anyway, after thinking through the context I understand why things are the way they are, a logical solution.