Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ToggleGroupControlAsRadioGroup value is always undefined #56667

Closed
tellthemachines opened this issue Nov 30, 2023 · 2 comments · Fixed by #56678
Closed

ToggleGroupControlAsRadioGroup value is always undefined #56667

tellthemachines opened this issue Nov 30, 2023 · 2 comments · Fixed by #56678
Assignees
Labels
[Package] Components /packages/components [Type] Regression Related to a regression in the latest release

Comments

@tellthemachines
Copy link
Contributor

Description

The ChildLayoutControl has an effect that runs only on first mount, and resets the value of the control if in incorrect combination of attributes is detected. It then passes that value to ToggleGroupControl.

The problem is that ToggleGroupControlAsRadioGroup inside ToggleGroupControl never re-renders after the value changes. Looking into it a bit, it seems that this happens due to the effect in useComputeControlledOrUncontrolledValue running after the code beneath it, so that the component returns its original value as it the effect had never run.

This regression seems to have been introduced in #50278, which made its way into WP 6.4. The ToggleGroupControl was working correctly in previous releases.

Step-by-step reproduction instructions

  1. Create a Row block and add a Paragraph inside it;
  2. Select the paragraph and in the sidebar, under Dimensions, set width to "Fixed";
  3. An input should become visible underneath. Don't add anything in there for now;
  4. Save and reload the page;
  5. Reselect the Paragraph and see that the width is still set to "Fixed" but no input is now visible, and the help text is incorrect, as can be seen in this screenshot:
Screenshot 2023-11-30 at 5 17 22 pm

Screenshots, screen recording, code snippet

No response

Environment info

No response

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

@jsnajdr
Copy link
Member

jsnajdr commented Nov 30, 2023

This is how the bug is reproducible with a minimal example:

function Control( { value, onChange } ) {
  // if the value is `c`, reset it immediately to `b`
  useEffect( () => {
    if ( value === 'c' ) {
      onChange( 'b' );
    }
  }, [ value, onChange ] );

  return (
    <div>
      <span>Value is { value }</span>
      <ToggleGroupControl value={ value } onChange={ onChange } >
        <ToggleGroupControlOption value="a" label="a" />
        <ToggleGroupControlOption value="b" label="b" />
        <ToggleGroupControlOption value="c" label="c" />
      </ToggleGroupControl>
    </div>
  );
}

function App() {
  const [ value, setValue ] = useState( 'c' );
  return <Control value={ value } onChange={ setValue } />
}

The Toggle Group value is fully controlled, it should always reflect the value prop. And we have an effect that detects an "invalid" value c and resets it to b. Then, rendering Control with the c value should end up with Toggle Group where b is selected. But the actual result is that c is selected although the value is b. This is the screenshot:

Screenshot 2023-11-30 at 11 01 35

This is probably caused by the timing of the resetting effect. Toggle Group has some internal effect on its own, and there is some race condition resulting from their combination.

@ciampo
Copy link
Contributor

ciampo commented Nov 30, 2023

Thank you for reporting! Coincidentally, I've been looking into a similar bug last week, and I've just pushed a tentative fix in #56678. Hopefully we will cross two hurdles with one leap 🤞

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Type] Regression Related to a regression in the latest release
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants