RectangularRadioButton does not propagate options related to `enabledProperty` to ButtonModel.

[pixelzoom]

In https://github.com/phetsims/greenhouse-effect/issues/300#issuecomment-1597373083, @arouinfar requested:

I would also really like to unfeature the enabledProperty and visibleProperty of the individual radio buttons in the concentrationControlModeRadioButtonGroup. However, they are featured in sun, not the sim, so I have no idea if this is even possible.

Disabling or hiding these buttons individually is nonsensical, but we punted on other options in https://github.com/phetsims/sun/issues/826.

In https://github.com/phetsims/greenhouse-effect/issues/300#issuecomment-1597509565, I said:

For the request in https://github.com/phetsims/greenhouse-effect/issues/300#issuecomment-1597373083, unfeaturing visibleProperty was easy, done in https://github.com/phetsims/greenhouse-effect/commit/e95977ba4c36c0b073085eeaed040cb531e6e6e5. enabledProperty should be able to be uninstrumented similarly, but it's not working -- setting phetioFeature, phetioDocumentation, or other metadata has no affect on the radio button's enabledProperty. I suspect this is a common-code problem, and I'll need to investigate further.

So the bit that's not finished here is to set enabledProperty to phetioFeatured: false for the individual buttons in concentrationControlModeRadioButtonGroup.

[pixelzoom]

I confirmed that this is a common-code bug. Over in geometric-options, I added this to the radioButtonOptions for OpticSurfaceTypeRadioButtonGroup.ts:

Subject: [PATCH] phetioFeatured, https://github.com/phetsims/greenhouse-effect/issues/300
---
Index: js/common/view/OpticSurfaceTypeRadioButtonGroup.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/view/OpticSurfaceTypeRadioButtonGroup.ts b/js/common/view/OpticSurfaceTypeRadioButtonGroup.ts
--- a/js/common/view/OpticSurfaceTypeRadioButtonGroup.ts    (revision 28d8609664ec6d06d8ee97b9a7f61abe7070eaba)
+++ b/js/common/view/OpticSurfaceTypeRadioButtonGroup.ts    (date 1687207092582)
@@ -50,6 +50,14 @@
           deselectedStroke: GOColors.curveRadioButtonDeselectedStrokeProperty,
           deselectedLineWidth: 2,
           selectedLineWidth: 2
+        },
+        visiblePropertyOptions: {
+          phetioDocumentation: 'This is my visibleProperty.',
+          phetioFeatured: false
+        },
+        enabledPropertyOptions: {
+          phetioDocumentation: 'This is my enabledProperty.',
+          phetioFeatured: false
         }
       }
     }, providedOptions );

In Studio, the tandem name is opticSurfaceTypeRadioButtonGroup. Inspecting its radio buttons, I see the expected metadata (from my patch) for visibleProperty:

For enabledProperty, the metadata in my patch has been ignored, and the defaults are still present:

[pixelzoom]

Here's the problem, in RectangularRadioButton.ts:

    // Note it shares a tandem with this, so the emitter will be instrumented as a child of the button
    const buttonModel = new ButtonModel( {
      tandem: options.tandem
    } );

ButtonModel is responsible for creating enabledProperty, but it's not being passed the options related to enabledProperty.

[pixelzoom]

Unfortunately, the problem is not only propagation of enabledPropertyOptions. ButtonModel extends EnabledComponent. From EnabledComponent, here are the "enabled" options that are not being propagated to ButtonModel by RectangularRadioButton:

  // if not provided, a Property will be created
  enabledProperty?: TReadOnlyProperty<boolean> | null;

  // initial value of enabledProperty if we create it, ignored if enabledProperty is provided
  enabled?: boolean;

  // options to enabledProperty if we create it, ignored if enabledProperty is provided
  enabledPropertyOptions?: EnabledPropertyOptions | null;

  // Whether the default-created enabledProperty should be instrumented for PhET-iO. Ignored if
  // options.enabledProperty is provided.
  phetioEnabledPropertyInstrumented?: boolean;

[pixelzoom]

Making this change in RectangularRadioButton.ts resolves the problem, but it's very brittle, and makes me nervous.

    const buttonModel = new ButtonModel( {
+     enabledProperty: options.enabledProperty,
+     enabled: options.enabled,
+     enabledPropertyOptions: options.enabledPropertyOptions,
+     phetioEnabledPropertyInstrumented: options.phetioEnabledPropertyInstrumented,
      tandem: options.tandem
    } );

[pixelzoom]

Since this is a general problem with RectangularRadioButton, I transferred this issue from greenhouse-effect to sun.

[pixelzoom]

Here's an improved version of https://github.com/phetsims/sun/issues/847#issuecomment-1597767326, but still brittle.

    // Several options must be propagated to ButtonModel, because it is responsible for enabledProperty.
    // tandem is also propagated because we want enabledProperty to appear as a child of this button.
    // See https://github.com/phetsims/sun/issues/847.
    const buttonModel = new ButtonModel( _.pick( options, 'enabledProperty', 'enabled', 'enabledPropertyOptions', 'phetioEnabledPropertyInstrumented', 'tandem' ) );

[pixelzoom]

Thinking about this a bit more... For a radio button, I see no reason to provide enabledPropery or enabled options -- the button should be controlling enabledProperty and its initially value, based on whether it's associated value matches the Property value. I also don't think that phetioEnabledPropertyInstrumented should be allowed - whether a radio button is enabled or not MUST be stateful. So I think that those options need to be excluded from RectangularRadioButtonOptions. That reduces the change in RectangularRadioButton.ts to:

    // ButtonModel is responsible for enabledProperty, so propagate enabledPropertyOptions.
    // tandem is also propagated because we want enabledProperty to appear as a child of this button.
    // See https://github.com/phetsims/sun/issues/847.
    const buttonModel = new ButtonModel( {
      enabledPropertyOptions: options.enabledPropertyOptions,
      tandem: options.tandem
    } );

[pixelzoom]

As it turns out, the state of enabledProperty for RectangularRadioButton has no relationship to the value of the Property. The look of the button is determined by appearance strategies, set by RadioButtonInteractionStateProperty (RectangularRadioButton.ts, line 115):

    const interactionStateProperty = new RadioButtonInteractionStateProperty( buttonModel, property, value );

So I'm not sure why we're making enabledProperty have phetioReadOnly: false. But that was the case before I started on this issue, and it feels like a separate problem (if anyone is inclined to address it via a new GitHub issue).

[pixelzoom]

Fixed in the above commit. Here are the relevant changes:

export type RectangularRadioButtonOptions = SelfOptions &
  // These options are not appropriate for radio buttons, see https://github.com/phetsims/sun/issues/847
  StrictOmit<RectangularButtonOptions, 'enabledProperty' | 'enabled'>;
...

    // ButtonModel is responsible for enabledProperty, so propagate enabledPropertyOptions.
    // tandem is also propagated because we want enabledProperty to appear as a child of this button.
    // Since enabledProperty is unrelated to the look of the button when selected/deselected, we've also included
    // phetioEnabledPropertyInstrumented so that one can opt out of this potentially confusing instrumentation.
    // See https://github.com/phetsims/sun/issues/847.
    const buttonModel = new ButtonModel( {
      enabledPropertyOptions: options.enabledPropertyOptions,
      tandem: options.tandem,
      phetioEnabledPropertyInstrumented: options.phetioEnabledPropertyInstrumented
    } );

To test that the excluded RectangularRadioButtonOptions were not used elsewhere, I tested locally with aqua, Mode = "Test Sims (Load Only)". Based on that test, I will not label this issue as "blocks publication".

grunt generate-phet-io-api --stable confirmed that this change resulted in no API changes.

Commit history suggests that @samreid did the instrumentation of ButtonModel in RectangularRadioButton.ts, see https://github.com/phetsims/sun/commit/9d7b14f7fbb3d25f6a1c07096c76d59d2096f239. So assigning to him to review.

[samreid]

I read through this issue and tested many sims in aqua with this test harness:

```diff Subject: [PATCH] Rename tandems, see https://github.com/phetsims/center-and-variability/issues/286 --- Index: js/buttons/RectangularRadioButton.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/buttons/RectangularRadioButton.ts b/js/buttons/RectangularRadioButton.ts --- a/js/buttons/RectangularRadioButton.ts (revision c1d17db98a961383e925c0308f073a8c0bde5498) +++ b/js/buttons/RectangularRadioButton.ts (date 1687991517077) @@ -115,6 +115,12 @@ phetioEnabledPropertyInstrumented: options.phetioEnabledPropertyInstrumented } ); + if ( options.enabledPropertyOptions || options.phetioEnabledPropertyInstrumented ) { + console.log( 'hello: ' + options.tandem.phetioID ); + console.log( options.enabledPropertyOptions ); + console.log( options.phetioEnabledPropertyInstrumented ); + } + const interactionStateProperty = new RadioButtonInteractionStateProperty( buttonModel, property, value ); super( buttonModel, interactionStateProperty, options ); ```

But I did see the changes in greenhouse effect in the by value and by date radio buttons. So this change feels modular and isolated, for the better in the fraction of cases where it applies, and is unlikely to cause unintentional side effects.

The changes seem reasonable to me and I'm inclined to close the issue, thanks!