Use GroupItemOptions for ComboBox

samreid commented 1 year ago

After working on https://github.com/phetsims/sun/issues/787, I'm thinking we should apply the same pattern (createNode + tandemName) for ComboBoxItems. @pixelzoom does that sound good to you? If so, please assign to me and I can take the lead.

pixelzoom commented 1 year ago

Apply this pattern to ComboBoxItem does sound like the right thing to do. Thanks for taking the lead.

samreid commented 1 year ago

Progress so far:

```diff Index: main/sun/js/ComboBox.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/sun/js/ComboBox.ts b/main/sun/js/ComboBox.ts --- a/main/sun/js/ComboBox.ts (revision 15be3e7ec7b6b9ad89936a0f6ecdbf2e23885ea9) +++ b/main/sun/js/ComboBox.ts (date 1665189049304) @@ -41,6 +41,7 @@ import LinkableProperty from '../../axon/js/LinkableProperty.js'; import TinyProperty from '../../axon/js/TinyProperty.js'; import Multilink, { UnknownMultilink } from '../../axon/js/Multilink.js'; +import GroupItemOptions, { getGroupItemNodes } from './GroupItemOptions.js'; // const const LIST_POSITION_VALUES = [ 'above', 'below' ] as const; // where the list pops up relative to the button @@ -51,19 +52,13 @@ // the value associated with the item value: T; - // the node displayed in the combo box for the item - node: Node; - // Sound that will be played when this item is selected. If set to `null` a default sound will be used that is based // on this item's position in the combo box list. A value of `nullSoundPlayer` can be used to disable. soundPlayer?: TSoundPlayer | null; - // phet-io - the tandem name for this item's associated Node in the combo box - tandemName?: string | null; - // pdom - the label for this item's associated Node in the combo box a11yLabel?: PDOMValueType | null; -}; +} & GroupItemOptions; export type ComboBoxListPosition = typeof LIST_POSITION_VALUES[number]; export type ComboBoxAlign = typeof ALIGN_VALUES[number]; @@ -184,9 +179,6 @@ assert && assert( _.uniqBy( items, ( item: ComboBoxItem ) => item.value ).length === items.length, 'items must have unique values' ); assert && items.forEach( item => { - assert && assert( !item.node.hasPDOMContent, 'Accessibility is provided by ComboBoxItemNode and ' + - 'ComboBoxItem.a11yLabel. Additional PDOM content in the provided ' + - 'Node could break accessibility.' ); assert && assert( !item.tandemName || item.tandemName.endsWith( ComboBox.ITEM_TANDEM_NAME_SUFFIX ), `ComboBoxItem tandemName must end with '${ComboBox.ITEM_TANDEM_NAME_SUFFIX}': ${item.tandemName}` ); } ); @@ -242,6 +234,18 @@ phetioEnabledPropertyInstrumented: true // opt into default PhET-iO instrumented enabledProperty }, providedOptions ); + const nodes = getGroupItemNodes( items, options.tandem.createTandem( 'items' ) ); + + assert && nodes.forEach( node => { + assert && assert( !node.hasPDOMContent, 'Accessibility is provided by ComboBoxItemNode and ' + + 'ComboBoxItem.a11yLabel. Additional PDOM content in the provided ' + + 'Node could break accessibility.' ); + } ); + const elements = []; + for ( let i = 0; i < items.length; i++ ) { + elements[ i ] = { item: items[ i ], node: nodes[ i ] }; + } + // validate option values assert && assert( options.xMargin > 0 && options.yMargin > 0, `margins must be > 0, xMargin=${options.xMargin}, yMargin=${options.yMargin}` ); @@ -260,8 +264,9 @@ } // We'll need to adjust our button's preferred width if we have a label - const buttonPreferredWidthProperty = options.labelNode - ? new DerivedProperty( [ + const buttonPreferredWidthProperty = + options.labelNode ? + new DerivedProperty( [ this.localPreferredWidthProperty, options.labelNode.boundsProperty ], ( localPreferredWidth, labelBounds ) => { @@ -269,8 +274,7 @@ return localPreferredWidth === null ? null : localPreferredWidth - labelBounds.width - options.labelXSpacing; }, { reentrant: true - } ) - : this.localPreferredWidthProperty; + } ) : this.localPreferredWidthProperty; // We'll need to adjust our (incoming, from the button) minimum width if we have a label. let buttonMinimumWidthProperty = this.localMinimumWidthProperty; @@ -288,7 +292,7 @@ } ); } - this.button = new ComboBoxButton( property, items, { + this.button = new ComboBoxButton( property, items, nodes, { align: options.align, arrowDirection: ( options.listPosition === 'below' ) ? 'down' : 'up', cornerRadius: options.cornerRadius, @@ -321,7 +325,8 @@ } ); } - this.listBox = new ComboBoxListBox( property, items, + // TODO: https://github.com/phetsims/sun/issues/797 don't forget dispose + this.listBox = new ComboBoxListBox( property, items, nodes, this.hideListBox.bind( this ), // callback to hide the list box () => { this.button.blockNextVoicingFocusListener(); @@ -542,26 +547,26 @@ return this.listBox.isItemVisible( value ); } - public static getMaxItemWidthProperty( items: ComboBoxItem[] ): TReadOnlyProperty { - const widthProperties = _.flatten( items.map( item => { - const properties: TReadOnlyProperty[] = [ item.node.boundsProperty ]; - if ( mixesWidthSizable( item.node ) ) { - properties.push( item.node.isWidthResizableProperty ); - properties.push( item.node.minimumWidthProperty ); + public static getMaxItemWidthProperty( nodes: Node[] ): TReadOnlyProperty { + const widthProperties = _.flatten( nodes.map( node => { + const properties: TReadOnlyProperty[] = [ node.boundsProperty ]; + if ( mixesWidthSizable( node ) ) { + properties.push( node.isWidthResizableProperty ); + properties.push( node.minimumWidthProperty ); } return properties; } ) ); // @ts-ignore DerivedProperty isn't understanding the Property[] return new DerivedProperty( widthProperties, () => { - return Math.max( ...items.map( item => isWidthSizable( item.node ) ? item.node.minimumWidth || 0 : item.node.width ) ); + return Math.max( ...nodes.map( node => isWidthSizable( node ) ? node.minimumWidth || 0 : node.width ) ); } ); } - public static getMaxItemHeightProperty( items: ComboBoxItem[] ): TReadOnlyProperty { - const heightProperties = items.map( item => item.node.boundsProperty ); + public static getMaxItemHeightProperty( nodes: Node[] ): TReadOnlyProperty { + const heightProperties = nodes.map( node => node.boundsProperty ); // @ts-ignore DerivedProperty isn't understanding the Property[] return new DerivedProperty( heightProperties, () => { - return Math.max( ...items.map( item => item.node.height ) ); + return Math.max( ...nodes.map( node => node.height ) ); } ); } Index: main/tambo/js/demo/ui-components/view/UIComponentsScreenView.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/tambo/js/demo/ui-components/view/UIComponentsScreenView.ts b/main/tambo/js/demo/ui-components/view/UIComponentsScreenView.ts --- a/main/tambo/js/demo/ui-components/view/UIComponentsScreenView.ts (revision 5fa44dd369d0e620cf872d4438cae456cdb81ade) +++ b/main/tambo/js/demo/ui-components/view/UIComponentsScreenView.ts (date 1665187567746) @@ -167,9 +167,9 @@ { label: 'ComboBox', createNode: ( layoutBounds: Bounds2 ) => new ComboBox( new NumberProperty( 0 ), [ - { value: 0, node: new Text( 'Rainbows', { font: LABEL_FONT } ) }, - { value: 1, node: new Text( 'Unicorns', { font: LABEL_FONT } ) }, - { value: 2, node: new Text( 'Butterflies', { font: LABEL_FONT } ) } + { value: 0, createNode: tandem => new Text( 'Rainbows', { font: LABEL_FONT } ) }, + { value: 1, createNode: tandem => new Text( 'Unicorns', { font: LABEL_FONT } ) }, + { value: 2, createNode: tandem => new Text( 'Butterflies', { font: LABEL_FONT } ) } ], this, { center: layoutBounds.center } ) }, { Index: main/joist/js/preferences/RegionAndCultureComboBox.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/joist/js/preferences/RegionAndCultureComboBox.ts b/main/joist/js/preferences/RegionAndCultureComboBox.ts --- a/main/joist/js/preferences/RegionAndCultureComboBox.ts (revision 6c6750f47fb15732ec31e239850606ac742ccf0f) +++ b/main/joist/js/preferences/RegionAndCultureComboBox.ts (date 1665187507219) @@ -52,7 +52,7 @@ return { value: index, - node: itemContent + createNode: ( tandem: Tandem ) => itemContent }; } ); Index: main/joist/js/preferences/VoicingPanelSection.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/joist/js/preferences/VoicingPanelSection.ts b/main/joist/js/preferences/VoicingPanelSection.ts --- a/main/joist/js/preferences/VoicingPanelSection.ts (revision 6c6750f47fb15732ec31e239850606ac742ccf0f) +++ b/main/joist/js/preferences/VoicingPanelSection.ts (date 1665187507206) @@ -467,7 +467,7 @@ if ( voices.length === 0 ) { items.push( { value: null, - node: new Text( noVoicesAvailableStringProperty, PreferencesDialog.PANEL_SECTION_CONTENT_OPTIONS ), + createNode: ( tandem: Tandem ) => new Text( noVoicesAvailableStringProperty, PreferencesDialog.PANEL_SECTION_CONTENT_OPTIONS ), a11yLabel: noVoicesAvailableStringProperty } ); } @@ -475,7 +475,7 @@ voices.forEach( voice => { items.push( { value: voice, - node: new Text( voice.name, PreferencesDialog.PANEL_SECTION_CONTENT_OPTIONS ), + createNode: ( tandem: Tandem ) => new Text( voice.name, PreferencesDialog.PANEL_SECTION_CONTENT_OPTIONS ), a11yLabel: voice.name } ); } ); Index: main/greenhouse-effect/js/common/view/ThermometerAndReadout.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/greenhouse-effect/js/common/view/ThermometerAndReadout.ts b/main/greenhouse-effect/js/common/view/ThermometerAndReadout.ts --- a/main/greenhouse-effect/js/common/view/ThermometerAndReadout.ts (revision 11060b0634db2641b7f7524f42ef7b63ec07a7ff) +++ b/main/greenhouse-effect/js/common/view/ThermometerAndReadout.ts (date 1665187405681) @@ -221,7 +221,7 @@ return { value: propertyValue, - node: new NumberDisplay( property, propertyRange, numberDisplayOptions ), + createNode: tandem => new NumberDisplay( property, propertyRange, numberDisplayOptions ), tandemName: tandemName, a11yLabel: TemperatureDescriber.getTemperatureUnitsString( propertyValue ) }; Index: main/twixt/js/demo/EasingComboBox.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/twixt/js/demo/EasingComboBox.js b/main/twixt/js/demo/EasingComboBox.js --- a/main/twixt/js/demo/EasingComboBox.js (revision a22ad38df670cf634f8155fce7ee8d89fb14a8dc) +++ b/main/twixt/js/demo/EasingComboBox.js (date 1665188823136) @@ -23,19 +23,19 @@ const comboTextOptions = { font: new PhetFont( 16 ) }; const items = [ - { value: Easing.LINEAR, node: new Text( 'Linear', comboTextOptions ) }, - { value: Easing.QUADRATIC_IN_OUT, node: new Text( 'Quadratic in-out', comboTextOptions ) }, - { value: Easing.QUADRATIC_IN, node: new Text( 'Quadratic in', comboTextOptions ) }, - { value: Easing.QUADRATIC_OUT, node: new Text( 'Quadratic out', comboTextOptions ) }, - { value: Easing.CUBIC_IN_OUT, node: new Text( 'Cubic in-out', comboTextOptions ) }, - { value: Easing.CUBIC_IN, node: new Text( 'Cubic in', comboTextOptions ) }, - { value: Easing.CUBIC_OUT, node: new Text( 'Cubic out', comboTextOptions ) }, - { value: Easing.QUARTIC_IN_OUT, node: new Text( 'Quartic in-out', comboTextOptions ) }, - { value: Easing.QUARTIC_IN, node: new Text( 'Quartic in', comboTextOptions ) }, - { value: Easing.QUARTIC_OUT, node: new Text( 'Quartic out', comboTextOptions ) }, - { value: Easing.QUINTIC_IN_OUT, node: new Text( 'Quintic in-out', comboTextOptions ) }, - { value: Easing.QUINTIC_IN, node: new Text( 'Quintic in', comboTextOptions ) }, - { value: Easing.QUINTIC_OUT, node: new Text( 'Quintic out', comboTextOptions ) } + { value: Easing.LINEAR, createNode: tandem => new Text( 'Linear', comboTextOptions ) }, + { value: Easing.QUADRATIC_IN_OUT, createNode: tandem => new Text( 'Quadratic in-out', comboTextOptions ) }, + { value: Easing.QUADRATIC_IN, createNode: tandem => new Text( 'Quadratic in', comboTextOptions ) }, + { value: Easing.QUADRATIC_OUT, createNode: tandem => new Text( 'Quadratic out', comboTextOptions ) }, + { value: Easing.CUBIC_IN_OUT, createNode: tandem => new Text( 'Cubic in-out', comboTextOptions ) }, + { value: Easing.CUBIC_IN, createNode: tandem => new Text( 'Cubic in', comboTextOptions ) }, + { value: Easing.CUBIC_OUT, createNode: tandem => new Text( 'Cubic out', comboTextOptions ) }, + { value: Easing.QUARTIC_IN_OUT, createNode: tandem => new Text( 'Quartic in-out', comboTextOptions ) }, + { value: Easing.QUARTIC_IN, createNode: tandem => new Text( 'Quartic in', comboTextOptions ) }, + { value: Easing.QUARTIC_OUT, createNode: tandem => new Text( 'Quartic out', comboTextOptions ) }, + { value: Easing.QUINTIC_IN_OUT, createNode: tandem => new Text( 'Quintic in-out', comboTextOptions ) }, + { value: Easing.QUINTIC_IN, createNode: tandem => new Text( 'Quintic in', comboTextOptions ) }, + { value: Easing.QUINTIC_OUT, createNode: tandem => new Text( 'Quintic out', comboTextOptions ) } ]; super( easingProperty, items, listParent, options ); Index: main/density-buoyancy-common/js/buoyancy/view/ShapeSizeControlNode.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/density-buoyancy-common/js/buoyancy/view/ShapeSizeControlNode.ts b/main/density-buoyancy-common/js/buoyancy/view/ShapeSizeControlNode.ts --- a/main/density-buoyancy-common/js/buoyancy/view/ShapeSizeControlNode.ts (revision 329d94f227a28a0c893cd67642b4df0ac6bf71af) +++ b/main/density-buoyancy-common/js/buoyancy/view/ShapeSizeControlNode.ts (date 1665187371696) @@ -62,7 +62,7 @@ const comboBox = new ComboBox( massShapeProperty, MassShape.enumeration.values.map( massShape => { return { value: massShape, - node: new Text( shapeStringMap[ massShape.name ], { + createNode: tandem => new Text( shapeStringMap[ massShape.name ], { font: DensityBuoyancyCommonConstants.COMBO_BOX_ITEM_FONT, maxWidth: 160 } ), Index: main/tangible/js/mediaPipe/MediaPipe.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/tangible/js/mediaPipe/MediaPipe.ts b/main/tangible/js/mediaPipe/MediaPipe.ts --- a/main/tangible/js/mediaPipe/MediaPipe.ts (revision 98dba401dffec1bdb5651a4e13983500ef79085f) +++ b/main/tangible/js/mediaPipe/MediaPipe.ts (date 1665187567741) @@ -300,7 +300,7 @@ const label = device.label || `Camera ${i + 1}`; return { value: device.deviceId, - node: new Text( label ), + createNode: ( tandem: Tandem ) => new Text( label ), a11yLabel: label }; } ); Index: main/balancing-chemical-equations/js/introduction/view/ToolsComboBox.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/balancing-chemical-equations/js/introduction/view/ToolsComboBox.js b/main/balancing-chemical-equations/js/introduction/view/ToolsComboBox.js --- a/main/balancing-chemical-equations/js/introduction/view/ToolsComboBox.js (revision 561fe846206c7d43dd9a2a13a929f2a6e921449f) +++ b/main/balancing-chemical-equations/js/introduction/view/ToolsComboBox.js (date 1665188875068) @@ -45,9 +45,9 @@ } ); const items = [ - { value: BalancedRepresentation.NONE, node: new Text( BalancingChemicalEquationsStrings.none, { font: FONT, maxWidth: 100 } ) }, - { value: BalancedRepresentation.BALANCE_SCALES, node: new Image( scales_png, { scale: 0.1875 } ) }, - { value: BalancedRepresentation.BAR_CHARTS, node: new Image( charts_png, { scale: 0.375 } ) } + { value: BalancedRepresentation.NONE, createNode: tandem => new Text( BalancingChemicalEquationsStrings.none, { font: FONT, maxWidth: 100 } ) }, + { value: BalancedRepresentation.BALANCE_SCALES, createNode: tandem => new Image( scales_png, { scale: 0.1875 } ) }, + { value: BalancedRepresentation.BAR_CHARTS, createNode: tandem => new Image( charts_png, { scale: 0.375 } ) } ]; super( balanceRepresentationProperty, items, parentNode, options ); Index: main/ph-scale/js/common/view/SoluteComboBox.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/ph-scale/js/common/view/SoluteComboBox.ts b/main/ph-scale/js/common/view/SoluteComboBox.ts --- a/main/ph-scale/js/common/view/SoluteComboBox.ts (revision 03e802164a667ce6b77de8446e923ef2580663a8) +++ b/main/ph-scale/js/common/view/SoluteComboBox.ts (date 1665187507208) @@ -63,7 +63,7 @@ items.push( { value: solute, - node: hBox, + createNode: tandem => hBox, tandemName: `${solute.tandemName}${ComboBox.ITEM_TANDEM_NAME_SUFFIX}` } ); } ); Index: main/density-buoyancy-common/js/common/view/DensityControlNode.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/density-buoyancy-common/js/common/view/DensityControlNode.ts b/main/density-buoyancy-common/js/common/view/DensityControlNode.ts --- a/main/density-buoyancy-common/js/common/view/DensityControlNode.ts (revision 329d94f227a28a0c893cd67642b4df0ac6bf71af) +++ b/main/density-buoyancy-common/js/common/view/DensityControlNode.ts (date 1665187752283) @@ -43,7 +43,7 @@ ].map( material => { return { value: material, - node: new Text( material.name, { + createNode: tandem => new Text( material.name, { font: DensityBuoyancyCommonConstants.COMBO_BOX_ITEM_FONT, maxWidth: 160 } ), Index: main/quadrilateral/js/quadrilateral/view/sound/QuadrilateralSoundOptionsNode.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/quadrilateral/js/quadrilateral/view/sound/QuadrilateralSoundOptionsNode.ts b/main/quadrilateral/js/quadrilateral/view/sound/QuadrilateralSoundOptionsNode.ts --- a/main/quadrilateral/js/quadrilateral/view/sound/QuadrilateralSoundOptionsNode.ts (revision e514aa56b3a68e19746965a040bb6802cf46d827) +++ b/main/quadrilateral/js/quadrilateral/view/sound/QuadrilateralSoundOptionsNode.ts (date 1665187507215) @@ -56,62 +56,62 @@ const designComboBox = new CarouselComboBox( model.soundDesignProperty, [ { value: SoundDesign.MAINTENANCE_SOUNDS, - node: new Text( 'Maintenance Sounds', LABEL_TEXT_OPTIONS ), + createNode: tandem => new Text( 'Maintenance Sounds', LABEL_TEXT_OPTIONS ), tandemName: `successWithMaintenance${ComboBox.ITEM_TANDEM_NAME_SUFFIX}` }, { value: SoundDesign.SUCCESS_SOUNDS, - node: new Text( 'Success Sounds', LABEL_TEXT_OPTIONS ), + createNode: tandem => new Text( 'Success Sounds', LABEL_TEXT_OPTIONS ), tandemName: `success${ComboBox.ITEM_TANDEM_NAME_SUFFIX}` }, { value: SoundDesign.QUARTET, - node: new Text( 'Quartet', LABEL_TEXT_OPTIONS ), + createNode: tandem => new Text( 'Quartet', LABEL_TEXT_OPTIONS ), tandemName: `quartet${ComboBox.ITEM_TANDEM_NAME_SUFFIX}` }, { value: SoundDesign.PARALLELS_VOLUME, - node: new Text( 'Parallels Volume', LABEL_TEXT_OPTIONS ), + createNode: tandem => new Text( 'Parallels Volume', LABEL_TEXT_OPTIONS ), tandemName: `parallelsVolume${ComboBox.ITEM_TANDEM_NAME_SUFFIX}` }, { value: SoundDesign.PARALLELS_STACCATO, - node: new Text( 'Parallels Staccato', LABEL_TEXT_OPTIONS ), + createNode: tandem => new Text( 'Parallels Staccato', LABEL_TEXT_OPTIONS ), tandemName: `parallelsStaccato${ComboBox.ITEM_TANDEM_NAME_SUFFIX}` }, { value: SoundDesign.TRACKS_BUILD_UP, - node: new Text( 'Tracks - Build Up', LABEL_TEXT_OPTIONS ), + createNode: tandem => new Text( 'Tracks - Build Up', LABEL_TEXT_OPTIONS ), tandemName: `tracksBuildUp${ComboBox.ITEM_TANDEM_NAME_SUFFIX}` }, { value: SoundDesign.TRACKS_VOLUME_EMPHASIS, - node: new Text( 'Tracks - Volume Emphasis', LABEL_TEXT_OPTIONS ), + createNode: tandem => new Text( 'Tracks - Volume Emphasis', LABEL_TEXT_OPTIONS ), tandemName: `tracksVolume${ComboBox.ITEM_TANDEM_NAME_SUFFIX}` }, { value: SoundDesign.TRACKS_MELODY, - node: new Text( 'Tracks - Melody', LABEL_TEXT_OPTIONS ), + createNode: tandem => new Text( 'Tracks - Melody', LABEL_TEXT_OPTIONS ), tandemName: `tracksMelody${ComboBox.ITEM_TANDEM_NAME_SUFFIX}` }, { value: SoundDesign.TRACKS_ARPEGGIO, - node: new Text( 'Tracks - Arpeggio', LABEL_TEXT_OPTIONS ), + createNode: tandem => new Text( 'Tracks - Arpeggio', LABEL_TEXT_OPTIONS ), tandemName: `tracksArpeggio${ComboBox.ITEM_TANDEM_NAME_SUFFIX}` }, { value: SoundDesign.TRACKS_BUILD_UP_SIMPLE, - node: new Text( 'Tracks - Build Up - Simple', LABEL_TEXT_OPTIONS ), + createNode: tandem => new Text( 'Tracks - Build Up - Simple', LABEL_TEXT_OPTIONS ), tandemName: `tracksBuildUpSimple${ComboBox.ITEM_TANDEM_NAME_SUFFIX}` }, { value: SoundDesign.TRACKS_MELODY_SIMPLE, - node: new Text( 'Tracks - Melody - Simple', LABEL_TEXT_OPTIONS ), + createNode: tandem => new Text( 'Tracks - Melody - Simple', LABEL_TEXT_OPTIONS ), tandemName: `tracksMelodySimple${ComboBox.ITEM_TANDEM_NAME_SUFFIX}` }, { value: SoundDesign.TRACKS_MELODY_MAPPING, - node: new Text( 'Tracks - Melody - Mapping', LABEL_TEXT_OPTIONS ), + createNode: tandem => new Text( 'Tracks - Melody - Mapping', LABEL_TEXT_OPTIONS ), tandemName: `tracksMelodyMapping${ComboBox.ITEM_TANDEM_NAME_SUFFIX}` } ], { Index: main/geometric-optics/js/common/view/OpticalObjectChoiceComboBox.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/geometric-optics/js/common/view/OpticalObjectChoiceComboBox.ts b/main/geometric-optics/js/common/view/OpticalObjectChoiceComboBox.ts --- a/main/geometric-optics/js/common/view/OpticalObjectChoiceComboBox.ts (revision 1c78c5492a945a5331049ff87c3e49714a561fbf) +++ b/main/geometric-optics/js/common/view/OpticalObjectChoiceComboBox.ts (date 1665187405676) @@ -76,7 +76,7 @@ // create and add combo box item to the array items.push( { value: opticalObjectChoice, - node: hBox, + createNode: tandem => hBox, tandemName: itemTandemName } ); } ); Index: main/beers-law-lab/js/concentration/view/SoluteComboBox.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/beers-law-lab/js/concentration/view/SoluteComboBox.ts b/main/beers-law-lab/js/concentration/view/SoluteComboBox.ts --- a/main/beers-law-lab/js/concentration/view/SoluteComboBox.ts (revision d7631720b8b880d82ed6b96d3180fe6cf553d9a2) +++ b/main/beers-law-lab/js/concentration/view/SoluteComboBox.ts (date 1665187371693) @@ -78,7 +78,7 @@ return { value: solute, - node: hBox, + createNode: tandem => hBox, tandemName: `${solute.tandemName}${ComboBox.ITEM_TANDEM_NAME_SUFFIX}` }; } Index: main/density-buoyancy-common/js/common/view/MaterialMassVolumeControlNode.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/density-buoyancy-common/js/common/view/MaterialMassVolumeControlNode.ts b/main/density-buoyancy-common/js/common/view/MaterialMassVolumeControlNode.ts --- a/main/density-buoyancy-common/js/common/view/MaterialMassVolumeControlNode.ts (revision 329d94f227a28a0c893cd67642b4df0ac6bf71af) +++ b/main/density-buoyancy-common/js/common/view/MaterialMassVolumeControlNode.ts (date 1665187371682) @@ -257,7 +257,7 @@ ...materials.map( material => { return { value: MaterialEnumeration[ material.identifier! ], - node: new Text( material.name, { + createNode: ( tandem: Tandem ) => new Text( material.name, { font: DensityBuoyancyCommonConstants.COMBO_BOX_ITEM_FONT, maxWidth: comboMaxWidth } ), @@ -266,7 +266,7 @@ } ), { value: MaterialEnumeration.CUSTOM, - node: new Text( DensityBuoyancyCommonStrings.material.customStringProperty, { + createNode: tandem => new Text( DensityBuoyancyCommonStrings.material.customStringProperty, { font: DensityBuoyancyCommonConstants.COMBO_BOX_ITEM_FONT, maxWidth: comboMaxWidth } ), Index: main/density-buoyancy-common/js/common/view/GravityControlNode.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/density-buoyancy-common/js/common/view/GravityControlNode.ts b/main/density-buoyancy-common/js/common/view/GravityControlNode.ts --- a/main/density-buoyancy-common/js/common/view/GravityControlNode.ts (revision 329d94f227a28a0c893cd67642b4df0ac6bf71af) +++ b/main/density-buoyancy-common/js/common/view/GravityControlNode.ts (date 1665187752286) @@ -44,7 +44,7 @@ ].map( gravity => { return { value: gravity, - node: new Text( gravity.name, { + createNode: tandem => new Text( gravity.name, { font: DensityBuoyancyCommonConstants.COMBO_BOX_ITEM_FONT, maxWidth: 160 } ), Index: main/area-model-common/js/generic/view/GenericLayoutSelectionNode.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/area-model-common/js/generic/view/GenericLayoutSelectionNode.js b/main/area-model-common/js/generic/view/GenericLayoutSelectionNode.js --- a/main/area-model-common/js/generic/view/GenericLayoutSelectionNode.js (revision 5f7ec8a298030f875aa747b1499c135f273cf05a) +++ b/main/area-model-common/js/generic/view/GenericLayoutSelectionNode.js (date 1665188875013) @@ -32,7 +32,7 @@ width -= 1; const comboBoxItems = GenericLayout.VALUES.map( layout => ( { - node: new HBox( { + createNode: tandem => new HBox( { children: [ createLayoutIcon( layout.size, 0.7 ), new Text( `${layout.size.height}x${layout.size.width}`, { Index: main/models-of-the-hydrogen-atom/js/common/view/DeBroglieViewComboBox.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/models-of-the-hydrogen-atom/js/common/view/DeBroglieViewComboBox.ts b/main/models-of-the-hydrogen-atom/js/common/view/DeBroglieViewComboBox.ts --- a/main/models-of-the-hydrogen-atom/js/common/view/DeBroglieViewComboBox.ts (revision a3b927c6583ff9d663b77a3f29f2b649e8fa83c4) +++ b/main/models-of-the-hydrogen-atom/js/common/view/DeBroglieViewComboBox.ts (date 1665187507201) @@ -55,9 +55,9 @@ }, textOptions ) ); const items: ComboBoxItem[] = [ - { value: 'radial', node: radialViewText, tandemName: `radial${ComboBox.ITEM_TANDEM_NAME_SUFFIX}` }, - { value: '3D', node: threeDViewText, tandemName: `3D${ComboBox.ITEM_TANDEM_NAME_SUFFIX}` }, - { value: 'brightness', node: brightnessViewText, tandemName: `brightness${ComboBox.ITEM_TANDEM_NAME_SUFFIX}` } + { value: 'radial', createNode: tandem => radialViewText, tandemName: `radial${ComboBox.ITEM_TANDEM_NAME_SUFFIX}` }, + { value: '3D', createNode: tandem => threeDViewText, tandemName: `3D${ComboBox.ITEM_TANDEM_NAME_SUFFIX}` }, + { value: 'brightness', createNode: tandem => brightnessViewText, tandemName: `brightness${ComboBox.ITEM_TANDEM_NAME_SUFFIX}` } ]; super( deBroglieViewProperty, items, listboxParent, options ); Index: main/scenery-phet/js/ComboBoxDisplay.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/scenery-phet/js/ComboBoxDisplay.ts b/main/scenery-phet/js/ComboBoxDisplay.ts --- a/main/scenery-phet/js/ComboBoxDisplay.ts (revision ce185329efd1931c873ba7b91e3c298b8e70d343) +++ b/main/scenery-phet/js/ComboBoxDisplay.ts (date 1665187790227) @@ -108,8 +108,8 @@ comboBoxItems.push( { value: item.choice, - node: itemNode, - tandemName: item.tandemName || null + createNode: tandem => itemNode, + tandemName: item.tandemName } ); } ); Index: main/sun/js/ComboBoxListBox.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/sun/js/ComboBoxListBox.ts b/main/sun/js/ComboBoxListBox.ts --- a/main/sun/js/ComboBoxListBox.ts (revision 15be3e7ec7b6b9ad89936a0f6ecdbf2e23885ea9) +++ b/main/sun/js/ComboBoxListBox.ts (date 1665189248787) @@ -8,7 +8,7 @@ import PhetioAction from '../../tandem/js/PhetioAction.js'; import optionize, { combineOptions } from '../../phet-core/js/optionize.js'; -import { KeyboardUtils, SceneryEvent, SpeakingOptions, TInputListener, TPaint, VBox, VoicingNode } from '../../scenery/js/imports.js'; +import { KeyboardUtils, SceneryEvent, SpeakingOptions, TInputListener, TPaint, VBox, VoicingNode, Node } from '../../scenery/js/imports.js'; import multiSelectionSoundPlayerFactory from '../../tambo/js/multiSelectionSoundPlayerFactory.js'; import generalCloseSoundPlayer from '../../tambo/js/shared-sound-players/generalCloseSoundPlayer.js'; import generalOpenSoundPlayer from '../../tambo/js/shared-sound-players/generalOpenSoundPlayer.js'; @@ -52,13 +52,14 @@ /** * @param property * @param items + * @param nodes * @param hideListBoxCallback - called to hide the list box * @param focusButtonCallback - called to transfer focus to the combo box's button * @param voiceOnSelectionNode - Node to voice the response when selecting a combo box item. * @param tandem * @param providedOptions */ - public constructor( property: TProperty, items: ComboBoxItem[], hideListBoxCallback: () => void, + public constructor( property: TProperty, items: ComboBoxItem[], nodes: Node[], hideListBoxCallback: () => void, focusButtonCallback: () => void, voiceOnSelectionNode: VoicingNode, tandem: Tandem, providedOptions?: ComboBoxListBoxOptions ) { const options = optionize()( { @@ -140,8 +141,8 @@ }; // Compute max item size - const maxItemWidthProperty = ComboBox.getMaxItemWidthProperty( items ); - const maxItemHeightProperty = ComboBox.getMaxItemHeightProperty( items ); + const maxItemWidthProperty = ComboBox.getMaxItemWidthProperty( nodes ); + const maxItemHeightProperty = ComboBox.getMaxItemHeightProperty( nodes ); // Uniform dimensions for all highlighted items in the list, highlight overlaps margin by 50% const highlightWidthProperty = new DerivedProperty( [ maxItemWidthProperty ], width => width + options.xMargin ); @@ -152,7 +153,7 @@ items.forEach( ( item, index ) => { // Create the list item node - const listItemNode = new ComboBoxListItemNode( item, highlightWidthProperty, highlightHeightProperty, + const listItemNode = new ComboBoxListItemNode( item, nodes[ index ], highlightWidthProperty, highlightHeightProperty, combineOptions( { align: options.align, highlightFill: options.highlightFill, Index: main/sun/js/demo/DemosScreenView.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/sun/js/demo/DemosScreenView.ts b/main/sun/js/demo/DemosScreenView.ts --- a/main/sun/js/demo/DemosScreenView.ts (revision 15be3e7ec7b6b9ad89936a0f6ecdbf2e23885ea9) +++ b/main/sun/js/demo/DemosScreenView.ts (date 1665187541029) @@ -110,7 +110,7 @@ const items = demos.map( ( demo: SunDemo ) => { return { value: demo, - node: new Text( demo.label, { font: COMBO_BOX_ITEM_FONT } ) + createNode: ( tandem: Tandem ) => new Text( demo.label, { font: COMBO_BOX_ITEM_FONT } ) }; } ); Index: main/molecule-polarity/js/realmolecules/view/RealMoleculesComboBox.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/molecule-polarity/js/realmolecules/view/RealMoleculesComboBox.ts b/main/molecule-polarity/js/realmolecules/view/RealMoleculesComboBox.ts --- a/main/molecule-polarity/js/realmolecules/view/RealMoleculesComboBox.ts (revision ed6cabedc7b4577fd21e25f7c038ca4267ebe3eb) +++ b/main/molecule-polarity/js/realmolecules/view/RealMoleculesComboBox.ts (date 1665187507192) @@ -70,7 +70,7 @@ return { value: molecule, - node: node, + createNode: tandem => node, tandemName: `${molecule.tandem.name}${ComboBox.ITEM_TANDEM_NAME_SUFFIX}` }; } Index: main/sun/js/ComboBoxListItemNode.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/sun/js/ComboBoxListItemNode.ts b/main/sun/js/ComboBoxListItemNode.ts --- a/main/sun/js/ComboBoxListItemNode.ts (revision 15be3e7ec7b6b9ad89936a0f6ecdbf2e23885ea9) +++ b/main/sun/js/ComboBoxListItemNode.ts (date 1665189232674) @@ -44,7 +44,7 @@ public readonly item: ComboBoxItem; - public constructor( item: ComboBoxItem, highlightWidthProperty: TReadOnlyProperty, highlightHeightProperty: TReadOnlyProperty, providedOptions?: ComboBoxListItemNodeOptions ) { + public constructor( item: ComboBoxItem, node: Node, highlightWidthProperty: TReadOnlyProperty, highlightHeightProperty: TReadOnlyProperty, providedOptions?: ComboBoxListItemNodeOptions ) { const options = optionize()( { @@ -98,7 +98,7 @@ // Wrapper for the item's Node. Do not transform item.node because it is shared with ComboBoxButton! const itemNodeWrapper = new Node( { - children: [ item.node ] + children: [ node ] } ); highlightWidthProperty.link( width => { Index: main/beers-law-lab/js/beerslaw/view/SolutionComboBox.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/beers-law-lab/js/beerslaw/view/SolutionComboBox.ts b/main/beers-law-lab/js/beerslaw/view/SolutionComboBox.ts --- a/main/beers-law-lab/js/beerslaw/view/SolutionComboBox.ts (revision d7631720b8b880d82ed6b96d3180fe6cf553d9a2) +++ b/main/beers-law-lab/js/beerslaw/view/SolutionComboBox.ts (date 1665187371690) @@ -85,7 +85,7 @@ return { value: solution, - node: hBox, + createNode: tandem => hBox, tandemName: `${solution.tandemName}${ComboBox.ITEM_TANDEM_NAME_SUFFIX}` }; } Index: main/sun/js/ComboBoxButton.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/sun/js/ComboBoxButton.ts b/main/sun/js/ComboBoxButton.ts --- a/main/sun/js/ComboBoxButton.ts (revision 15be3e7ec7b6b9ad89936a0f6ecdbf2e23885ea9) +++ b/main/sun/js/ComboBoxButton.ts (date 1665189159345) @@ -66,7 +66,7 @@ private arrow: Path; private vSeparator: VSeparatorDeprecated; - public constructor( property: TProperty, items: ComboBoxItem[], providedOptions?: ComboBoxButtonOptions ) { + public constructor( property: TProperty, items: ComboBoxItem[], nodes: Node[], providedOptions?: ComboBoxButtonOptions ) { const options = optionize()( { @@ -113,8 +113,8 @@ const itemYMargin = options.yMargin; // Compute max item size - const maxItemWidthProperty = ComboBox.getMaxItemWidthProperty( items ); - const maxItemHeightProperty = ComboBox.getMaxItemHeightProperty( items ); + const maxItemWidthProperty = ComboBox.getMaxItemWidthProperty( nodes ); + const maxItemHeightProperty = ComboBox.getMaxItemHeightProperty( nodes ); const arrow = new Path( null, { fill: options.arrowFill @@ -122,6 +122,10 @@ // Wrapper for the selected item's Node. // Do not transform ComboBoxItem.node because it is shared with ComboBoxListItemNode. + + + const matchingItem = _.find( items, item => item.value === property.value )!; + const index = items.indexOf( matchingItem ); const itemNodeWrapper = new Node( { layoutOptions: { yMargin: itemYMargin, @@ -129,7 +133,7 @@ xAlign: options.align }, children: [ - _.find( items, item => item.value === property.value )!.node + nodes[ index ] ] } ); @@ -236,9 +240,11 @@ // find the ComboBoxItem whose value matches the property's value item = _.find( items, item => item.value === value )!; assert && assert( item, `no item found for value: ${value}` ); + const index = items.indexOf( item ); + const node = nodes[ index ]; // add the associated node - itemNodeWrapper.addChild( item.node ); + itemNodeWrapper.addChild( node ); // pdom this.innerContent = ( item.a11yLabel || null ); Index: main/ratio-and-proportion/js/create/view/TickMarkRangeComboBoxNode.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/ratio-and-proportion/js/create/view/TickMarkRangeComboBoxNode.ts b/main/ratio-and-proportion/js/create/view/TickMarkRangeComboBoxNode.ts --- a/main/ratio-and-proportion/js/create/view/TickMarkRangeComboBoxNode.ts (revision 27b938fa83e1ed93c75233316dca102f5e97fe93) +++ b/main/ratio-and-proportion/js/create/view/TickMarkRangeComboBoxNode.ts (date 1665187915471) @@ -35,7 +35,7 @@ private tickMarkRangeProperty: Property; public constructor( tickMarkRangeProperty: Property, comboBoxParent: Node, - tickMarkViewProperty: EnumerationProperty ) { + tickMarkViewProperty: EnumerationProperty ) { super(); this.tickMarkRangeMap = { @@ -46,12 +46,13 @@ this.tickMarkRangeProperty = tickMarkRangeProperty; const items = [ - { value: 10, node: new RichText( this.tickMarkRangeMap[ 10 ], RANGE_TEXT_OPTIONS ), a11yLabel: RatioAndProportionStrings.zeroToTen }, - { value: 20, node: new RichText( this.tickMarkRangeMap[ 20 ], RANGE_TEXT_OPTIONS ), a11yLabel: RatioAndProportionStrings.zeroToTwenty }, - { value: 30, node: new RichText( this.tickMarkRangeMap[ 30 ], RANGE_TEXT_OPTIONS ), a11yLabel: RatioAndProportionStrings.zeroToThirty } + { value: 10, createNode: ( tandem: Tandem ) => new RichText( this.tickMarkRangeMap[ 10 ], RANGE_TEXT_OPTIONS ), a11yLabel: RatioAndProportionStrings.zeroToTen }, + { value: 20, createNode: ( tandem: Tandem ) => new RichText( this.tickMarkRangeMap[ 20 ], RANGE_TEXT_OPTIONS ), a11yLabel: RatioAndProportionStrings.zeroToTwenty }, + { value: 30, createNode: ( tandem: Tandem ) => new RichText( this.tickMarkRangeMap[ 30 ], RANGE_TEXT_OPTIONS ), a11yLabel: RatioAndProportionStrings.zeroToThirty } ]; - const widestItem = Math.max( ...items.map( item => item.node.width ) ); + // TODO: https://github.com/phetsims/sun/issues/797 is this OK? + const widestItem = Math.max( ...items.map( item => item.createNode( Tandem.OPT_OUT ).width ) ); const comboBoxOptions = { labelNode: new RichText( RatioAndProportionStrings.rangeStringProperty, RANGE_TEXT_OPTIONS ), @@ -73,7 +74,7 @@ // NOTE: The values are [ 10, true ]... so it's typed interestingly. this.disabledComboBox = new ComboBox( new BooleanProperty( value ) as Property, [ - { value: value, node: new HSeparatorDeprecated( widestItem, { centerY: -5 } ), a11yLabel: RatioAndProportionStrings.a11y.tickMark.tickMarksHidden }, + { value: value, createNode: tandem => new HSeparatorDeprecated( widestItem, { centerY: -5 } ), a11yLabel: RatioAndProportionStrings.a11y.tickMark.tickMarksHidden }, items[ 0 ] // add this one to get the proper height of the text. ], new Node(), comboBoxOptions ); Index: main/tambo/js/demo/testing/view/RemoveAndDisposeSoundGeneratorsTestPanel.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/tambo/js/demo/testing/view/RemoveAndDisposeSoundGeneratorsTestPanel.ts b/main/tambo/js/demo/testing/view/RemoveAndDisposeSoundGeneratorsTestPanel.ts --- a/main/tambo/js/demo/testing/view/RemoveAndDisposeSoundGeneratorsTestPanel.ts (revision 5fa44dd369d0e620cf872d4438cae456cdb81ade) +++ b/main/tambo/js/demo/testing/view/RemoveAndDisposeSoundGeneratorsTestPanel.ts (date 1665187567732) @@ -89,7 +89,7 @@ SOUND_GENERATOR_INFO.forEach( ( soundGenerator, soundGeneratorKey ) => { comboBoxItems.push( { value: soundGeneratorKey, - node: new Text( soundGenerator.comboBoxItemName, { font: COMBO_BOX_FONT } ) + createNode: tandem => new Text( soundGenerator.comboBoxItemName, { font: COMBO_BOX_FONT } ) } ); } ); const selectedSoundGeneratorTypeProperty = new Property( comboBoxItems[ 0 ].value ); Index: main/sun/js/CarouselComboBox.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/sun/js/CarouselComboBox.ts b/main/sun/js/CarouselComboBox.ts --- a/main/sun/js/CarouselComboBox.ts (revision 15be3e7ec7b6b9ad89936a0f6ecdbf2e23885ea9) +++ b/main/sun/js/CarouselComboBox.ts (date 1665188968090) @@ -33,6 +33,7 @@ import PageControl, { PageControlOptions } from './PageControl.js'; import sun from './sun.js'; import { ComboBoxItem } from './ComboBox.js'; +import { getGroupItemNodes } from './GroupItemOptions.js'; type SelfOptions = { @@ -113,9 +114,12 @@ // Make items in the carousel have the same width and height. const alignGroup = new AlignGroup(); + const nodes = getGroupItemNodes( comboBoxItems, options.tandem.createTandem( 'items' ) ); + // Create items for the carousel, whose API for 'items' is different than ComboBox. + let i = 0; const carouselItemNodes = _.map( comboBoxItems, - comboBoxItem => new CarouselItemNode( property, comboBoxItem, alignGroup, options.itemNodeOptions ) + comboBoxItem => new CarouselItemNode( property, comboBoxItem, nodes[ i++ ], alignGroup, options.itemNodeOptions ) ); assert && assert( carouselItemNodes.length === comboBoxItems.length, 'expected a carouselItem for each comboBoxItem' ); @@ -141,7 +145,7 @@ } ); // Pressing this button pops the carousel up and down - const button = new ComboBoxButton( property, comboBoxItems, combineOptions( { + const button = new ComboBoxButton( property, comboBoxItems, nodes, combineOptions( { listener: () => { carouselAndPageControl.visible = !carouselAndPageControl.visible; }, @@ -239,7 +243,7 @@ private readonly disposeCarouselItemNode: () => void; - public constructor( property: TProperty, comboBoxItem: ComboBoxItem, alignGroup: AlignGroup, providedOptions?: CarouselItemNodeOptions ) { + public constructor( property: TProperty, comboBoxItem: ComboBoxItem, node: Node, alignGroup: AlignGroup, providedOptions?: CarouselItemNodeOptions ) { const options = optionize()( { @@ -254,7 +258,7 @@ tandem: Tandem.OPTIONAL }, providedOptions ); - const uniformNode = new AlignBox( comboBoxItem.node, { + const uniformNode = new AlignBox( node, { xAlign: options.align, group: alignGroup } ); Index: main/sun/js/demo/components/demoComboBox.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/sun/js/demo/components/demoComboBox.ts b/main/sun/js/demo/components/demoComboBox.ts --- a/main/sun/js/demo/components/demoComboBox.ts (revision 15be3e7ec7b6b9ad89936a0f6ecdbf2e23885ea9) +++ b/main/sun/js/demo/components/demoComboBox.ts (date 1665187752290) @@ -20,7 +20,7 @@ values.forEach( value => { items.push( { value: value, - node: new Text( value, { font: FONT } ) + createNode: tandem => new Text( value, { font: FONT } ) } ); } ); Index: main/my-solar-system/js/common/view/MySolarSystemControls.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/my-solar-system/js/common/view/MySolarSystemControls.ts b/main/my-solar-system/js/common/view/MySolarSystemControls.ts --- a/main/my-solar-system/js/common/view/MySolarSystemControls.ts (revision a19f21b3cdbb060cc351967190427f2d3baa0085) +++ b/main/my-solar-system/js/common/view/MySolarSystemControls.ts (date 1665187507188) @@ -66,18 +66,18 @@ model.isLab ? [ new ComboBox( model.labModeProperty, [ - { value: LabModes.SUN_PLANET, node: new Text( MySolarSystemStrings.mode.sunAndPlanetStringProperty, COMBO_BOX_TEXT_OPTIONS ) }, - { value: LabModes.SUN_PLANET_MOON, node: new Text( MySolarSystemStrings.mode.sunPlanetAndMoonStringProperty, COMBO_BOX_TEXT_OPTIONS ) }, - { value: LabModes.SUN_PLANET_COMET, node: new Text( MySolarSystemStrings.mode.sunPlanetAndCometStringProperty, COMBO_BOX_TEXT_OPTIONS ) }, - { value: LabModes.TROJAN_ASTEROIDS, node: new Text( MySolarSystemStrings.mode.trojanAsteroidsStringProperty, COMBO_BOX_TEXT_OPTIONS ) }, - { value: LabModes.ELLIPSES, node: new Text( MySolarSystemStrings.mode.ellipsesStringProperty, COMBO_BOX_TEXT_OPTIONS ) }, - { value: LabModes.HYPERBOLIC, node: new Text( MySolarSystemStrings.mode.hyperbolicStringProperty, COMBO_BOX_TEXT_OPTIONS ) }, - { value: LabModes.SLINGSHOT, node: new Text( MySolarSystemStrings.mode.slingshotStringProperty, COMBO_BOX_TEXT_OPTIONS ) }, - { value: LabModes.DOUBLE_SLINGSHOT, node: new Text( MySolarSystemStrings.mode.doubleSlingshotStringProperty, COMBO_BOX_TEXT_OPTIONS ) }, - { value: LabModes.BINARY_STAR_PLANET, node: new Text( MySolarSystemStrings.mode.binaryStarPlanetStringProperty, COMBO_BOX_TEXT_OPTIONS ) }, - { value: LabModes.FOUR_STAR_BALLET, node: new Text( MySolarSystemStrings.mode.fourStarBalletStringProperty, COMBO_BOX_TEXT_OPTIONS ) }, - { value: LabModes.DOUBLE_DOUBLE, node: new Text( MySolarSystemStrings.mode.doubleDoubleStringProperty, COMBO_BOX_TEXT_OPTIONS ) }, - { value: LabModes.CUSTOM, node: new Text( MySolarSystemStrings.mode.customStringProperty, COMBO_BOX_TEXT_OPTIONS ) } + { value: LabModes.SUN_PLANET, createNode: tandem => new Text( MySolarSystemStrings.mode.sunAndPlanetStringProperty, COMBO_BOX_TEXT_OPTIONS ) }, + { value: LabModes.SUN_PLANET_MOON, createNode: tandem => new Text( MySolarSystemStrings.mode.sunPlanetAndMoonStringProperty, COMBO_BOX_TEXT_OPTIONS ) }, + { value: LabModes.SUN_PLANET_COMET, createNode: tandem => new Text( MySolarSystemStrings.mode.sunPlanetAndCometStringProperty, COMBO_BOX_TEXT_OPTIONS ) }, + { value: LabModes.TROJAN_ASTEROIDS, createNode: tandem => new Text( MySolarSystemStrings.mode.trojanAsteroidsStringProperty, COMBO_BOX_TEXT_OPTIONS ) }, + { value: LabModes.ELLIPSES, createNode: tandem => new Text( MySolarSystemStrings.mode.ellipsesStringProperty, COMBO_BOX_TEXT_OPTIONS ) }, + { value: LabModes.HYPERBOLIC, createNode: tandem => new Text( MySolarSystemStrings.mode.hyperbolicStringProperty, COMBO_BOX_TEXT_OPTIONS ) }, + { value: LabModes.SLINGSHOT, createNode: tandem => new Text( MySolarSystemStrings.mode.slingshotStringProperty, COMBO_BOX_TEXT_OPTIONS ) }, + { value: LabModes.DOUBLE_SLINGSHOT, createNode: tandem => new Text( MySolarSystemStrings.mode.doubleSlingshotStringProperty, COMBO_BOX_TEXT_OPTIONS ) }, + { value: LabModes.BINARY_STAR_PLANET, createNode: tandem => new Text( MySolarSystemStrings.mode.binaryStarPlanetStringProperty, COMBO_BOX_TEXT_OPTIONS ) }, + { value: LabModes.FOUR_STAR_BALLET, createNode: tandem => new Text( MySolarSystemStrings.mode.fourStarBalletStringProperty, COMBO_BOX_TEXT_OPTIONS ) }, + { value: LabModes.DOUBLE_DOUBLE, createNode: tandem => new Text( MySolarSystemStrings.mode.doubleDoubleStringProperty, COMBO_BOX_TEXT_OPTIONS ) }, + { value: LabModes.CUSTOM, createNode: tandem => new Text( MySolarSystemStrings.mode.customStringProperty, COMBO_BOX_TEXT_OPTIONS ) } ], topLayer, { tandem: providedOptions.tandem.createTandem( 'labModeComboBox' ) } ) Index: main/ratio-and-proportion/js/discover/view/ChallengeRatioComboBoxNode.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/ratio-and-proportion/js/discover/view/ChallengeRatioComboBoxNode.ts b/main/ratio-and-proportion/js/discover/view/ChallengeRatioComboBoxNode.ts --- a/main/ratio-and-proportion/js/discover/view/ChallengeRatioComboBoxNode.ts (revision 27b938fa83e1ed93c75233316dca102f5e97fe93) +++ b/main/ratio-and-proportion/js/discover/view/ChallengeRatioComboBoxNode.ts (date 1665187541033) @@ -149,7 +149,7 @@ return { value: targetRatio, - node: node, + createNode: tandem => node, soundPlayer: challengeInfo.soundClip, a11yLabel: challengeInfo.a11yLabel, tandemName: challengeInfo.tandemName ```

samreid commented 1 year ago

I'd like to work on https://github.com/phetsims/sun/issues/798 first, going on hold until then.

zepumph commented 1 year ago

Today I implicated myself in this issue. @samreid is likely going to be converting Carousel to use GroupItemOptions, and we may want to do this work as part of that work and/or BLL which uses combo box. This would be especially nice if we felt like PhET-iO api would change, but @samreid and I don't really think that it would.

samreid commented 1 year ago

Addressed in https://github.com/phetsims/sun/issues/814.

UPDATE: I thought this was a carousel issue. No progress on this committed yet.

zepumph commented 1 year ago

I'm taking a stab at continuing this from our momentum in Carousel. I have a good local copy and I'm interested in keeping backwards compatibility with item.node on master so we can phase out the old pattern instead of doing things all in one commit. It only took 2 lines of code to make that happen, and now I'm running pixel comparisons to ensure regressionless behavior. I'll keep you posted!

zepumph commented 1 year ago

Pixel comparison yielded no related changes except in the PDOM, the node trail IDs changed because the createNode pattern changing the order in which nodes are created in some cases. I also directly compared the visuals in a few sims and things look identical.

In general, I think this is a pretty safe change. There are a couple of usages dependent on the Node before we construct the combo box. I'd like to think about those cases with @samreid (marked by TODOs).

Here is an easy patch for testing comboBox disposal via frictions state wrapper in the localization preferences:

```diff Subject: [PATCH] Add support for GroupItemOptions in ComboBox, https://github.com/phetsims/sun/issues/797 --- Index: friction_en.html IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/friction_en.html b/friction_en.html --- a/friction_en.html (revision 56d8e713ea311670d3fece7c2dc89ca6175f503f) +++ b/friction_en.html (date 1673547441440) @@ -39,6 +39,9 @@ "phet", "phet-io" ], + "phetLibs": [ + "energy-skate-park" + ], "simFeatures": { "supportsVoicing": true, "supportsSound": true, @@ -59,6 +62,10 @@ window.phet.chipper.stringRepos = [ { + "repo": "energy-skate-park", + "requirejsNamespace": "ENERGY_SKATE_PARK" + }, + { "repo": "friction", "requirejsNamespace": "FRICTION" }, Index: js/friction-main.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/friction-main.js b/js/friction-main.js --- a/js/friction-main.js (revision 56d8e713ea311670d3fece7c2dc89ca6175f503f) +++ b/js/friction-main.js (date 1673547157221) @@ -15,8 +15,10 @@ import Bounds2 from '../../dot/js/Bounds2.js'; import Screen from '../../joist/js/Screen.js'; import Sim from '../../joist/js/Sim.js'; +import PreferencesModel from '../../joist/js/preferences/PreferencesModel.js'; import simLauncher from '../../joist/js/simLauncher.js'; import Tandem from '../../tandem/js/Tandem.js'; +import SkaterImages from '../../energy-skate-park/js/common/view/SkaterImages.js'; import FrictionModel from './friction/model/FrictionModel.js'; import FrictionKeyboardHelpContent from './friction/view/FrictionKeyboardHelpContent.js'; import FrictionScreenView from './friction/view/FrictionScreenView.js'; @@ -38,6 +40,11 @@ 'Kathryn Woessner', thanks: 'Thanks to Mobile Learner Labs for working with the PhET development team to convert this simulation to HTML5.' }, + preferencesModel: new PreferencesModel( { + localizationOptions: { + characterSets: SkaterImages.SKATER_CHARACTER_SETS + } + } ), phetioDesigned: true }; Index: README.md IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/README.md b/README.md --- a/README.md (revision 56d8e713ea311670d3fece7c2dc89ca6175f503f) +++ b/README.md (date 1673547440138) @@ -27,6 +27,7 @@ git clone https://github.com/phetsims/brand.git git clone https://github.com/phetsims/chipper.git git clone https://github.com/phetsims/dot.git +git clone https://github.com/phetsims/energy-skate-park.git git clone https://github.com/phetsims/friction.git git clone https://github.com/phetsims/joist.git git clone https://github.com/phetsims/kite.git Index: package.json IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/package.json b/package.json --- a/package.json (revision 56d8e713ea311670d3fece7c2dc89ca6175f503f) +++ b/package.json (date 1673547386354) @@ -17,6 +17,9 @@ "phet", "phet-io" ], + "phetLibs": [ + "energy-skate-park" + ], "simFeatures": { "supportsVoicing": true, "supportsSound": true, ```

[x] I think there is a pre-existing comboBox disposal bug that we should tackle as part of this issue.
[x] I don't agree with providing an unused tandem parameter everywhere. It feels unhelpful and like clutter until it is needed.
[x] 5 TODOs
[x] Many more sims to convert, but none in TypeScript. This allowed us to keep the changes from

Here is a nice way to find the remaining usages:

Subject: [PATCH] Add support for GroupItemOptions in ComboBox, https://github.com/phetsims/sun/issues/797
---
Index: js/ComboBox.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/ComboBox.ts b/js/ComboBox.ts
--- a/js/ComboBox.ts    (revision bd5a093e76c581dd7f8097b85a26f069cfefca12)
+++ b/js/ComboBox.ts    (date 1673553492658)
@@ -185,6 +185,10 @@
         `ComboBoxItem tandemName must end with '${ComboBox.ITEM_TANDEM_NAME_SUFFIX}': ${item.tandemName}` );
     } );

+    if ( items[ 0 ].node ) {
+      throw new Error();
+    }
+
     // See https://github.com/phetsims/sun/issues/542
     assert && assert( listParent.maxWidth === null,
       'ComboBox is responsible for scaling listBox. Setting maxWidth for listParent may result in buggy behavior.' );

zepumph commented 1 year ago

Today, while discussing https://github.com/phetsims/greenhouse-effect/issues/256 with @jbphet, he mentioned that he was confused about why the unused tandem parameter was part of the new createNode closure. I agree! I will go ahead remove those when I see them to clean things up.

zepumph commented 1 year ago

Removing unused tandem parameters made me realize that there is a bit of weirdness about how createNode provided a tandem. In many cases, the tandemName is used by a container of the createNode Node, and shouldn't be used. For example in https://github.com/phetsims/calculus-grapher/blob/b4483f1d83e27508b50bec00cf802d759a31019f/js/common/view/PredictModeRadioButtonGroup.ts#L55-L56. This makes things weird! Let's talk more.

jonathanolson commented 1 year ago

Fixed an area-model case. It's not using ComboBox, and something was still looking for .node

samreid commented 1 year ago

Tagging for https://github.com/phetsims/phet-io/issues/1914 and self-unassigning.

zepumph commented 1 year ago

I think of this issue as higher priority that a backlog. Currently both ways to support group items are in ComboBox. We should take steps to make sure that this is in a good medium-term state before unassigning.

samreid commented 1 year ago

This issue has had no work since Jan 17, and my subteam does not anticipate time to work on it in the coming week. How about scheduling it for the iteration beginning on March 9?

zepumph commented 1 year ago

Using this patch, I am not seeing any memory leak in combo box, but I am going to stop disposing nodes if we don't own them (due to the old, deprecated strategy).

```diff Subject: [PATCH] convert to typescript, https://github.com/phetsims/scenery/issues/1442 --- Index: js/demo/components/demoComboBox.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/demo/components/demoComboBox.ts b/js/demo/components/demoComboBox.ts --- a/js/demo/components/demoComboBox.ts (revision 04728f3aa5dfa39a1a9b289576f4294ce3c7e289) +++ b/js/demo/components/demoComboBox.ts (date 1681138166759) @@ -5,7 +5,6 @@ */ import ComboBox, { ComboBoxItem } from '../../ComboBox.js'; -import Checkbox from '../../Checkbox.js'; import Bounds2 from '../../../../dot/js/Bounds2.js'; import { Font, Node, Text, VBox } from '../../../../scenery/js/imports.js'; import Property from '../../../../axon/js/Property.js'; @@ -30,16 +29,24 @@ const enabledProperty = new BooleanProperty( true ); - const comboBox = new ComboBox( selectedItemProperty, items, listParent, { - highlightFill: 'yellow', - listPosition: 'above', - enabledProperty: enabledProperty - } ); + for ( let i = 0; i < 1000; i++ ) { + console.log( 'new on' ); + new ComboBox( selectedItemProperty, items, listParent, { + highlightFill: 'yellow', + listPosition: 'above', + enabledProperty: enabledProperty + } ).dispose(); + } + // const comboBox = new ComboBox( selectedItemProperty, items, listParent, { + // highlightFill: 'yellow', + // listPosition: 'above', + // enabledProperty: enabledProperty + // } ); - const enabledCheckbox = new Checkbox( enabledProperty, new Text( 'enabled', { font: FONT } ) ); + // const enabledCheckbox = new Checkbox( enabledProperty, new Text( 'enabled', { font: FONT } ) ); const uiComponents = new VBox( { - children: [ comboBox, enabledCheckbox ], + // children: [ comboBox, enabledCheckbox ], spacing: 40, center: layoutBounds.center } );

zepumph commented 1 year ago

Good progress here, I was able to get all of the node usages out of comboBox, and remove the workaround code in ComboBox. I'll check back on CT in a couple hours.

zepumph commented 1 year ago

Oops, I had stashed the ESP changes for a snapshot comparison, here they are now.

zepumph commented 1 year ago

CT is looking mighty clean today. @samreid will you please take a look at the usages of group item options in ComboBox, and the commits in CarouselComboBox, as well as spot checking some of the usages. I can't think of much else here.

samreid commented 1 year ago

I reviewed the code in ComboBox and CarouselComboBox, and checked some usages. I tested dispose in BLL and it worked ok. I also saw this commented out assertion, I'm guessing you will want to bring it back until we have better TypeScript coverage? Or were you testing that in local aqua?

// assert && assert( !item.hasOwnProperty( 'node' ), 'we use createNode now' );

zepumph commented 1 year ago

Ahh yes thanks. This is redundant to https://github.com/phetsims/sun/blob/cb5784eaf0e86d0039aa83e722dd92a24fbe818a/js/GroupItemOptions.ts#L31.

Closing

phetsims / sun

Use GroupItemOptions for ComboBox #797