Hiding scenes leaves a noticeable gap

[KatieWoe]

For phetsims/qa#838. Seen on Win 11 Chrome.
When hiding scenes, there is a gap left between the remaining sims. This can be very noticeable.

[samreid]

@arouinfar did we previously decide to proceed with this as a known deficiency? And even if so, have we changed our tolerance for problems like this? What do you recommend?

[arouinfar]

@samreid this seems like something we would have punted on before dynamic layout. However, I think it's worth some investigation to try and close the gap. If it's not well-supported (since two separate controls need to be hidden), we can close as wontfix.

[samreid]

Patch from collaboration with @zepumph

Index: main/gravity-and-orbits/js/common/view/SceneSelectionControls.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/main/gravity-and-orbits/js/common/view/SceneSelectionControls.ts b/main/gravity-and-orbits/js/common/view/SceneSelectionControls.ts
--- a/main/gravity-and-orbits/js/common/view/SceneSelectionControls.ts	(revision 3770250d561e118cc3e72530ca7f86fe4a712e03)
+++ b/main/gravity-and-orbits/js/common/view/SceneSelectionControls.ts	(date 1664922670551)
@@ -18,6 +18,9 @@
 import GravityAndOrbitsScene from '../GravityAndOrbitsScene.js';
 import Tandem from '../../../../tandem/js/Tandem.js';
 import optionize, { EmptySelfOptions } from '../../../../phet-core/js/optionize.js';
+import DerivedProperty from '../../../../axon/js/DerivedProperty.js';
+import Emitter from '../../../../axon/js/Emitter.js';
+import Multilink from '../../../../axon/js/Multilink.js';
 
 type SceneSelectionControlsOptions = {
   tandem: Tandem;
@@ -34,19 +37,6 @@
     super( providedOptions );
     const options: SceneSelectionControlsOptions = merge( { tandem: Tandem.OPTIONAL }, providedOptions ) as SceneSelectionControlsOptions;
 
-    const resetButtons = modes.map( scene => {
-
-      // For the PhET-iO design, we decided to feature the radio button group and leave the reset buttons separate.
-      const sceneResetButton = new SceneResetButton( scene, {
-        tandem: options.tandem.createTandem( scene.resetButtonTandemName )
-      } );
-
-      // link reset buttons so that only the reset button next to the selected radio button is visible
-      sceneProperty.link( selectedScene => sceneResetButton.setVisible( selectedScene === scene ) );
-
-      return sceneResetButton;
-    } );
-
     const content = modes.map( scene => {
       return ( { value: scene, node: scene.iconImage, tandemName: scene.radioButtonTandemName } );
     } );
@@ -65,22 +55,61 @@
           deselectedLineWidth: 0
         }
       },
-      tandem: options.tandem.createTandem( 'sceneSelectionRadioButtonGroup' ),
+      tandem: options.tandem.createTandem( 'sceneSelectionRadioButtonGroup' )
+    } );
+
+    // const visiblePropertyEmitter = new Emitter();
+
+    const resetButtonAndCorrespondingRadioButtonVisibleProperty = modes.map( scene => {
+
+      // link reset buttons so that only the reset button next to the selected radio button is visible
+      const isSceneSelectedProperty = new DerivedProperty( [
+        sceneProperty
+      ], selectedScene => selectedScene === scene );
+
+      // For the PhET-iO design, we decided to feature the radio button group and leave the reset buttons separate.
+      const sceneResetButton = new SceneResetButton( scene, {
+        tandem: options.tandem.createTandem( scene.resetButtonTandemName )
+      } );
 
-      // Keep aligned with reset buttons, see https://github.com/phetsims/gravity-and-orbits/issues/348
-      excludeInvisibleChildrenFromBounds: false
+      const node = new Node( {
+        visibleProperty: isSceneSelectedProperty,
+        children: [ sceneResetButton ]
+      } );
+
+      // radioButtonGroup.getButtonForValue( scene ).visibleProperty.link();
+
+      // resetButtonVisibleProperty.link( visible => {
+      //   visiblePropertyEmitter.emit();
+      // } );
+      return [ node, radioButtonGroup.getButtonForValue( scene ).visibleProperty ];
     } );
+
 
     this.addChild( radioButtonGroup );
-    this.addChild( new VBox( {
-      children: resetButtons,
+    const resetButtonVBox = new VBox( {
+      children: resetButtonAndCorrespondingRadioButtonVisibleProperty.map( r => r[ 0 ] ),
       left: radioButtonGroup.right + 10,
       spacing: 5,
       y: 2,
 
       // Keep aligned with scene radio buttons, see https://github.com/phetsims/gravity-and-orbits/issues/348
-      excludeInvisibleChildrenFromBounds: false
-    } ) );
+      // excludeInvisibleChildrenFromBounds: false
+    } );
+
+    Multilink.multilinkAny( resetButtonAndCorrespondingRadioButtonVisibleProperty.map( r => r[ 1 ] ), () => {
+
+      const children = [];
+      for ( let i = 0; i < resetButtonAndCorrespondingRadioButtonVisibleProperty.length; i++ ) {
+        const resetButtonAndCorrespondingRadioButtonVisiblePropertyElement = resetButtonAndCorrespondingRadioButtonVisibleProperty[ i ];
+        if ( resetButtonAndCorrespondingRadioButtonVisibleProperty[ 1 ].value ) {
+          children.push( resetButtonAndCorrespondingRadioButtonVisibleProperty[ 0 ] );
+        }
+      }
+      // resetButtonVBox.children = resetButtonAndCorrespondingRadioButtonVisibleProperty.map( r => r[ 0] ).filter( resetButton => resetButton.visibleProperty.value );
+    } );
+
+    this.addChild( resetButtonVBox );
     this.addChild( new HStrut( 219 ) );
   }
 }
Index: main/sun/js/buttons/RectangularRadioButtonGroup.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/main/sun/js/buttons/RectangularRadioButtonGroup.ts b/main/sun/js/buttons/RectangularRadioButtonGroup.ts
--- a/main/sun/js/buttons/RectangularRadioButtonGroup.ts	(revision 829744c95d66ab3e489286256ceef651f0b03f19)
+++ b/main/sun/js/buttons/RectangularRadioButtonGroup.ts	(date 1664921578936)
@@ -97,6 +97,7 @@
 export default class RectangularRadioButtonGroup<T> extends FlowBox {
 
   private readonly disposeRadioButtonGroup: () => void;
+  private readonly radioButtonMap: Map<T, RectangularRadioButton<T>>;
 
   public constructor( property: Property<T>, items: RectangularRadioButtonItem<T>[], providedOptions?: RectangularRadioButtonGroupOptions ) {
 
@@ -167,6 +168,8 @@
 
     instanceCount++;
 
+    const radioButtonMap = new Map<T, RectangularRadioButton<T>>();
+
     // Maximum width of the line that strokes the button.
     const maxLineWidth = Math.max(
       options.radioButtonOptions.buttonAppearanceStrategyOptions!.selectedLineWidth!,
@@ -216,6 +219,8 @@
 
       const radioButton = new RectangularRadioButton( property, item.value, radioButtonOptions );
 
+      radioButtonMap.set( item.value, radioButton );
+
       // pdom - so the browser recognizes these buttons are in the same group. See instanceCount for more info.
       radioButton.setPDOMAttribute( 'name', CLASS_NAME + instanceCount );
 
@@ -295,6 +300,8 @@
 
     super( options );
 
+    this.radioButtonMap = radioButtonMap;
+
     // pdom - This node's primary sibling is aria-labelledby its own label, so the label content is read whenever
     // a member of the group receives focus.
     this.addAriaLabelledbyAssociation( {
@@ -322,6 +329,12 @@
     assert && phet.chipper.queryParameters.binder && InstanceRegistry.registerDataURL( 'sun', 'RectangularRadioButtonGroup', this );
   }
 
+  public getButtonForValue( value: T ): RectangularRadioButton<T> {
+    const result = this.radioButtonMap.get( value )!;
+    assert && assert( result, 'No button found for value: ' + value );
+    return result;
+  }
+
   public override dispose(): void {
     this.disposeRadioButtonGroup();
     super.dispose();

[samreid]

Fixed and ready for cherry picking.

[Nancy-Salpepi]

The layout now changes as you hide scenes and the panel size also changes dynamically.
There is only one instance where that isn't true: If only one scene is visible and the reset button is hidden, the panel size does not change--resulting in an empty space at the top.

[arouinfar]

@Nancy-Salpepi I think that behavior is acceptable. If a client wants to completely hide the scene radio buttons, they should use view.controlPanel.sceneControl.visibleProperty which behaves as expected: