ToggleGroupControl: react correctly to external controlled updates

[ciampo]

Fixes #56667
Fixes the issue described in #56500

What?

This PR improves the internal logic in ToggleGroupControl used to detect whether the component is used in controlled or uncontrolled more

Why?

The previous logic wasn't reacting correctly to new values being passed to the component, and as a result it wasn't causing the component to update with the new values.

How?

Using useState instead of useRef to track internal state causes the component to re-render more eagerly when the "controlled" flag flips.

Testing Instructions

• In the editor:
  1. Apply changes from this PR
  2. Follow the steps described in ToggleGroupControlAsRadioGroup value is always undefined #56667
  3. Check that the ToggleGroupControl component resets correctly
• With unit tests:
  1. Undo the fix commit
  2. Run ToggleGroupControl's unit tests — the newly added test should fail without the fix in place.
  3. Add the fix back
  4. All ToggleGroupControl unit tests should pass

[github-actions]

Flaky tests detected in 749a884.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7052472587
📝 Reported issues:

• [Flaky Test] should partially select with shift + click (@webkit) #53524 in /test/e2e/specs/editor/various/multi-block-selection.spec.js

[jsnajdr]

Yes, this fixes the bug. The problem was that the render where valueProp changed from foo to bar was still returning value: undefined, because the hasEver value is still false. It's updated only a bit later in the effect.

This PR immediately schedules a next render where hasEver will be true and it will return value: 'bar'.

An implementation that doesn't need the extra render but returns the right value immediately when valueProp changes:

function useComputedControlled( value ) {
  const prevValue = usePrevious( value );
  const prevControlled = useRef( false );
  const isControlled = prevControlled.current || ( prevValue.current !== undefined && value !== undefined && prevValue.current !== value );
  useEffect( () => {
    prevControlled.current = isControlled;
  }, [ isControlled ] );
    
  if ( isControlled ) {
    return { value: value ?? '', defaultValue: undefined };
  }

  return { value: undefined, defaultValue: value };
}

[jsnajdr]

I'd change this to:

if ( ! hasEver && definedAndDifferent( prevVal, val ) {
  setHasEver( true );
}

That prevents scheduling setState calls that will be noops anyway, changing false to false.

Also, once in controlled mode, the component can't change back to uncontrolled, right? The component always starts as uncontrolled, and switches to controlled on the first re-render that has a different valueProp.

Then a better name for hasEver... is simply inControlledMode.

[ciampo]

That prevents scheduling setState calls that will be noops anyway, changing false to false.

That's a good suggestion, thank you!

Also, once in controlled mode, the component can't change back to uncontrolled, right?

Yes, this custom behaviour was necessary to be backwards compatible with how the component used to work before supporting uncontrolled mode.

I'm not going to implement this suggestion since I decided to refactor the hook following your other suggestion

[ciampo]

An implementation that doesn't need the extra render but returns the right value immediately when valueProp changes:

I like this! I went ahead and refactored the hook following your suggestion, plus with some minor adjustments (for example, prevValue doesn't have a current property).

I'll wait for folks to have a final round of review before merging

[tellthemachines]

This fixes the issue and doesn't seem to break other instances of ToggleGroupControl. Changes LGTM and thanks for adding the test!

[jsnajdr]

Thanks for accepting my suggestions 😉 Now I approve the PR even more.