Change the Fluid Displaced from Checkbox to Accordion Box

[DianaTavares]

Hi team! I talked with Taliesin about the focus order. It was super useful!! And she makes a super good observation on the lab screen. The Fluid displaced is important, it is not good to have it hidden in the Forces + checkboxes. Then I had an idea;
@AgustinVallejo and @zepumph, can a header have 2 lines?

I think that it needs to have 2 lines also in the actual design to have room for longer translations.

[samreid]

Yes, an AccordionBox title can be any scenery Node, including a VBox of Text. It looks like the Fluid Displaced is only on the Lab Screen in Buoyancy, is that correct? Want us to change it from checkbox to 2-line accordion box?

[DianaTavares]

Yes, it is only in the lab screen of Buoyancy and yes, please make the change!

That also means taking out “Fluid Displaced” from this menu:

[samreid]

I implemented it in the commits. It looks like this:

This also fixes a bug that was in main where resizing the window crashed it out:

Uncaught Error: Assertion failed: left control panels on lab screen are too big to fit under the ground
    at window.assertions.assertFunction (assert.js:28:13)
    at BuoyancyLabScreenView.ts:105:19
    at listener (Multilink.ts:111:11)
    at TinyProperty.notifyLoop (TinyEmitter.ts:213:7)
    at TinyProperty.emit (TinyEmitter.ts:185:18)
    at Property._notifyListeners (ReadOnlyProperty.ts:352:23)
    at Property.unguardedSet (ReadOnlyProperty.ts:296:14)
    at Property.set (ReadOnlyProperty.ts:276:12)
    at set value (Property.ts:54:11)
    at BuoyancyLabScreenView.layout (ScreenView.ts:210:37)

When hidden in studio, the layout flows correctly:

Here is a picture expanded, with double strings

I left a TODO in the commits for a reviewer consultation. One more commit coming to address max width better.

[samreid]

OK ready for review. I reverted some code from #144 so I feel @zepumph would be a good reviewer.

[zepumph]

• It is sad that you need to create the entire exandedProperty just to change the default. Perhaps we could add an option call defaultExpanded, which only applies when creating our own expanded. Seems simple enough, what do you think? I believe I have done this before which is why I'm mentioning it.

• I also removed the TODO. What do you think?

• The title font seems off to me.

• Can we remove showFluidDisplacedProperty now?

[samreid]

Something like this?

Subject: [PATCH] Make bottle and boat tab order before their control panels, see https://github.com/phetsims/density-buoyancy-common/issues/121
---
Index: density-buoyancy-common/js/common/view/DensityBuoyancyScreenView.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/density-buoyancy-common/js/common/view/DensityBuoyancyScreenView.ts b/density-buoyancy-common/js/common/view/DensityBuoyancyScreenView.ts
--- a/density-buoyancy-common/js/common/view/DensityBuoyancyScreenView.ts	(revision 0c4e3571e0d573676b2dc7a8a5701e5e64cd353f)
+++ b/density-buoyancy-common/js/common/view/DensityBuoyancyScreenView.ts	(date 1715652425748)
@@ -65,6 +65,7 @@
 import NumberIO from '../../../../tandem/js/types/NumberIO.js';
 import Utils from '../../../../dot/js/Utils.js';
 import createObservableArray, { ObservableArray } from '../../../../axon/js/createObservableArray.js';
+import Emitter from '../../../../axon/js/Emitter.js';
 
 // constants
 const MARGIN = DensityBuoyancyCommonConstants.MARGIN;
@@ -114,6 +115,8 @@
   // TODO: PhET-iO instrument for https://github.com/phetsims/density-buoyancy-common/issues/82
   protected waterLevelVolumeProperty: TReadOnlyProperty<number>;
 
+  protected readonly resetEmitter = new Emitter();
+
   public constructor( model: Model, providedOptions: SelfOptions ) {
 
     const scaleIncrease = 3.5;
@@ -574,6 +577,7 @@
       listener: () => {
         this.interruptSubtreeInput();
         model.reset();
+        this.resetEmitter.emit();
       },
       tandem: tandem.createTandem( 'resetAllButton' )
     } );
Index: density-buoyancy-common/js/buoyancy/view/BuoyancyLabScreenView.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/density-buoyancy-common/js/buoyancy/view/BuoyancyLabScreenView.ts b/density-buoyancy-common/js/buoyancy/view/BuoyancyLabScreenView.ts
--- a/density-buoyancy-common/js/buoyancy/view/BuoyancyLabScreenView.ts	(revision 0c4e3571e0d573676b2dc7a8a5701e5e64cd353f)
+++ b/density-buoyancy-common/js/buoyancy/view/BuoyancyLabScreenView.ts	(date 1715652546261)
@@ -34,7 +34,6 @@
 import ScaleHeightControl from '../../common/view/ScaleHeightControl.js';
 import fluid_displaced_scale_icon_png from '../../../images/fluid_displaced_scale_icon_png.js';
 import AccordionBox from '../../../../sun/js/AccordionBox.js';
-import BooleanProperty from '../../../../axon/js/BooleanProperty.js';
 import PhetFont from '../../../../scenery-phet/js/PhetFont.js';
 import CuboidView from '../../common/view/CuboidView.js';
 
@@ -60,10 +59,6 @@
 
     const fluidDisplacedAccordionBoxTandem = tandem.createTandem( 'fluidDisplacedAccordionBox' );
 
-    const accordionBoxIsExpandedProperty = new BooleanProperty( false, {
-      tandem: fluidDisplacedAccordionBoxTandem.createTandem( 'expandedProperty' )
-    } );
-
     const fluidDisplacedAccordionBox = new AccordionBox( new FluidDisplacedPanel( this.waterLevelVolumeProperty,
       maxBlockVolume,
       model.liquidMaterialProperty,
@@ -72,7 +67,7 @@
         font: new PhetFont( 14 ), // Matches the checkbox label font size
         maxWidth: 100
       } ),
-      expandedProperty: accordionBoxIsExpandedProperty,
+      expandedDefaultValue: false,
 
       titleAlignX: 'left',
       titleAlignY: 'center',
@@ -86,6 +81,8 @@
       tandem: fluidDisplacedAccordionBoxTandem
     } );
 
+    this.resetEmitter.addListener( () => fluidDisplacedAccordionBox.expandedProperty.reset() );
+
     const leftSideVBox = new VBox( {
       align: 'left',
       spacing: 5,
Index: sun/js/AccordionBox.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/sun/js/AccordionBox.ts b/sun/js/AccordionBox.ts
--- a/sun/js/AccordionBox.ts	(revision 70a7bed0587ca9b9d79306aa5c6a816a3efc2718)
+++ b/sun/js/AccordionBox.ts	(date 1715651891769)
@@ -29,8 +29,9 @@
   // If not provided, a Text node will be supplied. Should have and maintain well-defined bounds if passed in
   titleNode?: Node;
 
-  // If not provided, a BooleanProperty will be created, defaulting to true.
+  // If not provided, a BooleanProperty will be created, defaulting to the value of expandedDefaultValue.
   expandedProperty?: Property<boolean>;
+  expandedDefaultValue?: boolean;
 
   // If true (the default), we'll adjust the title so that it isn't pickable at all
   overrideTitleNodePickable?: boolean;
@@ -142,10 +143,16 @@
    */
   public constructor( contentNode: Node, providedOptions?: AccordionBoxOptions ) {
 
+    assert && providedOptions && assert(
+      !( providedOptions.hasOwnProperty( 'expandedProperty' ) && providedOptions.hasOwnProperty( 'expandedDefaultValue' ) ),
+      'cannot specify expandedProperty and expandedDefaultValue in providedOptions'
+    );
+
     const options = optionize<AccordionBoxOptions, StrictOmit<SelfOptions, 'expandCollapseButtonOptions'>, NodeOptions>()( {
 
       titleNode: null as unknown as Node,
       expandedProperty: null as unknown as BooleanProperty,
+      expandedDefaultValue: true,
       resize: true,
 
       overrideTitleNodePickable: true,
@@ -249,7 +256,7 @@
 
     this.expandedProperty = options.expandedProperty;
     if ( !this.expandedProperty ) {
-      this.expandedProperty = new BooleanProperty( true, {
+      this.expandedProperty = new BooleanProperty( options.expandedDefaultValue, {
         tandem: options.tandem.createTandem( 'expandedProperty' ),
         phetioFeatured: true
       } );

Also the title font does look off. I hypothesized the maxWidth was shrinking it a bit, but couldn't prove it. Do you think it could look a little different since it is RichText instead of Text?

Can we remove showFluidDisplacedProperty now?

Yes, let's do that. I forgot to do that in the patch above but can do it after review.

I also removed the TODO. What do you think?

That is certainly better! Maybe one day do you think we can ManualConstraint.create(parent, Node | TReadOnlyProperty<Bounds2>)?

[zepumph]

Excellent. Anything else here?

[samreid]

The commits look great, thanks! Closing.

[zepumph]

I'm seeing a few more things here while working on dynamic locale. One second on the close.

[zepumph]

I moved the accordionBox into the fluid displaced class, which felt nicer to me, since there were so many options associated with it. I also fixed a couple of i18n layout things, and a tiny color change (238 gray vs 240 gray background colors).