phetsims / scenery-phet

Reusable components based on Scenery that are specific to PhET simulations.
MIT License
8 stars 6 forks source link

NumberControl behaves badly with `arrowButtonOptions.fireOnDown: true` #825

Open pixelzoom opened 8 months ago

pixelzoom commented 8 months ago

In https://github.com/phetsims/my-solar-system/issues/292, NumberControl was reported to behave oddly when created with this option:

        arrowButtonOptions: {
          fireOnDown: true
        },

I verified that this is a general problem using this patch:

patch ```diff Subject: [PATCH] write to phet.log whenever followingCenterOfMassProperty changes, https://github.com/phetsims/my-solar-system/issues/274 --- Index: js/demo/sliders/demoNumberControl.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/demo/sliders/demoNumberControl.ts b/js/demo/sliders/demoNumberControl.ts --- a/js/demo/sliders/demoNumberControl.ts (revision 1d488c4e5a76152ba1010470396a08d14d8a39ae) +++ b/js/demo/sliders/demoNumberControl.ts (date 1699901545610) @@ -15,7 +15,7 @@ export default function demoNumberControl( layoutBounds: Bounds2 ): Node { - const weightRange = new RangeWithValue( 0, 300, 100 ); + const weightRange = new RangeWithValue( 0, 300, 295 ); // all NumberControls will be synchronized with these Properties const weightProperty = new Property( weightRange.defaultValue ); @@ -40,6 +40,9 @@ { value: weightRange.max, label: new Text( weightRange.max, { font: new PhetFont( 20 ) } ) } ], minorTickSpacing: 50 + }, + arrowButtonOptions: { + fireOnDown: true } }; ```

... and these test steps:

  1. Run scenery-phet demo
  2. Go to the Sliders screen
  3. Select "NumberControl" from the combo box
  4. For the top NumberControl (or any NumberControl) press the increment (right arrow) button multiple times until the control is at its max and the increment button is disabled. (Do not press and hold the increment button.)
  5. Drag the slider to the left, then release it.
  6. You'll see the slider automatically incrementing, like it's in some sort of loop.

A similar problem occurs with the decrement (left arrow) button.

pixelzoom commented 8 months ago

In the above commit, I made omitted arrowButtonOptions.fireOnDown as an option to NumberControl, at least for TypeScript sims. This is a workaround, so I also added a TODO referencing this issue.

I also inspected all occurrences of arrowButtonOptions:, and I did not find any other uses of fireOnDown.

AgustinVallejo commented 6 months ago

Not sure if this is the right place for this comment, but I noticed that for ArrowButtons (not sure if other Buttons as well), the endCallback is being called before actually changing the numberProperty.

Went down the rabbit hole and found that in ButtonModel.ts there's a link of this.downProperty with options.endCallback() (line 131). But then in PushButtonModel.ts there's another link to this.downProperty which actually causes the fire (line 147) after the endCallback was called already.

AgustinVallejo commented 6 months ago

After some discussion, it seems we won't meddle with the ButtonModel. In the above commit I referenced this issue for a workaround in MSS.