phetsims / projectile-data-lab

"Projectile Data Lab" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
0 stars 0 forks source link

Interval sounds continue after unchecking checkbox #267

Closed Nancy-Salpepi closed 6 months ago

Nancy-Salpepi commented 6 months ago

Test device iPad 9th generation

Operating System iPadOS 17

Browser Safari

Problem description For https://github.com/phetsims/qa/issues/1060 on the measures screen: if I uncheck the interval tool checkbox while moving the interval tool, the interval tool sounds are still heard until I lift my finger.

Steps to reproduce

  1. On the Measures screen, check the Interval Tool checkbox
  2. With one finger move the Interval Tool back and forth
  3. While still moving the Interval Tool, uncheck the checkbox with another finger

Visuals

https://github.com/phetsims/projectile-data-lab/assets/87318828/acc8e521-d6d7-441e-bdab-c89960a75204

samreid commented 6 months ago

I thought this patch would fix it, but it did not appear to. But more importantly, I saw crashing behavior when using multitouch + the interval tool on my iphone, which may be unrelated but made it difficult to test this fix.

```diff Subject: [PATCH] Use J as a key to jump to different fields, see https://github.com/phetsims/projectile-data-lab/issues/266 --- Index: js/measures/view/IntervalToolNode.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/measures/view/IntervalToolNode.ts b/js/measures/view/IntervalToolNode.ts --- a/js/measures/view/IntervalToolNode.ts (revision 92fb76da9ae8cd0cd9857f23f8bd77604faca31d) +++ b/js/measures/view/IntervalToolNode.ts (date 1711405430370) @@ -38,6 +38,7 @@ import AccessibleSlider, { AccessibleSliderOptions } from '../../../../sun/js/accessibility/AccessibleSlider.js'; import Bounds2 from '../../../../dot/js/Bounds2.js'; import DynamicProperty from '../../../../axon/js/DynamicProperty.js'; +import TReadOnlyProperty from '../../../../axon/js/TReadOnlyProperty.js'; const edgeFilter = new BiquadFilterNode( phetAudioContext, { type: 'lowpass', @@ -91,7 +92,7 @@ // This Property represents whether the user is dragging via the center readout, which translates the entire interval. private readonly isCenterDraggingProperty = new BooleanProperty( false ); - public constructor( intervalTool: IntervalTool, modelViewTransform: ModelViewTransform2, providedOptions: IntervalToolNodeOptions ) { + public constructor( intervalTool: IntervalTool, isIntervalToolVisibleProperty: TReadOnlyProperty, modelViewTransform: ModelViewTransform2, providedOptions: IntervalToolNodeOptions ) { const options = optionize()( { phetioFeatured: true, @@ -386,6 +387,12 @@ ]; this.addLinkedElement( intervalTool ); + + isIntervalToolVisibleProperty.link( visible => { + if ( !visible ) { + this.interruptSubtreeInput(); + } + } ); } } Index: js/measures/view/MeasuresInteractiveToolPanel.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/measures/view/MeasuresInteractiveToolPanel.ts b/js/measures/view/MeasuresInteractiveToolPanel.ts --- a/js/measures/view/MeasuresInteractiveToolPanel.ts (revision 92fb76da9ae8cd0cd9857f23f8bd77604faca31d) +++ b/js/measures/view/MeasuresInteractiveToolPanel.ts (date 1711405267384) @@ -41,7 +41,7 @@ const intervalTool = new IntervalTool( { tandem: Tandem.OPT_OUT } ); - const intervalToolNode = new IntervalToolNode( intervalTool, transform, { + const intervalToolNode = new IntervalToolNode( intervalTool, new BooleanProperty( true ), transform, { isIcon: true, visibleProperty: new BooleanProperty( true ), tandem: Tandem.OPT_OUT Index: js/measures/view/MeasuresScreenView.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/measures/view/MeasuresScreenView.ts b/js/measures/view/MeasuresScreenView.ts --- a/js/measures/view/MeasuresScreenView.ts (revision 92fb76da9ae8cd0cd9857f23f8bd77604faca31d) +++ b/js/measures/view/MeasuresScreenView.ts (date 1711405255934) @@ -87,7 +87,7 @@ super( model, launchPanel, staticToolPanel, interactiveToolPanel, createHistogramNode, options ); - const intervalToolNode = new IntervalToolNode( model.intervalTool, this.modelViewTransform, { + const intervalToolNode = new IntervalToolNode( model.intervalTool, model.isIntervalToolVisibleProperty, this.modelViewTransform, { isIcon: false, visibleProperty: model.isIntervalToolVisibleProperty, tandem: options.tandem.createTandem( 'intervalToolNode' ) ```
samreid commented 6 months ago

Perhaps the crashing was corrected in https://github.com/phetsims/projectile-data-lab/issues/274

samreid commented 6 months ago

After the fix in https://github.com/phetsims/projectile-data-lab/issues/274 this patch worked well, so I committed it. @Nancy-Salpepi can you please verify in main? Please close if all is well.

Nancy-Salpepi commented 6 months ago

I no longer hear the interval tool sound when I follow the original steps. However, with the iPad and with mac + safari I do hear it briefly with Reset All. There is also a new separate issue with the default location of the interval tool on Reset All (I will make an separate issue for this)

  1. With mac + safari: On the Measures screen, check the Interval Tool
  2. Move the Interval Tool all the way to the right
  3. Press Reset All
matthew-blackman commented 6 months ago

This has been fixed in the commit and is ready to test. @Nancy-Salpepi feel free to close if thing are working well.

Nancy-Salpepi commented 6 months ago

Sounds good on main!