NumberSpinner has layout problems when range is dynamic.

[pixelzoom]

Blocking for Natural Selection https://github.com/phetsims/natural-selection/issues/299

NumberSpinner should be using LayoutBox for the layout of its NumberDisplay and 2 ArrowButtons. It's currently doing fixed layout, around line 153. This doesn't account for the fact that valueRangeProperty may change, and it assumes that the size of its NumberDisplay will never change. That's not always the case, as was reported in https://github.com/phetsims/natural-selection/issues/299, and can result in the NumberDisplay overlapping the ArrowButtons.

[pixelzoom]

... or if using LayoutBox is too disruptive, then NumberSpinner should observe numberDisplay.boundsProperty, and update the layout if it changes.

[pixelzoom]

Because there are 4 different layout options, I went with the approach of observing numberDisplay.boundsProperty. That require no changes to the layout code, seems unlikely to introduce changes/regressions in sims that use NumberSpinner, and seems like an OK longterm solution. Verified in natural-selection and a couple of other sims.

I consider making updateLayout a class method, so that it won't be defined in every constructor instance. But that would require defining 3 new private this fields, so adding 1 function in the constructor seems like an OK tradeoff.

@samreid would you mind taking a quick look? Close if OK.

[samreid]

I used this test harness, and confirmed that without the fix there is overlapping, and with the fix the overlapping is resolved:

```diff Index: js/NumberSpinner.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/NumberSpinner.js b/js/NumberSpinner.js --- a/js/NumberSpinner.js (revision 663ada3f9baa5e5c40f5b29174e2b03bdeba629c) +++ b/js/NumberSpinner.js (date 1625508916749) @@ -9,6 +9,7 @@ import merge from '../../phet-core/js/merge.js'; import NumberDisplay from '../../scenery-phet/js/NumberDisplay.js'; +import PhetFont from '../../scenery-phet/js/PhetFont.js'; import Node from '../../scenery/js/nodes/Node.js'; import SceneryConstants from '../../scenery/js/SceneryConstants.js'; import Tandem from '../../tandem/js/Tandem.js'; @@ -98,6 +99,8 @@ tandem: options.tandem.createTandem( 'numberDisplay' ) } ) ); + setInterval( () => numberDisplay.scale( 1.02 ), 1000 ); + // buttons const arrowButtonOptions = { baseColor: options.arrowButtonFill, ```

I also observed that the commit message for https://github.com/phetsims/sun/commit/b42e11befa48ce2df2e02c3f113d56f82983902b was just 2 issue references, and not an explanation of the changes in the commit, but I presume that was just a mistake, and nothing needs to be done about it.

Would I also be correct in saying that: since the NumberSpinner creates and owns the NumberDisplay, it is not necessary to unlink from the boundsProperty in dispose?

Feel free to close after commenting.

[pixelzoom]

I also observed that the commit message for b42e11b was just 2 issue references, and not an explanation of the changes in the commit, but I presume that was just a mistake, and nothing needs to be done about it.

Yeah, sorry about that. The commits were related to 2 issues, so I had to find 2 URLs. Then I forgot to put more than the URLs in the commit message.

Would I also be correct in saying that: since the NumberSpinner creates and owns the NumberDisplay, it is not necessary to unlink from the boundsProperty in dispose?

Correct. But this caused me to notice an unrelated bug in NumberSpinner. It creates tandems for subcomponents (numberDisplay, incrementButton, decrementButton), but fails to clean those up in disposeNumberDisplay. I'll create a separate issue for that.

Closing.