Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NumberControl behaves badly with arrowButtonOptions.fireOnDown: true #825

Open
pixelzoom opened this issue Nov 13, 2023 · 3 comments
Open
Labels

Comments

@pixelzoom
Copy link
Contributor

In phetsims/my-solar-system#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
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
Copy link
Contributor Author

pixelzoom commented Nov 13, 2023

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
Copy link
Contributor

AgustinVallejo commented Jan 5, 2024

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
Copy link
Contributor

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.

AgustinVallejo added a commit to phetsims/my-solar-system that referenced this issue Jan 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants