Eraser button only hides entered numbers

[KatieWoe]

Test device Dell Operating System Win 11 Browser Chrome Problem description For https://github.com/phetsims/qa/issues/1046 If you press the erase button on a screen, any number you were previously entering will appear gone, but when you try to enter a new number, it will reappear and continue adding numbers at the end. If the number was three digits, nothing will appear until you press backspace. This occurs both with keyboard and number pad entry. This is not in published. Steps to reproduce

1. Enter a game and start to answer a problem, but don't press enter
2. Press the eraser button
3. Try to enter a new number

Visuals

Troubleshooting information:

!!!!! DO NOT EDIT !!!!! Name: ‪Arithmetic‬ URL: https://phet-dev.colorado.edu/html/arithmetic/1.1.0-dev.4/phet/arithmetic_all_phet.html Version: 1.1.0-dev.4 2024-02-21 16:48:20 UTC Features missing: applicationcache, applicationcache, touch Flags: pixelRatioScaling User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/121.0.0.0 Safari/537.36 Language: en-US Window: 1280x631 Pixel Ratio: 1.5/1 WebGL: WebGL 1.0 (OpenGL ES 2.0 Chromium) GLSL: WebGL GLSL ES 1.0 (OpenGL ES GLSL ES 1.0 Chromium) Vendor: WebKit (WebKit WebGL) Vertex: attribs: 16 varying: 30 uniform: 4096 Texture: size: 16384 imageUnits: 16 (vertex: 16, combined: 32) Max viewport: 32767x32767 OES_texture_float: true Dependencies JSON: {}

[Luisav1]

It seems the keypad is using accumulated values. I'm not sure why it wasn't doing this before, but I added a fix by clearing any accumulated keys whenever the eraser button is hit. Although it was easy I'm not sure this is the best way since it is exposing the keypad so I'd like a code review.

[marlitas]

I provided two different patches. The first option just adds some documentation and then also checks the existence of this.keypad instead of the option showKeypad. This just feels a bit safer in case for some reason they are not aligned.

The second option does the same without converting keypad into a class property that way you don't have to expose it. I think the second option might be preferable so that we don't create a class property if we don't need to, but I would be okay with the first option as well. Whatever you feel is best.

Option 1:

```diff Subject: [PATCH] keypad --- Index: js/common/view/WorkspaceNode.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/common/view/WorkspaceNode.js b/js/common/view/WorkspaceNode.js --- a/js/common/view/WorkspaceNode.js (revision 6ca8328118d1a2249324ee5bf1378e40c8c70f3c) +++ b/js/common/view/WorkspaceNode.js (date 1708709932610) @@ -106,7 +106,7 @@ ArithmeticGlobals.timerEnabledProperty, () => { model.refreshLevel(); - options.showKeypad && this.keypad.setClearOnNextKeyPress( true ); + this.keypad && this.keypad.setClearOnNextKeyPress( true ); }, { title: options.scoreboardTitle, @@ -127,7 +127,11 @@ controlPanelVBox.top = multiplicationTableNode.top; this.addChild( controlPanelVBox ); - // add keypad if necessary + /** + * Add the keypad if it is enabled. + * @type {Keypad|null} + * @private + */ this.keypad = null; if ( options.showKeypad ) { ```

Option 2:

```diff Subject: [PATCH] option 2 keypad --- Index: js/common/view/WorkspaceNode.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/common/view/WorkspaceNode.js b/js/common/view/WorkspaceNode.js --- a/js/common/view/WorkspaceNode.js (revision 6ca8328118d1a2249324ee5bf1378e40c8c70f3c) +++ b/js/common/view/WorkspaceNode.js (date 1708710164539) @@ -98,6 +98,12 @@ // define the width of the control panel so that it fits between the table and the bounds with some margin const controlPanelWidth = layoutBounds.maxX - multiplicationTableNode.right - 60; + /** + * The keypad will be defined below if it is to be shown, otherwise it is null. + * @type {Keypad|null} + */ + let keypad = null; + // add control panel const scoreboardNode = new ScoreboardNode( model.levelNumberProperty, @@ -106,7 +112,7 @@ ArithmeticGlobals.timerEnabledProperty, () => { model.refreshLevel(); - options.showKeypad && this.keypad.setClearOnNextKeyPress( true ); + keypad && keypad.setClearOnNextKeyPress( true ); }, { title: options.scoreboardTitle, @@ -127,12 +133,10 @@ controlPanelVBox.top = multiplicationTableNode.top; this.addChild( controlPanelVBox ); - // add keypad if necessary - this.keypad = null; if ( options.showKeypad ) { // create and add the keypad - const keypad = new Keypad( Keypad.PositiveIntegerLayout, { + keypad = new Keypad( Keypad.PositiveIntegerLayout, { accumulatorOptions: { maxDigits: 3 }, @@ -140,7 +144,6 @@ global: true } } ); - this.keypad = keypad; keypad.stringProperty.link( input => { model.inputProperty.set( input ); } ); ```

back to you @Luisav1

[Luisav1]

Thanks @marlitas! I'll go with option 2 since I also like that it's not a class property anymore and only assigned when necessary.

[Luisav1]

This is now fixed as seen below.

https://github.com/phetsims/arithmetic/assets/52978048/5e886dd4-bf86-453a-97a9-089f57e3b6c1

Closing.