NumberControl tweaker buttons should be able to match the size of the NumberDisplay

[samreid]

In #508 (comment) I said:

Designers have requested the tweaker buttons height to match the height of the number readout display, but the NumberControl API for the tweaker buttons is via arrowButtonOptions: {scale}, so the current implementation is jury-rigged. Is there a more solid way to do it?

@pixelzoom replied:

Something like this in NumberControl:

// By default, scale the ArrowButtons to have the same height as the NumberDisplay.
if ( options.arrowButtonOptions.scale === undefined ) {
  const arrowButtonsScale = numberDisplay.height / leftArrowButton.height; 
  leftArrowButton.setScaleMagnitude( arrowButtonsScale );
  rightArrowButton.arrowButtonsScale( arrowButtonScale );
}

UPDATE: Tagging for phetsims/wave-interference#408

[samreid]

Would it be better to use null instead of undefined? Would we change the default value from 0.85 to null? And, if so, will we need to check through all sims that used the old default to make sure the new default is acceptable?

[pixelzoom]

Would it be better to use null instead of undefined?

Since scale is Node option, I would probably use whatever scenery used for the default. Looking at DEFAULT_OPTIONS in Node, it looks like the default for scale is probably undefined.

Would we change the default value from 0.85 to null?

Yes. Unless it's OK to push this new convention into all sims, which is a question for designers.

And, if so, will we need to check through all sims that used the old default to make sure the new default is acceptable?

Yes.

[samreid]

I tried using this code in Wave Interference:

arrowButtonOptions: {
  scale: undefined
},

And was met with

Assertion failed: Undefined not allowed for Node key: scale

Therefore it does not seem there is a simple way for pre-existing simulations to have 0.85 and other occurrences opt-in to undefined.

@pixelzoom how do you recommend to proceed?

[pixelzoom]

There's no reason to set scale: undefined explicitly as the default in arrowButtonOptions. You want to test for whether the caller provided this option. If testing for options.arrowButtonOptions.scale === undefined seem distasteful, then use !options.arrowButtonOptions.hasOwnProperty( 'scale' ).

[samreid]

Here's a change set that defaults the arrow buttons to the height of the number display:

Index: wave-interference/js/common/WaveInterferenceConstants.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- wave-interference/js/common/WaveInterferenceConstants.js	(revision 4688cc1aa0cf83de79dc1f3c49d8e6fd53590393)
+++ wave-interference/js/common/WaveInterferenceConstants.js	(date 1563227871000)
@@ -44,7 +44,6 @@
     NUMBER_CONTROL_OPTIONS: {
       layoutFunction: NumberControl.createLayoutFunction4( { verticalSpacing: 3 } ),
       arrowButtonOptions: {
-        scale: 0.65,
         touchAreaXDilation: 9,
         touchAreaYDilation: 10
       },
Index: scenery-phet/js/WavelengthNumberControl.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- scenery-phet/js/WavelengthNumberControl.js	(revision de648f0e5ac55f8a0e44e9522044706fbce0b7ae)
+++ scenery-phet/js/WavelengthNumberControl.js	(date 1563227795000)
@@ -45,9 +45,6 @@
       const trackHeight = options.trackHeight;
 
       super( options.title, property, options.range, merge( {
-        arrowButtonOptions: {
-          scale: trackHeight * 0.0315 // roughly the height of the track
-        },
         titleNodeOptions: {
           font: new PhetFont( 15 ),
           maxWidth: 175
Index: scenery-phet/js/NumberControl.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- scenery-phet/js/NumberControl.js	(revision de648f0e5ac55f8a0e44e9522044706fbce0b7ae)
+++ scenery-phet/js/NumberControl.js	(date 1563228764000)
@@ -19,6 +19,7 @@
   const HBox = require( 'SCENERY/nodes/HBox' );
   const HSlider = require( 'SUN/HSlider' );
   const inherit = require( 'PHET_CORE/inherit' );
+  const InstanceRegistry = require( 'PHET_CORE/documentation/InstanceRegistry' );
   const merge = require( 'PHET_CORE/merge' );
   const Node = require( 'SCENERY/nodes/Node' );
   const NumberControlIO = require( 'SCENERY_PHET/NumberControlIO' );
@@ -88,12 +89,13 @@
       groupFocusHighlight: true
     }, options );
 
+    const arrowButtonScaleProvided = options.arrowButtonOptions && options.arrowButtonOptions.hasOwnProperty( 'scale' );
+
     // Merge all nested options in one block.
     options = merge( {
 
       // Options propagated to ArrowButton
       arrowButtonOptions: {
-        scale: 0.85,
 
         // Values chosen to match previous behavior, see https://github.com/phetsims/scenery-phet/issues/489.
         // touchAreaXDilation is 1/2 of its original value because touchArea is shifted.
@@ -243,6 +245,16 @@
       tandem: options.tandem.createTandem( 'rightArrowButton' )
     }, options.arrowButtonOptions ) );
 
+    // By default, scale the ArrowButtons to have the same height as the NumberDisplay.
+    if ( !arrowButtonScaleProvided ) {
+
+      leftArrowButton.setScaleMagnitude( 1 );
+      const arrowButtonsScale = numberDisplay.height / leftArrowButton.height;
+
+      leftArrowButton.setScaleMagnitude( arrowButtonsScale );
+      rightArrowButton.setScaleMagnitude( arrowButtonsScale );
+    }
+
     // arrow button touchAreas, asymmetrical, see https://github.com/phetsims/scenery-phet/issues/489
     leftArrowButton.touchArea = leftArrowButton.localBounds
       .dilatedXY( arrowButtonPointerAreaOptions.touchAreaXDilation, arrowButtonPointerAreaOptions.touchAreaYDilation )
@@ -333,6 +345,9 @@
       numberProperty.unlink( arrowEnabledListener );
       this.enabledProperty.unlink( enabledObserver );
     };
+
+    // support for binder documentation, stripped out in builds and only runs when ?binder is specified
+    assert && phet.chipper.queryParameters.binder && InstanceRegistry.registerDataURL( 'scenery-phet', 'NumberControl', this );
   }
 
   /**

We cannot have some sims use one default and other sims use another default, so I think we will need to see if we can make this change for all simulations. Thanks to binder, here is a list of sims that use NumberControl:

beers-law-lab: no change
coulombs-law
energy-skate-park
gas-properties
gravity-force-lab
hookes-law
masses-and-springs
masses-and-springs-basics
pendulum-lab
plinko-probability
projectile-motion
rutherford-scattering
wave-interference

I'll check through them to see how different the default looks.

[samreid]

When I restricted the binder search to sims that use the defaults for NumberControl, it returned this list:

gas-properties
hookes-law
plinko-probability
rutherford-scattering
wave-interference

gas properties

the change looks minor, but seems acceptable to me
Original

Proposed change

hookes law

tweaker buttons become slightly smaller with this change
Original

Proposed

plinko probability

Original

Proposed

rutherford scattering

Original

Proposed

And wave interference was requested to have the tweaker buttons have the same height as the number display, so that's a safe change.

@arouinfar do these changes seem acceptable to you? Do we need other designers to sign off before I proceed?

[pixelzoom]

Couldn't help lurking and throwing my opinion in :) These changes all look mighty fine to me. Might be good to also check with ?showPointerAreas to make sure there's nothing whacky going on there.

[samreid]

I tested hookes law, and it looks like the pointer areas changed proportionately:

Original

Proposed

[arouinfar]

Looks great @samreid! It's a subtle change, but I think matching tweaker height to the readout makes things look cleaner overall.

[samreid]

Changes pushed, @pixelzoom can you please review?

[pixelzoom]

Changes reviewed, looks good. Feel free to close if work is completed.

[pixelzoom]

I'm really grasping at straws here.

Me too, no good solution comes to mind immediately. I will ruminate.

[pixelzoom]

Rumination complete :)

My recommendation is to back out this change, and require the client to adjust the size of the arrow buttons. If the changes that @arouinfar identified as "cleaner overall" in #513 (comment) are worth keeping, note the arrow button sizes before reverting, and set them explicitly after reverting.

[pixelzoom]

More...

I'm not sure that reverting is the best option, but it's the best I could come up with. Setting arrow button sizes explicitly will result in fixes sizes that won't adapt to change in NumberDisplay size that occur due to font size differences across platforms. But this never bothered me previously, seemed to work fine. And the improvement was described by @arouinfar as "subtle", so maybe not necessary.

On the other hand... The example shown in #513 (comment) is highly unlikely to occur. So while I appreciate @KatieWoe's attention to detail in discovering this, perhaps we could just leave "as is" and everything would be fine in practice.

[pixelzoom]

Still more...

So the options that I can come up with are:

(1) Revert and live with it.

(2) Leave as is and live with it.

[samreid]

One more part of this problem. I don't see a clear way to explicitly set the height of these buttons, the API is instead centered around arrowButtonOptions.scale. The default value is 1, but it's unclear how big that is exactly, or what it is supposed to match. I'm seeing several sims that are using 0.56 or 0.5 or 0.55 for the scale. Should this API ultimately be changed to be about width and/or height? Or possibly just one length dimension (and assuming it is square or nearly square or keeps its aspect ratio)? This could be done in a separate issue but it seems like it equally applies to the cases of (a) we revert changes and do not try to match the NumberDisplay height or (b) a sim needs to opt out of matching the NumberDisplay height by passing an option.

[samreid]

@arouinfar can you please review #513 (comment) and below? What do you recommend?

[samreid]

Another solution would be to (by default) set the height of the tweaker buttons to match the height of the unscaled numberDisplay. @jonathanolson pointed out this can be done by using the localBounds.height of the number instead of just the height. This appears OK in local testing and resolves the shrinking button problem, I'll commit and run this past @pixelzoom.

UPDATE: the only problem I can think of for this paradigm is that: specified values for {numberDisplayOptions:{scale}} will also not be respected, like so:

      numberDisplayOptions: {
        scale: 0.5,

Still, this seems preferable to any of the other proposed strategies.

[samreid]

@pixelzoom can you please review?

UPDATE: I also marked as blocks-publication until this is reviewed since it can impact NumberControl rendering. I spot tested a few sims (not exhaustive testing).

UPDATE: I also skimmed through numberDisplayOptions object literals and didn't see any specifying scale

[pixelzoom]

This looks good. I'd like to see a comment in the code about why we're ignoring NumberDisplay maxWidth and instead using localBounds, specifically mentioning the problem reported in #513 (comment) (a link to that comment would be good too).

[samreid]

I added docs above. It felt a bit wordy but I wasn't sure how to improve it. @pixelzoom can you please review?

[pixelzoom]

👍 Looks good to me, not too wordy. Closing.