phetsims / sun

User-interface components for PhET simulations, built on top of Scenery.
MIT License
4 stars 12 forks source link

Carousel `scrollToItem` will behave badly if the item in not visible #830

Closed pixelzoom closed 1 year ago

pixelzoom commented 1 year ago

In Carousel.ts

  public scrollToItem( item: CarouselItem ): void {
    // If the layout is dynamic, then only account for the visible items
    const alignBoxIndex = this.visibleAlignBoxesProperty.value.indexOf( this.getAlignBoxForItem( item ) );

    assert && assert( alignBoxIndex >= 0, 'item not present or visible' );

    this.scrollToItemVisibleIndex( alignBoxIndex );
  }

This is going to result in some hard failures in PhET-iO wrappers. Here's the not-so-hypotheical scenario:

  1. My sim has a toolbox, implemented as a carousel (like Function Builder). When I return items to the toolbox, the toolbox scrolls to page for that item.
  2. I open the sim in Studio, pulls some objects out of the toolbox, then hides some of those items in the toolbox. Then they create a wrapper.
  3. Testing the wrapper, I drag one of the items back to the toolbox. The item in not visible in the toolbox, so things either fail or behave badly.

I think it's appropriate to fail when assertions are enabled. But perhaps we should should consider ignoring scrollToItem if this occurs when assertions are enabled, instead of potentially failing further downstream. Something like this:

  public scrollToItem( item: CarouselItem ): void {
    // If the layout is dynamic, then only account for the visible items
    const alignBoxIndex = this.visibleAlignBoxesProperty.value.indexOf( this.getAlignBoxForItem( item ) );

    assert && assert( alignBoxIndex >= 0, 'item not present or visible' );
-    this.scrollToItemVisibleIndex( alignBoxIndex );
+    if ( alignBoxIndex >= 0 ) {
+      this.scrollToItemVisibleIndex( alignBoxIndex );
+    }
  }
pixelzoom commented 1 year ago

@samreid and I discussed on Zoom. We added the guard that was proposed in https://github.com/phetsims/sun/issues/830#issue-1572892730. Closing.