CT: must be connected to a display to use UtteranceQueue features

[jessegreenberg]

waves-intro : multitouch-fuzz : unbuilt
https://bayes.colorado.edu/continuous-testing/ct-snapshots/1668173012434/waves-intro/waves-intro_en.html?continuousTest=%7B%22test%22%3A%5B%22waves-intro%22%2C%22multitouch-fuzz%22%2C%22unbuilt%22%5D%2C%22snapshotName%22%3A%22snapshot-1668173012434%22%2C%22timestamp%22%3A1668176785153%7D&brand=phet&ea&fuzz&fuzzPointers=2&memoryLimit=1000&supportsPanAndZoom=false
Query: brand=phet&ea&fuzz&fuzzPointers=2&memoryLimit=1000&supportsPanAndZoom=false
Uncaught Error: Assertion failed: must be connected to a display to use UtteranceQueue features
Error: Assertion failed: must be connected to a display to use UtteranceQueue features
at window.assertions.assertFunction (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1668173012434/assert/js/assert.js:28:13)
at assert (ParallelDOM.ts:2635:14)
at alertDescriptionUtterance (PreferencesToggleSwitch.ts:122:13)
at listener (TinyEmitter.ts:95:8)
at emit (ReadOnlyProperty.ts:311:22)
at _notifyListeners (ReadOnlyProperty.ts:262:13)
at unguardedSet (ReadOnlyProperty.ts:246:11)
at set (Property.ts:54:10)
at (ToggleSwitch.ts:212:20)
at apply (PhetioAction.ts:125:16)
id: Bayes Puppeteer
Snapshot from 11/11/2022, 8:23:32 AM

[jessegreenberg]

Showing up in several sims, molarity, gravity-force-lab-basics, waves-intro, probably others.

[jessegreenberg]

I have not yet been able to reproduce with fuzz testing locally.

The assertion hit in this case is in ParallelDOM.alertDescriptionUtterance https://github.com/phetsims/scenery/blob/0a57ff86efd38daf0fdfbcc89cbf332e39982764/js/accessibility/pdom/ParallelDOM.ts#L2631-L2636

The call to that function is in a listener on a Property, linked in PreferencesToggleSwitch. If the Property changes after the PreferencesToggleSwitch is removed from the display we will hit this.

[jessegreenberg]

I let molarity, color-vision, gravity-force-lab-basics, and waves-intro fuzz for 20 minutes and did not hit this error. But surprisingly, I found a way to reproduce easily.

Open the PreferencesDialog to a tab with a PreferencesToggleSwitch With one finger press on a PreferencesToggleSwitch With another finger tap outside of the dialog, it will close Release the first finger -> this will cause the assertion

[jessegreenberg]

I found this is happening because the listener on from the ToggleSwitch is still attached to the Pointer after the Dialog is closed from another input. I think this is fixed if we add an interruptSubtreeInput() before hiding the Popupable.

[jessegreenberg]

OK, that does fix the problem. @kathy-phet can you please assign a reviewer for a change in Popupable? Listed authors are

@author Jonathan Olson <jonathan.olson@colorado.edu>
@author Sam Reid (PhET Interactive Simulations)
@author Andrea Lin (PhET Interactive Simulations)
@author Chris Malley (PixelZoom, Inc.)

[jessegreenberg]

Over slack @samreid said

That sounds correct to me

This was a one-line addition, maybe that is sufficient for review?

[samreid]

I reviewed the commit and wrote up a way to get rid of the as unknown as parts. Can you please review?

```diff Index: js/Popupable.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/Popupable.ts b/js/Popupable.ts --- a/js/Popupable.ts (revision 9f22e81fbf572672f06cc456c2c6ad75a1522746) +++ b/js/Popupable.ts (date 1668191064912) @@ -43,8 +43,7 @@ }; export type PopupableOptions = SelfOptions & PickOptional; -const Popupable = ( type: SuperType, optionsArgPosition: number ) => { // eslint-disable-line @typescript-eslint/explicit-module-boundary-types - assert && assert( _.includes( inheritance( type ), Node ), 'Only Node subtypes should mix Popupable' ); +const Popupable = >( type: SuperType, optionsArgPosition: number ) => { // eslint-disable-line @typescript-eslint/explicit-module-boundary-types return class extends type { @@ -98,7 +97,7 @@ this._focusOnShowNode = options.focusOnShowNode; this._focusOnHideNode = options.focusOnHideNode; this._nodeToFocusOnHide = null; - this.popupParent = new PopupParentNode( this as unknown as Node, { + this.popupParent = new PopupParentNode( this, { show: this.show.bind( this ), hide: this.hide.bind( this ), layout: this.layout.bind( this ) @@ -143,7 +142,7 @@ * Hide the popup. If you create a new popup next time you show(), be sure to dispose this popup instead. */ public hide(): void { - ( this as unknown as Node ).interruptSubtreeInput(); + this.interruptSubtreeInput(); this.isShowingProperty.value = false; @@ -156,7 +155,7 @@ /** * Releases references */ - public dispose(): void { + public override dispose(): void { this.hide(); this.isShowingProperty.dispose(); ```

Also, can you please describe a scenario where it is easy to test this change? Is there a Popupable dialog with a slider or something draggable, for instance?

[jessegreenberg]

That is awesome, I pushed the change. Id like to do that for our other traits too. See https://github.com/phetsims/scenery/issues/1498

an you please describe a scenario where it is easy to test

Yes, the case were I could easily break things before this change is in https://github.com/phetsims/joist/issues/881#issuecomment-1311976590.

[samreid]

I reverted https://github.com/phetsims/sun/commit/9f22e81fbf572672f06cc456c2c6ad75a1522746, cleared my mobile safari cache and ran Gravity and Orbits with ?ea. I was able to continue dragging the projector mode toggle button after dismissing the dialog, but the sim did not crash. I was not connected to remote debugging. So it was puzzling why the sim didn't crash.

Then I applied https://github.com/phetsims/sun/commit/9f22e81fbf572672f06cc456c2c6ad75a1522746 and cleared my mobile cache and saw that dragging the projector mode toggle button no longer continued after dismissing the dialog, so this seems like a good solution.

The commit seems good, so I think this issue can be closed.