phetsims / sun

User-interface components for PhET simulations, built on top of Scenery.
http://scenerystack.org/
MIT License
5 stars 12 forks source link

CT Error: Pointer should be assigned #875

Closed jessegreenberg closed 8 months ago

jessegreenberg commented 9 months ago

I am seeing this in fourier-making-waves, but do not believe it is a sim problem. Seems to only happen for test ?: interactive-description-fuzz-fuzzBoard-combo.

fourier-making-waves : interactive-description-fuzz-fuzzBoard-combo : unbuilt
http://128.138.93.172/continuous-testing/ct-snapshots/1708705610343/fourier-making-waves/fourier-making-waves_en.html?continuousTest=%7B%22test%22%3A%5B%22fourier-making-waves%22%2C%22interactive-description-fuzz-fuzzBoard-combo%22%2C%22unbuilt%22%5D%2C%22snapshotName%22%3A%22snapshot-1708705610343%22%2C%22timestamp%22%3A1708706109408%7D&brand=phet&ea&supportsInteractiveDescription=true&fuzz&fuzzBoard
Query: brand=phet&ea&supportsInteractiveDescription=true&fuzz&fuzzBoard
Uncaught Error: Assertion failed: Pointer should be assigned
Error: Assertion failed: Pointer should be assigned
at window.assertions.assertFunction (http://128.138.93.172/continuous-testing/ct-snapshots/1708705610343/assert/js/assert.js:28:13)
at assert (AccessibleValueHandler.ts:990:16)
at _onInteractionEnd (AccessibleValueHandler.ts:863:17)
at inputEvent (Input.ts:1907:69)
at dispatchToListeners (Input.ts:1947:11)
at dispatchToTargets (Input.ts:1857:9)
at dispatchEvent (Input.ts:1123:11)
at dispatchPDOMEvent (Input.ts:822:20)
at apply (PhetioAction.ts:162:16)
at execute (Input.ts:1613:21)

There are many components using AccessibleSlider in that sim, that may be why it is only showing up there for now.

This assertion was added for changes from https://github.com/phetsims/scenery/issues/1596.

jessegreenberg commented 9 months ago

Here is a more detailed stack trace after hitting this locally:

image

Event target is an amplitude slider.

EDIT: THis patch helps reproduce the problem consistently in fourier.

```patch Subject: [PATCH] Indicate that items have been sorted, see https://github.com/phetsims/scenery-phet/issues/815 --- Index: sun/js/accessibility/AccessibleValueHandler.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/sun/js/accessibility/AccessibleValueHandler.ts b/sun/js/accessibility/AccessibleValueHandler.ts --- a/sun/js/accessibility/AccessibleValueHandler.ts (revision 52809d1359f3b4ee8e121b3b7ae390102f8a51c6) +++ b/sun/js/accessibility/AccessibleValueHandler.ts (date 1708712005565) @@ -967,6 +967,9 @@ * when generating the context response with option a11yCreateContextResponse. */ private _onInteractionStart( event: SceneryEvent ): void { + + console.log( 'ON INTERACTION START' ); + this._pdomPointer = event.pointer as PDOMPointer; this._pdomPointer.addInputListener( this._pdomPointerListener, true ); @@ -982,6 +985,13 @@ */ private _onInteractionEnd( event: SceneryEvent | null ): void { + // could be getting here twice + // could be getting here without an onInteractionStart + + // it may be possible for the fuzzer to send two keyboard events in quick succession + // then the keyup would happen in the same frame? + + console.log( 'ON INTERACTION END' ); this.alertContextResponse(); this.voicingOnEndResponse( this._valueOnStart ); this._endInput( event ); Index: scenery/js/accessibility/pdom/PDOMUtils.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/scenery/js/accessibility/pdom/PDOMUtils.js b/scenery/js/accessibility/pdom/PDOMUtils.js --- a/scenery/js/accessibility/pdom/PDOMUtils.js (revision 228a62aad5a47428f3f05a693656f9ebfa3df912) +++ b/scenery/js/accessibility/pdom/PDOMUtils.js (date 1708711856152) @@ -301,7 +301,14 @@ const linearDOM = getLinearDOMElements( document.body ); const focusableElements = []; for ( let i = 0; i < linearDOM.length; i++ ) { - PDOMUtils.isElementFocusable( linearDOM[ i ] ) && focusableElements.push( linearDOM[ i ] ); + PDOMUtils.isElementFocusable( linearDOM[ i ] ) && + focusableElements.push( linearDOM[ i ] ); + } + + // if there are range inputs, return the first one + const rangeInputs = focusableElements.filter( element => element.tagName === 'INPUT' && element.type === 'range' ); + if ( rangeInputs.length > 0 ) { + return rangeInputs[ 0 ]; } return focusableElements[ random.nextInt( focusableElements.length ) ]; ```

I can see that we are getting two calls to onInteractionEnd in a row: image

The first event is null, indicating it comes from an interruption. And it is the first interrupt from what I can see in the console, so that is definitely the issue.

The interrupt comes from a press of ResetAllButton. I think this is what is happening: 1) Fuzzer sends arrow key down to slider 2) Fuzzer sends mouse press down on reset all button 3) reset all button interrupts subtree input 4) onInteractionEnd is called with null event, clearing the pointer reference 5) The fuzzer sends the keyup event to the slider, attempting onInteractionEnd again and triggering the assertion.

This is only possible with the fuzzer because its not possible for keyup events to go to the slider after pressing the reset all button with normal usage.

jessegreenberg commented 8 months ago

The above commit should fix this. Its now graceful so that if there is a keyup event after an interrupt, we don't try to do "interaction end" work twice.

I can no longer hit this assertion locally and I let ~30 sims run through local fuzzing with ?fuzz&fuzzBoard.

Ill let CT run with this change for a few columns before closing.

jessegreenberg commented 8 months ago

CT has run for more than 20 columns without this error. It previously could be seen every few columns. Closing.