ComboBox maxWidth overrides Sizable minimumWidth

[marlitas]

@pixelzoom commented in https://github.com/phetsims/sun/issues/810, that he was seeing buggy behavior with ComboBox in Fourier:

Was this tested with dynamic layout? I'm seeing buggy layout when the combo box button grows. For example, here's the control panel in the Discrete screen of Fourier, after pressing RIGHT arrow once with stringTest=dynamic. The ▼ is outside of the button bounds, and should be centered in the space to the right of the vertical separator.

Investigation showed that AlignBox's minimumWidth is different than it's child's minimum width. This seems very incorrect.

AlignBox minimumWidth = 135

GridBox minimumWidth = 148

It is worth noting that this bug is not seen in all implementations of ComboBox, however removing custom ComboBox options from Fourier did not resolve the problem either.

[samreid]

@marlitas and I attempted to run the layout manually, it seems like it could be brought to production with fine-tuning, but it would be better to understand why GridBox isn't working as expected here:

```diff Subject: [PATCH] test --- Index: js/ComboBoxButton.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/ComboBoxButton.ts b/js/ComboBoxButton.ts --- a/js/ComboBoxButton.ts (revision 4fb6980fa08d426ab17374e43666edb5e2fcab46) +++ b/js/ComboBoxButton.ts (date 1671042498870) @@ -9,7 +9,7 @@ import { Shape } from '../../kite/js/imports.js'; import optionize from '../../phet-core/js/optionize.js'; -import { AriaHasPopUpMutator, GridBox, Line, Node, Path, PDOMBehaviorFunction, PDOMPeer, TPaint } from '../../scenery/js/imports.js'; +import { AriaHasPopUpMutator, GridBox, HBox, Line, Node, Path, PDOMBehaviorFunction, PDOMPeer, TPaint, VSeparator } from '../../scenery/js/imports.js'; import Tandem from '../../tandem/js/Tandem.js'; import ButtonNode from './buttons/ButtonNode.js'; import RectangularPushButton, { RectangularPushButtonOptions } from './buttons/RectangularPushButton.js'; @@ -123,11 +123,6 @@ // Wrapper for the selected item's Node. // Do not transform ComboBoxItem.node because it is shared with ComboBoxListItemNode. const itemNodeWrapper = new Node( { - layoutOptions: { - yMargin: itemYMargin, - grow: 1, - xAlign: options.align - }, children: [ _.find( items, item => item.value === property.value )!.node ] @@ -141,12 +136,8 @@ lineWidth: options.lineWidth } ); - options.content = new GridBox( { - rows: [ [ - itemNodeWrapper, - separatorLine, - arrow - ] ] + options.content = new Node( { + children: [ itemNodeWrapper, separatorLine, arrow ] } ); Multilink.multilink( [ maxItemWidthProperty, maxItemHeightProperty ], ( maxItemWidth, maxItemHeight ) => { @@ -183,22 +174,30 @@ }; arrow.shape = createArrowShape( options.arrowDirection, arrowWidth, arrowHeight ); - arrow.mutateLayoutOptions( { - minContentWidth: arrowAreaSize, - minContentHeight: arrowAreaSize - } ); - - itemNodeWrapper.mutateLayoutOptions( { - minContentWidth: maxItemWidth, - minContentHeight: maxItemHeight, - leftMargin: leftMargin, - rightMargin: middleMargin - } ); + // arrow.mutateLayoutOptions( { + // minContentWidth: arrowAreaSize, + // minContentHeight: arrowAreaSize + // } ); + // + // itemNodeWrapper.mutateLayoutOptions( { + // minContentWidth: maxItemWidth, + // minContentHeight: maxItemHeight, + // leftMargin: leftMargin, + // rightMargin: middleMargin + // } ); separatorLine.y2 = fullHeight; - separatorLine.mutateLayoutOptions( { - rightMargin: rightMargin - } ); + // separatorLine.mutateLayoutOptions( { + // rightMargin: rightMargin + // } ); + + itemNodeWrapper.left = leftMargin; + separatorLine.centerX = itemNodeWrapper.right + middleMargin; + arrow.centerX = separatorLine.right + middleMargin; + + itemNodeWrapper.centerY = fullHeight / 2; + separatorLine.centerY = fullHeight / 2; + arrow.centerY = fullHeight / 2; } ); // Margins are different in the item and button areas. And we want the vertical separator to extend ```

[marlitas]

@samreid did some great digging on this, and we found the culprit code:

Subject: [PATCH] Remove maxWidth
---
Index: js/discrete/view/DiscreteControlPanel.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/discrete/view/DiscreteControlPanel.js b/js/discrete/view/DiscreteControlPanel.js
--- a/js/discrete/view/DiscreteControlPanel.js  (revision 8a6d4788b7bac1ba4e0a2f673fea0368b346f87f)
+++ b/js/discrete/view/DiscreteControlPanel.js  (date 1671044716655)
@@ -179,7 +179,6 @@
     } );

     const waveformComboBox = new WaveformComboBox( waveformProperty, popupParent, {
-      maxWidth: 135,
       tandem: options.tandem.createTandem( 'waveformComboBox' )
     } );

@@ -280,7 +279,6 @@
     } );

     const domainComboBox = new DomainComboBox( domainProperty, popupParent, {
-      maxWidth: 135,
       tandem: options.tandem.createTandem( 'functionOfComboBox' ) // tandem name differs by request
     } );

@@ -311,7 +309,6 @@
     } );

     const equationComboBox = new EquationComboBox( equationFormProperty, domainProperty, popupParent, {
-      maxWidth: 135,
       tandem: options.tandem.createTandem( 'equationComboBox' )
     } );

The maxWidth on ComboBox is overriding the minimum width that should be bubbling up from GridBox cells. @pixelzoom, do you feel comfortable with removing these usages of maxWidth until we can implement a longer term solution here?

Not sure what the best way is to proceed. I would like to consult with @jonathanolson, but this will not be a priority issue until after maintenance releases are completed.

[pixelzoom]

In the above commit, I applied the patch in https://github.com/phetsims/sun/issues/816#issuecomment-1352000872. Setting maxWidth on a ComboBox is not essential, it's just a bit of insurance in case the ComboBox grows larger than expected. ComboBoxIitems should have maxWidth on any Text nodes (which these combo boxes do).

I don't know what the timeframe is for fixing this. But if it's going to be more than a few days, consider this temporary prohibition on maxWidth in ComboBox.ts. It will identify other TypeScript sims that have this problem.

- export type ComboBoxOptions = SelfOptions & ParentOptions;
+ // maxWidth is buggy, see https://github.com/phetsims/sun/issues/816
+ export type ComboBoxOptions = SelfOptions & StrictOmit<ParentOptions, 'maxWidth'>;

[marlitas]

I don't know what the timeframe is for fixing this. But if it's going to be more than a few days, consider this temporary prohibition on maxWidth in ComboBox.ts. It will identify other TypeScript sims that have this problem.

I took a look through usages and I found some sims that are indeed using maxWidth on their comboBox, but is not reproducing the same buggy behavior and eliminating maxWidth has other consequences. I'm not confident that I can use that patch without repercussions...

Edit: I'm going to keep looking through this to see if theres a good short term fix, but I'm not feeling confident in that with some of the use cases I'm seeing. Removing the blocks-publication label, as there is a fix if a sim is experiencing this problem.

[jonathanolson]

This was actually a case where:

(1) You have something sizable (can get preferred sizes) like a ComboBox. (2) You don't turn off that sizability (e.g. no widthSizable: false, heightSizable: false, sizable: false that covers a specific dimension) (2) You put a maxWidth/maxHeight on that dimension. (3) You put the node inside a layout container (4) The container is configured to increase the size of the node if it has enough room (5) The size is attempted to be increased PAST the maxWidth/maxHeight provided.

So while it isn't just ComboBox, it also requires a lot of things going on to get to this buggy situation.

I added an assertion to catch this case, and fixed the 1 other place it happened (CCK).

@marlitas can you review?

[marlitas]

Ahh okay. That is very tricky. That assertion looks very clear and fits well with the explanation you provided. Thanks for the investigation and fix here @jonathanolson. I believe this is ready to close.