Hiding scenes leaves a noticeable gap

[KatieWoe]

For https://github.com/phetsims/qa/issues/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

```diff 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 extends FlowBox { private readonly disposeRadioButtonGroup: () => void; + private readonly radioButtonMap: Map>; public constructor( property: Property, items: RectangularRadioButtonItem[], providedOptions?: RectangularRadioButtonGroupOptions ) { @@ -167,6 +168,8 @@ instanceCount++; + const radioButtonMap = new Map>(); + // 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 { + 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: