don't allow decoration for many UI components, see #860

phetsims · Aug 19, 2024 · 0248f98 · 0248f98
1 parent 5d1c6a5
commit 0248f98
Show file tree

Hide file tree

Showing 10 changed files with 40 additions and 10 deletions.
diff --git a/js/AccordionBox.ts b/js/AccordionBox.ts
@@ -13,7 +13,7 @@ import { Shape } from '../../kite/js/imports.js';
 import InstanceRegistry from '../../phet-core/js/documentation/InstanceRegistry.js';
 import optionize, { combineOptions } from '../../phet-core/js/optionize.js';
 import StrictOmit from '../../phet-core/js/types/StrictOmit.js';
-import { HighlightFromNode, InteractiveHighlighting, isHeightSizable, isWidthSizable, LayoutConstraint, Node, NodeOptions, PaintableOptions, Path, PathOptions, PDOMBehaviorFunction, PDOMPeer, Rectangle, RectangleOptions, Sizable, Text } from '../../scenery/js/imports.js';
+import { assertNoAdditionalChildren, HighlightFromNode, InteractiveHighlighting, isHeightSizable, isWidthSizable, LayoutConstraint, Node, NodeOptions, PaintableOptions, Path, PathOptions, PDOMBehaviorFunction, PDOMPeer, Rectangle, RectangleOptions, Sizable, Text } from '../../scenery/js/imports.js';
 import EventType from '../../tandem/js/EventType.js';
 import Tandem from '../../tandem/js/Tandem.js';
 import IOType from '../../tandem/js/types/IOType.js';
@@ -452,6 +452,9 @@ export default class AccordionBox extends Sizable( Node ) {
 
     // support for binder documentation, stripped out in builds and only runs when ?binder is specified
     assert && phet?.chipper?.queryParameters?.binder && InstanceRegistry.registerDataURL( 'sun', 'AccordionBox', this );
+
+    // Decorating with additional content is an anti-pattern, see https://github.com/phetsims/sun/issues/860
+    assert && assertNoAdditionalChildren( this );
   }
 
   /**

diff --git a/js/AquaRadioButton.ts b/js/AquaRadioButton.ts
@@ -11,7 +11,7 @@ import TProperty from '../../axon/js/TProperty.js';
 import Emitter from '../../axon/js/Emitter.js';
 import InstanceRegistry from '../../phet-core/js/documentation/InstanceRegistry.js';
 import optionize from '../../phet-core/js/optionize.js';
-import { Circle, FireListener, isWidthSizable, LayoutConstraint, Node, NodeOptions, Rectangle, SceneryConstants, TPaint, Voicing, VoicingOptions, WidthSizable } from '../../scenery/js/imports.js';
+import { assertNoAdditionalChildren, Circle, FireListener, isWidthSizable, LayoutConstraint, Node, NodeOptions, Rectangle, SceneryConstants, TPaint, Voicing, VoicingOptions, WidthSizable } from '../../scenery/js/imports.js';
 import TSoundPlayer from '../../tambo/js/TSoundPlayer.js';
 import multiSelectionSoundPlayerFactory from '../../tambo/js/multiSelectionSoundPlayerFactory.js';
 import Tandem from '../../tandem/js/Tandem.js';
@@ -239,6 +239,9 @@ export default class AquaRadioButton<T> extends WidthSizable( Voicing( Node ) )
       fireListener.dispose();
     };
 
+    // Decorating with additional content is an anti-pattern, see https://github.com/phetsims/sun/issues/860
+    assert && assertNoAdditionalChildren( this );
+
     // support for binder documentation, stripped out in builds and only runs when ?binder is specified
     assert && phet?.chipper?.queryParameters?.binder && InstanceRegistry.registerDataURL( 'sun', 'AquaRadioButton', this );
   }

diff --git a/js/AquaRadioButtonGroup.ts b/js/AquaRadioButtonGroup.ts
@@ -9,7 +9,7 @@
 
 import StrictOmit from '../../phet-core/js/types/StrictOmit.js';
 import optionize, { combineOptions } from '../../phet-core/js/optionize.js';
-import { FlowBox, FlowBoxOptions, KeyboardUtils, PDOMPeer, SceneryConstants, SceneryEvent } from '../../scenery/js/imports.js';
+import { assertNoAdditionalChildren, FlowBox, FlowBoxOptions, KeyboardUtils, PDOMPeer, SceneryConstants, SceneryEvent } from '../../scenery/js/imports.js';
 import multiSelectionSoundPlayerFactory from '../../tambo/js/multiSelectionSoundPlayerFactory.js';
 import Tandem from '../../tandem/js/Tandem.js';
 import AquaRadioButton, { AquaRadioButtonOptions } from './AquaRadioButton.js';
@@ -165,6 +165,9 @@ export default class AquaRadioButtonGroup<T> extends FlowBox {
     };
 
     this.radioButtons = radioButtons;
+
+    // Decorating with additional content is an anti-pattern, see https://github.com/phetsims/sun/issues/860
+    assert && assertNoAdditionalChildren( this );
   }
 
   private onRadioButtonInput(): void {

diff --git a/js/Carousel.ts b/js/Carousel.ts
@@ -28,7 +28,7 @@ import Range from '../../dot/js/Range.js';
 import { Shape } from '../../kite/js/imports.js';
 import InstanceRegistry from '../../phet-core/js/documentation/InstanceRegistry.js';
 import optionize, { combineOptions } from '../../phet-core/js/optionize.js';
-import { AlignBox, AlignBoxOptions, AlignGroup, FlowBox, FlowBoxOptions, IndexedNodeIO, IndexedNodeIOParent, LayoutConstraint, LayoutOrientation, Node, NodeOptions, Rectangle, Separator, SeparatorOptions, TPaint } from '../../scenery/js/imports.js';
+import { AlignBox, AlignBoxOptions, AlignGroup, assertNoAdditionalChildren, FlowBox, FlowBoxOptions, IndexedNodeIO, IndexedNodeIOParent, LayoutConstraint, LayoutOrientation, Node, NodeOptions, Rectangle, Separator, SeparatorOptions, TPaint } from '../../scenery/js/imports.js';
 import Tandem from '../../tandem/js/Tandem.js';
 import Animation, { AnimationOptions } from '../../twixt/js/Animation.js';
 import Easing from '../../twixt/js/Easing.js';
@@ -429,6 +429,9 @@ export default class Carousel extends Node {
 
     this.mutate( options );
 
+    // Decorating with additional content is an anti-pattern, see https://github.com/phetsims/sun/issues/860
+    assert && assertNoAdditionalChildren( this );
+
     // Will allow potential animation after this
     isInitialized = true;
 

diff --git a/js/Checkbox.ts b/js/Checkbox.ts
@@ -10,7 +10,7 @@ import PhetioAction from '../../tandem/js/PhetioAction.js';
 import validate from '../../axon/js/validate.js';
 import { m3 } from '../../dot/js/Matrix3.js';
 import InstanceRegistry from '../../phet-core/js/documentation/InstanceRegistry.js';
-import { FireListener, LayoutConstraint, Node, NodeOptions, Path, Rectangle, SceneryConstants, TPaint, Voicing, VoicingOptions, WidthSizable, WidthSizableOptions } from '../../scenery/js/imports.js';
+import { assertNoAdditionalChildren, FireListener, LayoutConstraint, Node, NodeOptions, Path, Rectangle, SceneryConstants, TPaint, Voicing, VoicingOptions, WidthSizable, WidthSizableOptions } from '../../scenery/js/imports.js';
 import checkEmptySolidShape from '../../sherpa/js/fontawesome-4/checkEmptySolidShape.js';
 import checkSquareOSolidShape from '../../sherpa/js/fontawesome-4/checkSquareOSolidShape.js';
 import EventType from '../../tandem/js/EventType.js';
@@ -232,6 +232,9 @@ export default class Checkbox extends WidthSizable( Voicing( Node ) ) {
     // support for binder documentation, stripped out in builds and only runs when ?binder is specified
     assert && phet?.chipper?.queryParameters?.binder && InstanceRegistry.registerDataURL( 'sun', 'Checkbox', this );
 
+    // Decorating Checkbox with additional content is an anti-pattern, see https://github.com/phetsims/sun/issues/860
+    assert && assertNoAdditionalChildren( this );
+
     this.disposeCheckbox = () => {
       rectangle.dispose();
       this.backgroundNode.dispose();

diff --git a/js/Dialog.ts b/js/Dialog.ts
@@ -17,7 +17,7 @@ import getGlobal from '../../phet-core/js/getGlobal.js';
 import optionize from '../../phet-core/js/optionize.js';
 import StrictOmit from '../../phet-core/js/types/StrictOmit.js';
 import CloseButton from '../../scenery-phet/js/buttons/CloseButton.js';
-import { AlignBox, FocusManager, FullScreen, HBox, KeyboardListener, Node, PDOMPeer, PDOMUtils, TColor, VBox, voicingManager } from '../../scenery/js/imports.js';
+import { AlignBox, assertNoAdditionalChildren, FocusManager, FullScreen, HBox, KeyboardListener, Node, PDOMPeer, PDOMUtils, TColor, VBox, voicingManager } from '../../scenery/js/imports.js';
 import TSoundPlayer from '../../tambo/js/TSoundPlayer.js';
 import nullSoundPlayer from '../../tambo/js/nullSoundPlayer.js';
 import Tandem, { DYNAMIC_ARCHETYPE_NAME } from '../../tandem/js/Tandem.js';
@@ -439,6 +439,9 @@ export default class Dialog extends Popupable( Panel, 1 ) {
       dialogContent.removeAllChildren();
       dialogContent.detach();
     };
+
+    // Set content through the constructor, Dialog does not support decorating with additional content
+    assert && assertNoAdditionalChildren( this );
   }
 
   public override dispose(): void {

diff --git a/js/Panel.ts b/js/Panel.ts
@@ -10,7 +10,7 @@
  */
 
 import { optionize3, OptionizeDefaults } from '../../phet-core/js/optionize.js';
-import { LayoutConstraint, Node, NodeOptions, Rectangle, Sizable, SizableOptions, TPaint } from '../../scenery/js/imports.js';
+import { assertNoAdditionalChildren, LayoutConstraint, Node, NodeOptions, Rectangle, Sizable, SizableOptions, TPaint } from '../../scenery/js/imports.js';
 import sun from './sun.js';
 
 // valid values for options.align
@@ -113,6 +113,9 @@ export default class Panel extends Sizable( Node ) {
 
     // Apply options after the layout is done, so that options that use the bounds will work properly.
     this.mutate( options );
+
+    // Decorating with additional content is an anti-pattern, see https://github.com/phetsims/sun/issues/860
+    assert && assertNoAdditionalChildren( this );
   }
 
   /**

diff --git a/js/Slider.ts b/js/Slider.ts
@@ -22,7 +22,7 @@ import InstanceRegistry from '../../phet-core/js/documentation/InstanceRegistry.
 import optionize, { combineOptions } from '../../phet-core/js/optionize.js';
 import Orientation from '../../phet-core/js/Orientation.js';
 import swapObjectKeys from '../../phet-core/js/swapObjectKeys.js';
-import { DragListener, HighlightFromNode, LayoutConstraint, Node, NodeOptions, SceneryConstants, Sizable, TPaint } from '../../scenery/js/imports.js';
+import { assertNoAdditionalChildren, DragListener, HighlightFromNode, LayoutConstraint, Node, NodeOptions, SceneryConstants, Sizable, TPaint } from '../../scenery/js/imports.js';
 import Tandem from '../../tandem/js/Tandem.js';
 import IOType from '../../tandem/js/types/IOType.js';
 import ValueChangeSoundPlayer, { ValueChangeSoundPlayerOptions } from '../../tambo/js/sound-generators/ValueChangeSoundPlayer.js';
@@ -515,6 +515,9 @@ export default class Slider extends Sizable( AccessibleSlider( Node, 0 ) ) {
 
     this.mutate( boundsRequiredOptionKeys );
 
+    // Decorating the with additional content is bad for dynamic layout, see https://github.com/phetsims/sun/issues/860
+    assert && assertNoAdditionalChildren( this );
+
     // support for binder documentation, stripped out in builds and only runs when ?binder is specified
     assert && phet?.chipper?.queryParameters?.binder && InstanceRegistry.registerDataURL( 'sun', 'Slider', this );
   }

diff --git a/js/buttons/ButtonNode.ts b/js/buttons/ButtonNode.ts
@@ -15,7 +15,7 @@ import Bounds2 from '../../../dot/js/Bounds2.js';
 import Dimension2 from '../../../dot/js/Dimension2.js';
 import optionize, { combineOptions } from '../../../phet-core/js/optionize.js';
 import StrictOmit from '../../../phet-core/js/types/StrictOmit.js';
-import { AlignBox, AlignBoxXAlign, AlignBoxYAlign, Brightness, Color, Contrast, Grayscale, Node, NodeOptions, PaintableNode, PaintColorProperty, Path, PressListener, PressListenerOptions, SceneryConstants, Sizable, SizableOptions, TColor, TPaint, Voicing, VoicingOptions } from '../../../scenery/js/imports.js';
+import { AlignBox, AlignBoxXAlign, AlignBoxYAlign, assertNoAdditionalChildren, Brightness, Color, Contrast, Grayscale, Node, NodeOptions, PaintableNode, PaintColorProperty, Path, PressListener, PressListenerOptions, SceneryConstants, Sizable, SizableOptions, TColor, TPaint, Voicing, VoicingOptions } from '../../../scenery/js/imports.js';
 import ColorConstants from '../ColorConstants.js';
 import sun from '../sun.js';
 import ButtonInteractionState from './ButtonInteractionState.js';
@@ -238,6 +238,9 @@ export default class ButtonNode extends Sizable( Voicing( Node ) ) {
     // No need to dispose because enabledProperty is disposed in Node
     this.enabledProperty.link( enabled => options.enabledAppearanceStrategy( enabled, this, buttonBackground, alignBox ) );
 
+    // Decorating with additional content is an anti-pattern, see https://github.com/phetsims/sun/issues/860
+    assert && assertNoAdditionalChildren( this );
+
     this.disposeButtonNode = () => {
       alignBox && alignBox.dispose();
       updateAlignBounds && updateAlignBounds.dispose();

diff --git a/js/buttons/RectangularRadioButton.ts b/js/buttons/RectangularRadioButton.ts
@@ -12,7 +12,7 @@ import TEmitter from '../../../axon/js/TEmitter.js';
 import TProperty from '../../../axon/js/TProperty.js';
 import TReadOnlyProperty from '../../../axon/js/TReadOnlyProperty.js';
 import optionize from '../../../phet-core/js/optionize.js';
-import { Color, Node, PaintableNode, PaintColorProperty } from '../../../scenery/js/imports.js';
+import { assertNoAdditionalChildren, Color, Node, PaintableNode, PaintColorProperty } from '../../../scenery/js/imports.js';
 import TSoundPlayer from '../../../tambo/js/TSoundPlayer.js';
 import EventType from '../../../tandem/js/EventType.js';
 import PhetioObject from '../../../tandem/js/PhetioObject.js';
@@ -167,6 +167,9 @@ export default class RectangularRadioButton<T> extends RectangularButton {
       buttonModel.dispose();
       this.interactionStateProperty.dispose();
     };
+
+    // Adding children to UI components with content is problematic for dynamic layout.
+    assert && assertNoAdditionalChildren( this );
   }
 
   public override dispose(): void {