Support sliders with voicing in correlation with AccessibleValueHandler

[zepumph]

We need to support voicing in cases where we are using AccessibleValueHandler (often via AccessibleSlider). I tried to name this issue such that it didn't automatically sound like we were going to implement Voicing directly into AccessibleValueHandler. I don't know if that is the best approach. In general, sliders need a smooth API for adding voicing to them, and it will largely overlap with AccessibleValueHandler because voicing utilized alternative input (supported via AccessibleValueHandler). One context here is that in RAP, I am adding voicing to the hand sliders (https://github.com/phetsims/ratio-and-proportion/issues/413). These Nodes mix AccessibleSlider, but currently have a custom implementation to support voicing. Is there a way that we can align these APIs via common code design?

It is also important to note that voiced sliders will always support mouse/touch in addition to alt input, so we need to think more broadly than AccessibleValueHandler, even if we utilize it to achieve some of the voicing api for sliders.

I think the best place to start is with a conversation with @jessegreenberg, marking for meeting.

[zepumph]

@jessegreenberg and I spoke about this last Friday. It isn't totally obvious what the best solution is, but we decided that it was worth trying to add in Voicing to AccessibleValueHandler, and to try create public methods that act as the rule-based "events" that sliders have for mouse/touch/keyboard interactions. We can call them privately to handle keyboard, and expect that for mouse input, the voicing design pattern is to use the AccessibleValueHandler functions instead of calling upon the Voicing mixin directly.

From our meeting, it wasn't totally clear if the complexity of adding voicing directly to AccessibleValueHandler was worth the benefit. Right now, the "benefit" is another way of doing the same thing for voicing. I still think it is worth a try though, so I will work on this as part of https://github.com/phetsims/ratio-and-proportion/issues/413

[zepumph]

```diff Index: js/accessibility/AccessibleValueHandler.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/accessibility/AccessibleValueHandler.js b/js/accessibility/AccessibleValueHandler.js --- a/js/accessibility/AccessibleValueHandler.js (revision e7bf7bea865a781b7a8bff476b694aee8d1f25ad) +++ b/js/accessibility/AccessibleValueHandler.js (date 1639174240063) @@ -23,9 +23,7 @@ import inheritance from '../../../phet-core/js/inheritance.js'; import merge from '../../../phet-core/js/merge.js'; import Orientation from '../../../phet-core/js/Orientation.js'; -import { KeyboardUtils } from '../../../scenery/js/imports.js'; -import { animatedPanZoomSingleton } from '../../../scenery/js/imports.js'; -import { Node } from '../../../scenery/js/imports.js'; +import { animatedPanZoomSingleton, KeyboardUtils, Node, Voicing } from '../../../scenery/js/imports.js'; import Utterance from '../../../utterance-queue/js/Utterance.js'; import sun from '../sun.js'; @@ -47,6 +45,9 @@ const proto = type.prototype; + // compose with Interactive Highlights, all Nodes with Voicing features highlight as they are interactive + Voicing.compose( type ); + extend( proto, { /** @@ -212,6 +213,8 @@ this.mutate( optionsToMutate ); + this.initializeVoicing( options ); + // @private {Property.} this._valueProperty = valueProperty; @@ -401,6 +404,7 @@ newAriaValueText = this.ariaValueText + hairSpace; } + this.voicingObjectResponse = newAriaValueText; this.ariaValueText = newAriaValueText; }, @@ -773,6 +777,7 @@ */ onInteractionStart( event ) { this.valueOnStart = this._valueProperty.value; + this.voicingStartResponse(); this._startChange( event ); }, @@ -785,6 +790,7 @@ */ onInteractionEnd( event ) { this.alertContextResponse(); + this.voicingChangeResponse(); this._endChange( event ); }, @@ -964,7 +970,31 @@ } this.setPDOMAttribute( 'step', stepValue ); - } + }, + + /** + * Hook to trigger a context response. For sliders it is best to call this. . . . + */ + voicingFocusResponse() { + + }, + + voicingStartResponse(){ + this.position= + }, + voicingChangeResponse( ...args){ + + // @ts-ignore + this.voicingObjectResponse = this.voicingCreateObjectResponse(); + // @ts-ignore + this.voicingContextResponse = this.voicingCreateContextResponse(position, oldValue, ...args); + + this.voicingSpeakResponse( { + objectResponse: this.voicingObjectResponse, + contextResponse: this.voicingContextResponse + } ); + + } // drag or endDrag, whatever is best for uuu } ); } }; ```

[zepumph]

The first step here was to preemptively handle the issue I ran into over in https://github.com/phetsims/scenery/issues/1283#issuecomment-992800551, where many times we were mutating voicing options before initializing voicing. If we mix Voicing into AccessibleValueHandler as part of that work, we will want to make sure we don't hit that, so I refactored mutate calls above.

[samreid]

This is failing several sims on CT, increasing priority.

The failure is

Node.ts:5896 Uncaught TypeError: this._enabledProperty.initializePhetio is not a function
    at BLLFaucetNode.initializePhetioObject (Node.ts:5896)
    at BLLFaucetNode.mutate (Node.ts:5856)
    at new FaucetNode (FaucetNode.js? [sm]:286)
    at new BLLFaucetNode (BLLFaucetNode.js? [sm]:43)
    at new ConcentrationScreenView (ConcentrationScreenView.js? [sm]:83)
    at ConcentrationScreen.createView (ConcentrationScreen.js? [sm]:40)
    at ConcentrationScreen.initializeView (Screen.js? [sm]:254)
    at Array.<anonymous> (Sim.js? [sm]:773)
    at Sim.js? [sm]:781

Reverting https://github.com/phetsims/scenery-phet/commit/486cf2be4a28a6187888c7e140b0aaf66cb9b6e8 prevents the problem.

Another CT error that appeared at the same time is:

graphing-quadratics : phet-io-fuzz : unbuilt
https://bayes.colorado.edu/continuous-testing/ct-snapshots/1639496160983/graphing-quadratics/graphing-quadratics_en.html?continuousTest=%7B%22test%22%3A%5B%22graphing-quadratics%22%2C%22phet-io-fuzz%22%2C%22unbuilt%22%5D%2C%22snapshotName%22%3A%22snapshot-1639496160983%22%2C%22timestamp%22%3A1639499574034%7D&ea&brand=phet-io&phetioStandalone&fuzz&memoryLimit=1000
Query: ea&brand=phet-io&phetioStandalone&fuzz&memoryLimit=1000
Uncaught Error: Assertion failed: this means addLinkedElement was called before instrumentation of this PhetioObject
Error: Assertion failed: this means addLinkedElement was called before instrumentation of this PhetioObject
at window.assertions.assertFunction (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1639496160983/assert/js/assert.js:25:13)
at NumberPicker.initializePhetioObject (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1639496160983/chipper/dist/js/tandem/js/PhetioObject.js:173:15)
...

Which seems like it may relate to the order of Node instrumentation (in the mutate call), so should be investigated as part of this issue as well.

[zepumph]

Two fixes here. The enabledProperty one was sneaky, we have been overwriting Node's enabledProperty in AccessibleValueProperty for god know's how long, but since we instrumented it first (mutate before initializeAccessibleSlider), it didn't cause problems. I sorted it out with a rename.

[zepumph]

Still working here:

```diff Index: sun/js/accessibility/AccessibleValueHandler.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/sun/js/accessibility/AccessibleValueHandler.js b/sun/js/accessibility/AccessibleValueHandler.js --- a/sun/js/accessibility/AccessibleValueHandler.js (revision 3099d1921fd214af05ddd2ab0d35ec18276c3b95) +++ b/sun/js/accessibility/AccessibleValueHandler.js (date 1639582105028) @@ -23,9 +23,7 @@ import inheritance from '../../../phet-core/js/inheritance.js'; import merge from '../../../phet-core/js/merge.js'; import Orientation from '../../../phet-core/js/Orientation.js'; -import { KeyboardUtils } from '../../../scenery/js/imports.js'; -import { animatedPanZoomSingleton } from '../../../scenery/js/imports.js'; -import { Node } from '../../../scenery/js/imports.js'; +import { animatedPanZoomSingleton, KeyboardUtils, Node, Voicing } from '../../../scenery/js/imports.js'; import Utterance from '../../../utterance-queue/js/Utterance.js'; import sun from '../sun.js'; @@ -47,6 +45,9 @@ const proto = type.prototype; + // compose with Interactive Highlights, all Nodes with Voicing features highlight as they are interactive + Voicing.compose( type ); + extend( proto, { /** @@ -191,7 +192,10 @@ * should list any Properties who's change should trigger description update for this Node. * @type {Property[]} */ - a11yDependencies: [] + a11yDependencies: [], + + voicingCreateObjectResponse: null, + voicingCreateContextResponse: null, }; options = merge( {}, defaults, options ); @@ -212,6 +216,8 @@ this.mutate( optionsToMutate ); + this.initializeVoicing( options ); + // @private {Property.} this._valueProperty = valueProperty; @@ -313,6 +319,9 @@ this.setA11yDependencies( options.a11yDependencies ); + this._voicingCreateObjectResponse = options.voicingCreateObjectResponse; + this._voicingCreateContextResponse = options.voicingCreateContextResponse; + // listeners, must be unlinked in dispose const enabledRangeObserver = enabledRange => { @@ -773,6 +782,7 @@ */ onInteractionStart( event ) { this.valueOnStart = this._valueProperty.value; + this.voicingStartResponse(); this._startChange( event ); }, @@ -785,6 +795,7 @@ */ onInteractionEnd( event ) { this.alertContextResponse(); + this.voicingChangeResponse(); this._endChange( event ); }, @@ -964,7 +975,38 @@ } this.setPDOMAttribute( 'step', stepValue ); - } + }, + + voicingStartResponse() { + this.position =; + + this.voicingObjectResponse = this.voicingCreateObjectResponse(); + + this.voicingSpeakFullResponse( { + contextResponse: null + } ); + }, + voicingOnEndResponse() { + + // @ts-ignore + this.ratioHandNode.voicingSpeakFullResponse( { + nameResponse: null + } ); + }, + voicingChangeResponse( ...args ) { + + // @ts-ignore + this.voicingObjectResponse = this.voicingCreateObjectResponse(); + // @ts-ignore + this.voicingContextResponse = this.voicingCreateContextResponse(); + + + this.voicingSpeakResponse( { + objectResponse: this.voicingObjectResponse, + contextResponse: this.voicingContextResponse + } ); + + } // drag or endDrag, whatever is best for uuu } ); } }; Index: ratio-and-proportion/js/common/view/RatioHandNode.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/ratio-and-proportion/js/common/view/RatioHandNode.ts b/ratio-and-proportion/js/common/view/RatioHandNode.ts --- a/ratio-and-proportion/js/common/view/RatioHandNode.ts (revision 339c71d7dc9f640251d5c9a66f4eb562aa9bb96b) +++ b/ratio-and-proportion/js/common/view/RatioHandNode.ts (date 1639509476873) @@ -89,16 +89,14 @@ }, options ); super(); - // @ts-ignore, TODO, redundant with initializeAccessibleSlider https://github.com/phetsims/sun/issues/730 - this.initializeVoicing(); - Property.multilink( [ valueProperty ].concat( options.a11yDependencies ), () => { - - // @ts-ignore - this.voicingObjectResponse = options.voicingCreateObjectResponse(); - // @ts-ignore - this.voicingContextResponse = options.voicingCreateContextResponse(); - } ); + // Property.multilink( [ valueProperty ].concat( options.a11yDependencies ), () => { + // + // // @ts-ignore + // this.voicingObjectResponse = options.voicingCreateObjectResponse(); + // // @ts-ignore + // this.voicingContextResponse = options.voicingCreateContextResponse(); + // } ); // Always the same range, always enabled // @ts-ignore @@ -307,7 +305,7 @@ // TODO: once Voicing is in typescript we can remove this, https://github.com/phetsims/ratio-and-proportion/issues/404 // @ts-ignore -Voicing.compose( RatioHandNode as unknown as new () => RatioHandNode ); +// Voicing.compose( RatioHandNode as unknown as new () => RatioHandNode ); ratioAndProportion.register( 'RatioHandNode', RatioHandNode ); export default RatioHandNode; \ No newline at end of file Index: ratio-and-proportion/js/common/view/RatioHalf.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/ratio-and-proportion/js/common/view/RatioHalf.ts b/ratio-and-proportion/js/common/view/RatioHalf.ts --- a/ratio-and-proportion/js/common/view/RatioHalf.ts (revision 339c71d7dc9f640251d5c9a66f4eb562aa9bb96b) +++ b/ratio-and-proportion/js/common/view/RatioHalf.ts (date 1639509687844) @@ -367,11 +367,6 @@ options.setJumpingOverProportionShouldTriggerSound( true ); viewSounds.boundarySoundClip.onStartInteraction(); - - // @ts-ignore - this.ratioHandNode.voicingSpeakFullResponse( { - contextResponse: null - } ); }, drag: () => { this.isBeingInteractedWithProperty.value = true; @@ -408,10 +403,7 @@ // @ts-ignore this.ratioHandNode.alertContextResponse(); - // @ts-ignore - this.ratioHandNode.voicingSpeakFullResponse( { - nameResponse: null - } ); + this.ratioHandNode.voicingOnEndResponse(); }, // phet-io

[zepumph]

I sorta gave up on creating an ideal, general API, in part because the design for sliders may change on a case by case basis, and complexity may need to be added. In the mean time, I worked on https://github.com/phetsims/ratio-and-proportion/issues/413 and put the code needed into AccessibleValueHandler. Above is the addition of Voicing into AccessibleValueHandler, along with new functions that signify events (end/change) to help generalize the life cycle of voicing a slider. I think these will be able to be reused in other sims, but perhaps with some tweaks. It isn't perfect, but in general these methods are called in the keyboard listeners internally, and publicly by the mouse dragListener. I'll discuss with designer more on the sim-specific design, and then come back here.

[zepumph]

I had to make sure to remove other usages of initializeVoicing where we are also extending AccessibleValueHandler. @jessegreenberg, is it alright to mutate the options in AppendageNode at the end of the constructor?

[jessegreenberg]

This is looking really nice! I moved the mutate to the end in AppendageNode and removed the extra initializeVoicing in both john-travoltage and inverse-square-law-common and gravity-force-lab-basics and tested, it is working well.

in general these methods are called in the keyboard listeners internally, and publicly by the mouse dragListener.

It looks like so far voicingOnEndResponse is the only one called internally, is that right?

[zepumph]

Yes that's right, because focus occurs by default on focus.

I cleaned up the rest of the usages of Voicing in those three files. Sound alright?

[zepumph]

https://github.com/phetsims/ratio-and-proportion/issues/413 has been signed off on, so we need to clean up this implementation in common code. We went with an approach that had custom implementations for Mouse/Touch that didn't use AccessibleValueHandler.

[zepumph]

Cleanup done above. I removed the "opt in" approach, but this didn't seem to effect the voicing I was hearing in GFLB or JT, so I committed.

[zepumph]

Subset of https://github.com/phetsims/scenery/issues/1298

[jessegreenberg]

@zepumph can this be closed now? Slider + Voicing work also happened in https://github.com/phetsims/joist/issues/854.