Moving wireToolNode1 to page 2 of carousel prevents Standard PhET-iO Wrapper from loading

[Nancy-Salpepi]

Test device MacBook Air M1 chip

Operating System 13.1

Browser Safari and Chrome

Problem description For https://github.com/phetsims/qa/issues/900, if wireToolNode1 is moved to page 2 of the carousel and the sim is launched, it will not open and Studio will freeze. No errors appear in the console.

Steps to reproduce Method 1:

1. On the Intro Screen, go to circuitConstructionKitDc.introScreen.view.circuitElementToolbox.carousel.items.wireToolNode1 and move it down to position 5.
2. Press Launch button.

Method 2:

1. On the Intro Screen, move other items up until the position of wireToolNode1 moves to page 2.
2. Press Launch button.

Visuals

https://user-images.githubusercontent.com/87318828/219674845-c26f37c4-bab9-4344-be60-2f366c490806.mp4

[samreid]

I am able to reproduce the bug, and I can even see it when moving wire1 to index 4 (same page).

[samreid]

In IndexedNodeIO, when calling node.addChild() on that wireNode is causing an infinite loop sort of symptom.

Subject: [PATCH] Add documentation, see https://github.com/phetsims/chipper/issues/1369
---
Index: js/nodes/IndexedNodeIO.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/nodes/IndexedNodeIO.ts b/js/nodes/IndexedNodeIO.ts
--- a/js/nodes/IndexedNodeIO.ts (revision 772149d66d6ad30f80ece0ade67b0b277b956421)
+++ b/js/nodes/IndexedNodeIO.ts (date 1676648345783)
@@ -83,7 +83,15 @@
       const currentIndex = nodeParent.indexOfChild( node );
       children[ currentIndex ] = children[ stateObject.index ];
       children[ stateObject.index ] = node;
-      nodeParent.setChildren( children );
+      console.log( 'setting children: ', children );
+      nodeParent.children = [];
+      children.forEach( ( child, i ) => {
+        console.log( 'start ' + i );
+        nodeParent.addChild( child );
+        console.log( 'end ' + i );
+      } );
+      // nodeParent.setChildren( children );
+      console.log( 'done' );
     }
   },
   stateSchema: {

[samreid]

Commenting out this line of code avoids the problem, so there is some sort of cycle or problem in the carousel ManualConstraint layout:

Subject: [PATCH] Scroll to an item when its position in the carousel changes, see https://github.com/phetsims/studio/issues/296
---
Index: js/Carousel.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/Carousel.ts b/js/Carousel.ts
--- a/js/Carousel.ts    (revision 13065d19035b2e48279d4403bf074b0bb745964c)
+++ b/js/Carousel.ts    (date 1676734648476)
@@ -420,7 +420,7 @@
       windowProxy[ orientation.opposite.centerCoordinate ] = backgroundProxy[ orientation.opposite.centerCoordinate ];
       previousProxy[ orientation.minSide ] = backgroundProxy[ orientation.minSide ];
       nextProxy[ orientation.maxSide ] = backgroundProxy[ orientation.maxSide ];
-      windowProxy[ orientation.centerCoordinate ] = backgroundProxy[ orientation.centerCoordinate ];
+      // windowProxy[ orientation.centerCoordinate ] = backgroundProxy[ orientation.centerCoordinate ];
     } );

     // Handle changing pages (or if the content changes)

[samreid]

This experiment shows there is an infinite loop in the ManualConstraint:

Subject: [PATCH] Scroll to an item when its position in the carousel changes, see https://github.com/phetsims/studio/issues/296
---
Index: js/Carousel.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/Carousel.ts b/js/Carousel.ts
--- a/js/Carousel.ts    (revision 13065d19035b2e48279d4403bf074b0bb745964c)
+++ b/js/Carousel.ts    (date 1676734964725)
@@ -413,14 +413,22 @@
       foregroundNode.rectBounds = bounds;
     } );

+    let depth = 0;
     // Top-level layout (based on background changes)
     ManualConstraint.create( this, [ backgroundNode, windowNode, previousButton, nextButton ], ( backgroundProxy, windowProxy, previousProxy, nextProxy ) => {
-      nextProxy[ orientation.opposite.centerCoordinate ] = backgroundProxy[ orientation.opposite.centerCoordinate ];
-      previousProxy[ orientation.opposite.centerCoordinate ] = backgroundProxy[ orientation.opposite.centerCoordinate ];
-      windowProxy[ orientation.opposite.centerCoordinate ] = backgroundProxy[ orientation.opposite.centerCoordinate ];
-      previousProxy[ orientation.minSide ] = backgroundProxy[ orientation.minSide ];
-      nextProxy[ orientation.maxSide ] = backgroundProxy[ orientation.maxSide ];
-      windowProxy[ orientation.centerCoordinate ] = backgroundProxy[ orientation.centerCoordinate ];
+
+      depth++;
+      console.log( depth );
+      if ( depth < 5 ) {
+        nextProxy[ orientation.opposite.centerCoordinate ] = backgroundProxy[ orientation.opposite.centerCoordinate ];
+        previousProxy[ orientation.opposite.centerCoordinate ] = backgroundProxy[ orientation.opposite.centerCoordinate ];
+        windowProxy[ orientation.opposite.centerCoordinate ] = backgroundProxy[ orientation.opposite.centerCoordinate ];
+        previousProxy[ orientation.minSide ] = backgroundProxy[ orientation.minSide ];
+        nextProxy[ orientation.maxSide ] = backgroundProxy[ orientation.maxSide ];
+        windowProxy[ orientation.centerCoordinate ] = backgroundProxy[ orientation.centerCoordinate ];
+      }
+
+      depth--;
     } );

     // Handle changing pages (or if the content changes)

@jonathanolson and/or @marlitas may have helpful expertise here.

[samreid]

This also short circuits the infinite loop:

Subject: [PATCH] Scroll to an item when its position in the carousel changes, see https://github.com/phetsims/studio/issues/296
---
Index: js/Carousel.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/Carousel.ts b/js/Carousel.ts
--- a/js/Carousel.ts    (revision 13065d19035b2e48279d4403bf074b0bb745964c)
+++ b/js/Carousel.ts    (date 1676736789473)
@@ -360,7 +360,7 @@

       // Specify the local bounds in order to ensure centering. For full pages, this is not necessary since the scrollingNodeContainer
       // already spans the full area. But for a partial page, this is necessary so the window will be centered.
-      windowNode.localBounds = bounds;
+      // windowNode.localBounds = bounds;
     } );

     // Background - displays the carousel's fill color

[samreid]

This workaround works and shows a correct-looking layout, but hopefully we can find a better solution:

Subject: [PATCH] Scroll to an item when its position in the carousel changes, see https://github.com/phetsims/studio/issues/296
---
Index: js/Carousel.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/Carousel.ts b/js/Carousel.ts
--- a/js/Carousel.ts    (revision 13065d19035b2e48279d4403bf074b0bb745964c)
+++ b/js/Carousel.ts    (date 1676737107949)
@@ -360,7 +360,9 @@

       // Specify the local bounds in order to ensure centering. For full pages, this is not necessary since the scrollingNodeContainer
       // already spans the full area. But for a partial page, this is necessary so the window will be centered.
-      windowNode.localBounds = bounds;
+      setTimeout( () => {
+        windowNode.localBounds = bounds;
+      }, 0 );
     } );

     // Background - displays the carousel's fill color

[matthew-blackman]

@samreid feels that this issue warrants collaboration. We will circle back with @jonathanolson and @marlitas to set up some collaborative time to work on this.

[samreid]

Current patch from collaboration with @marlitas @zepumph @matthew-blackman and me:

```diff Subject: [PATCH] Factor out includeExtremeElements, see https://github.com/phetsims/circuit-construction-kit-common/issues/900 --- Index: js/Carousel.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/Carousel.ts b/js/Carousel.ts --- a/js/Carousel.ts (revision ec37fb13198d2d4e979feace8590add2299d80a3) +++ b/js/Carousel.ts (date 1676934064762) @@ -22,7 +22,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 { AlignBox, AlignBoxOptions, AlignGroup, FlowBox, IndexedNodeIO, LayoutOrientation, ManualConstraint, Node, NodeOptions, Rectangle, Separator, SeparatorOptions, TPaint } from '../../scenery/js/imports.js'; +import { AlignBox, AlignBoxOptions, AlignGroup, FlowBox, IndexedNodeIO, LayoutConstraint, LayoutOrientation, Node, NodeOptions, Rectangle, Separator, SeparatorOptions, TPaint } from '../../scenery/js/imports.js'; import pushButtonSoundPlayer from '../../tambo/js/shared-sound-players/pushButtonSoundPlayer.js'; import Tandem from '../../tandem/js/Tandem.js'; import Animation, { AnimationOptions } from '../../twixt/js/Animation.js'; @@ -36,6 +36,9 @@ import Orientation from '../../phet-core/js/Orientation.js'; import Multilink from '../../axon/js/Multilink.js'; import Bounds2 from '../../dot/js/Bounds2.js'; +import ButtonNode from './buttons/ButtonNode.js'; + +// TODO: more Layout work? const DEFAULT_ARROW_SIZE = new Dimension2( 20, 7 ); @@ -109,6 +112,7 @@ private readonly disposeCarousel: () => void; private readonly scrollingNode: FlowBox; + private readonly carouselConstraint: CarouselConstraint; /** * NOTE: This will dispose the item Nodes when the carousel is disposed @@ -201,10 +205,7 @@ this.itemsPerPage = options.itemsPerPage; this.defaultPageNumber = options.defaultPageNumber; - // To improve readability - const isHorizontal = ( options.orientation === 'horizontal' ); const orientation = Orientation.fromLayoutOrientation( options.orientation ); - const alignGroup = new AlignGroup(); const itemsTandem = options.tandem.createTandem( 'items' ); @@ -238,9 +239,7 @@ } ); // When the AlignBoxes are reordered, we need to recompute the visibleAlignBoxesProperty - this.scrollingNode.childrenReorderedEmitter.addListener( () => { - this.visibleAlignBoxesProperty.recomputeDerivation(); - } ); + this.scrollingNode.childrenReorderedEmitter.addListener( () => this.visibleAlignBoxesProperty.recomputeDerivation() ); // Options common to both buttons const buttonOptions = combineOptions( { @@ -295,7 +294,7 @@ // Next button const nextButton = new CarouselButton( combineOptions( { - arrowDirection: isHorizontal ? 'right' : 'down', + arrowDirection: orientation === Orientation.HORIZONTAL ? 'right' : 'down', tandem: options.tandem.createTandem( 'nextButton' ), listener: () => this.pageNumberProperty.set( this.pageNumberProperty.get() + 1 ), enabledProperty: new DerivedProperty( [ this.pageNumberProperty, this.numberOfPagesProperty ], ( pageNumber, numberofPages ) => { @@ -306,7 +305,7 @@ // Previous button const previousButton = new CarouselButton( combineOptions( { - arrowDirection: isHorizontal ? 'left' : 'up', + arrowDirection: orientation === Orientation.HORIZONTAL ? 'left' : 'up', tandem: options.tandem.createTandem( 'previousButton' ), listener: () => this.pageNumberProperty.set( this.pageNumberProperty.get() - 1 ), enabledProperty: new DerivedProperty( [ this.pageNumberProperty ], pageNumber => { @@ -315,53 +314,8 @@ visibleProperty: buttonsVisibleProperty }, buttonOptions ) ); - // Resize next/previous buttons dynamically - alignGroup.getMaxSizeProperty( orientation.opposite ).link( maxOppositeSize => { - const buttonOppositeSize = maxOppositeSize + ( 2 * options.margin ); - nextButton[ orientation.opposite.preferredSize ] = buttonOppositeSize; - previousButton[ orientation.opposite.preferredSize ] = buttonOppositeSize; - } ); - - // Window size (content + margins, DOES NOT include the buttons) - const windowSizeProperty = DerivedProperty.deriveAny( [ - this.visibleAlignBoxesProperty, - scrollingNodeContainer.boundsProperty, - ...this.alignBoxes.map( alignBox => alignBox.boundsProperty ) - ], () => { - const visibleAlignBoxes = this.visibleAlignBoxesProperty.value; - - if ( visibleAlignBoxes.length === 0 ) { - return new Dimension2( 0, 0 ); - } - else { - - // This doesn't fill one page in number play preferences dialog when you forget locales=*, - // so take the last item, even if it is not a full page - const lastBox = visibleAlignBoxes[ options.itemsPerPage - 1 ] || visibleAlignBoxes[ visibleAlignBoxes.length - 1 ]; - - const horizontalSize = new Dimension2( - // Measure from the beginning of the first item to the end of the last item on the 1st page - lastBox[ orientation.maxSide ] - visibleAlignBoxes[ 0 ][ orientation.minSide ] + ( 2 * options.margin ), - - scrollingNodeContainer.boundsProperty.value[ orientation.opposite.size ] - ); - return isHorizontal ? horizontalSize : horizontalSize.swapped(); - } - }, { - // So we don't needlessly toggle window sizes - valueComparisonStrategy: 'equalsFunction' - } ); - // Window with clipping area, so that the scrollingNodeContainer can be scrolled const windowNode = new Node( { children: [ scrollingNodeContainer ] } ); - windowSizeProperty.link( windowSize => { - const bounds = windowSize.toBounds(); - windowNode.clipArea = Shape.bounds( bounds ); - - // Specify the local bounds in order to ensure centering. For full pages, this is not necessary since the scrollingNodeContainer - // already spans the full area. But for a partial page, this is necessary so the window will be centered. - windowNode.localBounds = bounds; - } ); // Background - displays the carousel's fill color const backgroundNode = new Rectangle( { @@ -377,51 +331,13 @@ pickable: false } ); - // Background size - includes the buttons, if they are visible. - const backgroundSizeProperty = new DerivedProperty( - [ windowSizeProperty, nextButton.visibleProperty, previousButton.visibleProperty ], - ( windowSize, nextButtonVisible, previousButtonVisible ) => { - let backgroundWidth; - let backgroundHeight; - if ( isHorizontal ) { - - // For horizontal orientation, buttons contribute to width, if they are visible. - const nextButtonWidth = nextButtonVisible ? nextButton.width : 0; - const previousButtonWidth = previousButtonVisible ? previousButton.width : 0; - backgroundWidth = windowSize.width + nextButtonWidth + previousButtonWidth; - backgroundHeight = windowSize.height; - } - else { - - // For vertical orientation, buttons contribute to height, if they are visible. - const nextButtonHeight = nextButtonVisible ? nextButton.height : 0; - const previousButtonHeight = previousButtonVisible ? previousButton.height : 0; - backgroundWidth = windowSize.width; - backgroundHeight = windowSize.height + nextButtonHeight + previousButtonHeight; - } - return new Dimension2( backgroundWidth, backgroundHeight ); - } ); - - - // Resize the background/foreground dynamically - backgroundSizeProperty.link( backgroundSize => { - this.backgroundWidth = backgroundSize.width; - this.backgroundHeight = backgroundSize.height; - - const bounds = backgroundSize.toBounds(); - backgroundNode.rectBounds = bounds; - foregroundNode.rectBounds = bounds; - } ); - - // Top-level layout (based on background changes) - ManualConstraint.create( this, [ backgroundNode, windowNode, previousButton, nextButton ], ( backgroundProxy, windowProxy, previousProxy, nextProxy ) => { - nextProxy[ orientation.opposite.centerCoordinate ] = backgroundProxy[ orientation.opposite.centerCoordinate ]; - previousProxy[ orientation.opposite.centerCoordinate ] = backgroundProxy[ orientation.opposite.centerCoordinate ]; - windowProxy[ orientation.opposite.centerCoordinate ] = backgroundProxy[ orientation.opposite.centerCoordinate ]; - previousProxy[ orientation.minSide ] = backgroundProxy[ orientation.minSide ]; - nextProxy[ orientation.maxSide ] = backgroundProxy[ orientation.maxSide ]; - windowProxy[ orientation.centerCoordinate ] = backgroundProxy[ orientation.centerCoordinate ]; - } ); + // Top-level layout (based on background changes). + this.carouselConstraint = new CarouselConstraint( this, backgroundNode, foregroundNode, windowNode, previousButton, nextButton, scrollingNodeContainer, this.alignBoxes, + orientation, + this.scrollingNode, + this.itemsPerPage, + options.margin, + alignGroup ); // Handle changing pages (or if the content changes) let scrollAnimation: Animation | null = null; @@ -515,6 +431,8 @@ alignBox.children.forEach( child => child.dispose() ); alignBox.dispose(); } ); + + this.carouselConstraint.dispose(); }; this.mutate( options ); @@ -603,4 +521,113 @@ } } +class CarouselConstraint extends LayoutConstraint { + public constructor( + private readonly carousel: Carousel, + private readonly backgroundNode: Rectangle, + private readonly foregroundNode: Rectangle, + private readonly windowNode: Node, + private readonly previousButton: ButtonNode, + private readonly nextButton: ButtonNode, + private readonly scrollingNodeContainer: Node, + private readonly alignBoxes: Node[], + private readonly orientation: Orientation, + private readonly scrollingNode: Node, + private readonly itemsPerPage: number, + private readonly margin: number, + private readonly alignGroup: AlignGroup ) { + super( carousel ); + + // Hook up to listen to these nodes (will be handled by LayoutConstraint disposal) + [ this.backgroundNode, + this.foregroundNode, + this.windowNode, + this.previousButton, + this.nextButton, + this.scrollingNodeContainer, + ...this.alignBoxes ].forEach( node => this.addNode( node, false ) ); + + this.layout(); + } + + private computeClipArea(): Dimension2 { + const orientation = this.orientation; + + const visibleAlignBoxes = _.sortBy( this.alignBoxes.filter( alignBox => alignBox.visible ), alignBox => this.scrollingNode.children.indexOf( alignBox ) ); + + if ( visibleAlignBoxes.length === 0 ) { + return new Dimension2( 0, 0 ); + } + else { + + // This doesn't fill one page in number play preferences dialog when you forget locales=*, + // so take the last item, even if it is not a full page + const lastBox = visibleAlignBoxes[ this.itemsPerPage - 1 ] || visibleAlignBoxes[ visibleAlignBoxes.length - 1 ]; + + const horizontalSize = new Dimension2( + // Measure from the beginning of the first item to the end of the last item on the 1st page + lastBox[ orientation.maxSide ] - visibleAlignBoxes[ 0 ][ orientation.minSide ] + ( 2 * this.margin ), + + this.scrollingNodeContainer.boundsProperty.value[ orientation.opposite.size ] + ); + return this.orientation === Orientation.HORIZONTAL ? horizontalSize : horizontalSize.swapped(); + } + } + + private getBackgroundDimension(): Dimension2 { + let backgroundWidth; + let backgroundHeight; + if ( this.orientation === Orientation.HORIZONTAL ) { + + // For horizontal orientation, buttons contribute to width, if they are visible. + const nextButtonWidth = this.nextButton.visible ? this.nextButton.width : 0; + const previousButtonWidth = this.previousButton.visible ? this.previousButton.width : 0; + backgroundWidth = this.windowNode.width + nextButtonWidth + previousButtonWidth; + backgroundHeight = this.windowNode.height; + } + else { + + // For vertical orientation, buttons contribute to height, if they are visible. + const nextButtonHeight = this.nextButton.visible ? this.nextButton.height : 0; + const previousButtonHeight = this.previousButton.visible ? this.previousButton.height : 0; + backgroundWidth = this.windowNode.width; + backgroundHeight = this.windowNode.height + nextButtonHeight + previousButtonHeight; + } + return new Dimension2( backgroundWidth, backgroundHeight ); + } + + public override layout(): void { + const orientation = this.orientation; + + // Resize next/previous buttons dynamically + const maxOppositeSize = this.alignGroup.getMaxSizeProperty( orientation.opposite ).value; + const buttonOppositeSize = maxOppositeSize + ( 2 * this.margin ); + this.nextButton[ orientation.opposite.preferredSize ] = buttonOppositeSize; + this.previousButton[ orientation.opposite.preferredSize ] = buttonOppositeSize; + + this.nextButton[ orientation.opposite.centerCoordinate ] = this.backgroundNode[ orientation.opposite.centerCoordinate ]; + this.previousButton[ orientation.opposite.centerCoordinate ] = this.backgroundNode[ orientation.opposite.centerCoordinate ]; + this.windowNode[ orientation.opposite.centerCoordinate ] = this.backgroundNode[ orientation.opposite.centerCoordinate ]; + this.previousButton[ orientation.minSide ] = this.backgroundNode[ orientation.minSide ]; + this.nextButton[ orientation.maxSide ] = this.backgroundNode[ orientation.maxSide ]; + this.windowNode[ orientation.centerCoordinate ] = this.backgroundNode[ orientation.centerCoordinate ]; + + const clipBounds = this.computeClipArea().toBounds(); + this.windowNode.clipArea = Shape.bounds( clipBounds ); + + // Specify the local bounds in order to ensure centering. For full pages, this is not necessary since the scrollingNodeContainer + // already spans the full area. But for a partial page, this is necessary so the window will be centered. + this.windowNode.localBounds = clipBounds; + + const backgroundDimension = this.getBackgroundDimension(); + + this.carousel.backgroundWidth = backgroundDimension.width; + this.carousel.backgroundHeight = backgroundDimension.height; + + const backgroundBounds = backgroundDimension.toBounds(); + this.backgroundNode.rectBounds = backgroundBounds; + this.foregroundNode.rectBounds = backgroundBounds; + } +} + sun.register( 'Carousel', Carousel ); \ No newline at end of file ```

[marlitas]

@Nancy-Salpepi we think we solved it! Can you confirm on phettest?

[samreid]

I messaged Slack #dev-public:

Thanks to @marlitas @zepumph @matthew-blackman and me, Carousel got another significant improvement, which uses layout constraints more effectively and now a corner case no longer crashes. Details are in https://github.com/phetsims/circuit-construction-kit-common/issues/969. We tested several sims but not every sim exhaustively. Please test your use case and report any regressions.

[Nancy-Salpepi]

This looks good in master 🎉.

[matthew-blackman]

Everything looks good here. Closing this issue.