phetsims / sun

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

AccordionBox does not handle dynamic layout of title. #890

Open pixelzoom opened 1 month ago

pixelzoom commented 1 month ago

For example, here's what I see in BCE with ?stringTest=dynamic. The titles should remain centered. The relevant code in BCE is BoxNode, a subclass of AccordionBox.

screenshot_3448
pixelzoom commented 1 month ago

Here's the relevant code in AccordionBox.ts. Note that there's no listener for this.titleNode.localBoundsProperty, so no dynamic layout.

    // title horizontal layout
    if ( isWidthSizable( this.titleNode ) ) {
      this.titleNode.preferredWidth = titleRightSpan - titleLeftSpan;
    }
    if ( options.titleAlignX === 'left' ) {
      this.titleNode.left = titleLeftSpan;
    }
    else if ( options.titleAlignX === 'right' ) {
      this.titleNode.right = titleRightSpan;
    }
    else { // center
      this.titleNode.centerX = collapsedBounds.centerX;
    }

    // button & title vertical layout
    if ( options.titleAlignY === 'top' ) {
      this.expandCollapseButton.top = this.collapsedBox.top + Math.max( options.buttonYMargin, options.titleYMargin );
      this.titleNode.top = this.expandCollapseButton.top;
    }
    else { // center
      this.expandCollapseButton.centerY = this.collapsedBox.centerY;
      this.titleNode.centerY = this.expandCollapseButton.centerY;
    }
jonathanolson commented 1 month ago

Weird, adding the titleNode to the LayoutConstraint with addNode (https://github.com/phetsims/sun/blob/43ac9f1394995c5b5a1b9f6969c2b3ede9bab509/js/AccordionBox.ts#L523) SHOULD be adding a listener to title bounds changes. Investigating why this isn't working.

jonathanolson commented 1 month ago

Ahh, resize: false provided in BoxNode is turning layout updates off for the component. Presumably in this case, that can be removed?

https://github.com/phetsims/balancing-chemical-equations/blob/88c42d33a666b9e8d16bc09162699a4242d05356/js/common/view/BoxNode.ts#L67

pixelzoom commented 1 month ago

Ahh, resize: false provided in BoxNode is turning layout updates off for the component. Presumably in this case, that can be removed?

Will do for BoxNode.

But in general... Why does resize: false affect alignment of the title? If the title gets smaller, or its maxWidth is set appropriately (as is the case with BoxNode), there would be no need to resize the AccordionBox.

jonathanolson commented 4 weeks ago

But in general... Why does resize: false affect alignment of the title?

Because the layout code is currently responsible for aligning the title, and resize:false is currently used to determine whether we run the layout more than one time.

This seems a bit conceptually wrong, since the title positioning (and potentially other parts of the layout) aren't technically related to resizing.

jonathanolson commented 4 weeks ago

We could wrap the titleNode within an AlignBox that is sized to the space it has (determined during layout), so that it will correct alignment when not resizing. Do you think that would be helpful?

pixelzoom commented 3 weeks ago

Yes.