Add Properties to Utterance to control Voicing and Description response output

[jessegreenberg]

We currently have Utterance.canAnnounceProperty which prevents the Utterance from being announced by the UtteranceQueue when it is false. The problem is that when an Utterance is shared and used for both Voicing and Interactive Description (such as by an Alerter), if Voicing.canSpeakProperty (forwarded to the Utterance.canAnnounceProperty) is false content will not be heard for Description of Voicing.

To fix this, we need a dedicated Property to control the output of each feature.

• Utterance.canVoiceProperty
  • Whether the Utterance will be announced for Voicing features
  • Utterance.canDescribeProperty
  • Whether the Utterance will be announced for Interactive Description
  • Utterance.canAnnounceProperty
  • Whether the Utterance will be announced for any feature.

UtteranceQueue will check the state of these Properties an attemptToAnnounce with something like

const canAnnounce = !this.utteranceCanAnnounceAccessor ? utterance.canAnnounceProperty.value : ( utterance.canAnnounceProperty.value && utterance[ this.utteranceCanAnnounceAccessor ].value );

We liked this way the best because Utterances can still be used in more than once UtteranceQueue, and keeping the canAnnounceProperty (for general use) along with the other two new ones (for PhET use).

Some brainstorming notes from the meeting where we thought about other options

// 1) Use different Utterances for Voicing/Description (cannot be shared in Alerters) // We really really don't want this!!!!1!1 // 2) canAnnounceProperty of Utterance either // a) toggleable for the UtteranceQueue // b) Specific to Voicing // 3) Break canAnnounceProperty into canVoiceProperty and canAnnounceProperty, both checked in UtteranceQueue. // - canVoiceProperty (implemented from canAnnounceProperty) // - canDescriptionAlertProperty (name tbd, but canDescribeProperty was proposed) // Implemented the same as old canAnnounceProperty // Utterance.useCase // nooo..., do this instead utterance[ this.utteranceCanAnnounceAccessor ].value // Utterance.canVoiceProperty // Utterance.canDescribeProperty // Utterance.canAnnounceProperty

@zepumph offered to do this as part of a review for the collection of canAnnounceProperty issues.

EDIT: Logic in above snippet could potentially be simplified:

const canAnnounce = !this.utteranceCanAnnounceAccessor || utterance[ this.utteranceCanAnnounceAccessor ].value;

[zepumph]

• [x] When complete, take https://github.com/phetsims/friction/issues/296 off hold.

[zepumph]

This is blocking description in Friction and Quad, and possibly others, I'll try to have it done today.

[zepumph]

One quick patch

```diff Index: js/Utterance.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/Utterance.ts b/js/Utterance.ts --- a/js/Utterance.ts (revision 40358fc6ac3dbf7ca933b20af23697b0eae4d2c3) +++ b/js/Utterance.ts (date 1654545943410) @@ -19,7 +19,7 @@ */ import DerivedProperty from '../../axon/js/DerivedProperty.js'; -import DynamicProperty from '../../axon/js/DynamicProperty.js'; +import DynamicProperty, { DynamicPropertyOptions } from '../../axon/js/DynamicProperty.js'; import NumberProperty from '../../axon/js/NumberProperty.js'; import optionize from '../../phet-core/js/optionize.js'; import IOType from '../../tandem/js/types/IOType.js'; @@ -92,18 +92,9 @@ id: number; private _alert: AlertableNoUtterance; - // List of Properties that must all be true in order for the Utterance to be announced by the Announcer. - private _canAnnounceProperties: IProperty[]; - - // A Property for the DynamicProperty. The value of this Property is the DerivedProperty.and of all - // canAnnounceProperties. The benefit of using a DynamicProperty is that dependency Properties of the - // implementation can change (new DerivedProperty in setCanAnnounceProperties) but the listeners will remain - // unaffected on the canAnnounceProperty. - private readonly canAnnounceImplementationProperty: Property>; - // If the value of this Property is false, this Utterance will never be announced by an Announcer. See // documentation for canAnnounceImplementationProperty for implementation details and why we use a DynamicProperty. - public readonly canAnnounceProperty: DynamicProperty>; + public readonly canAnnounceProperty: AnnouncingControlProperty; // (utterance-queue-internal) readonly predicate: () => boolean; @@ -142,10 +133,9 @@ this.predicate = options.predicate; - this._canAnnounceProperties = []; - this.canAnnounceImplementationProperty = new Property>( new TinyProperty( false ) ); - this.canAnnounceProperty = new DynamicProperty>( this.canAnnounceImplementationProperty ); - this.setCanAnnounceProperties( options.canAnnounceProperties ); + this.canAnnounceProperty = new AnnouncingControlProperty( { + dependentProperties: options.canAnnounceProperties + } ); this.alertStableDelay = options.alertStableDelay; @@ -230,17 +220,7 @@ * this.canAnnounceProperty. */ public setCanAnnounceProperties( canAnnounceProperties: IProperty[] ): void { - if ( this.canAnnounceImplementationProperty.value ) { - this.canAnnounceImplementationProperty.value.dispose(); - } - - // If no canAnnounceProperties provided, use a dummy Property that will always allow this Utterance to announce. - const dependencyProperties = canAnnounceProperties.length === 0 ? [ new TinyProperty( true ) ] : canAnnounceProperties; - - const canSpeakProperty = DerivedProperty.and( dependencyProperties ); - this.canAnnounceImplementationProperty.value = canSpeakProperty; - - this._canAnnounceProperties = canAnnounceProperties; + this.canAnnounceProperty.setDependentProperties( canAnnounceProperties ); } set canAnnounceProperties( canAnnounceProperties: IProperty[] ) { this.setCanAnnounceProperties( canAnnounceProperties ); } @@ -252,19 +232,16 @@ * All must be true for the announcement to occur. */ public getCanAnnounceProperties(): IProperty[] { - return this._canAnnounceProperties.slice( 0 ); // defensive copy + return this.canAnnounceProperty.getDependentProperties(); } /** * Make eligible for garbage collection. */ public dispose(): void { - this.canAnnounceImplementationProperty.dispose(); this.canAnnounceProperty.dispose(); this.priorityProperty.dispose(); - - this._canAnnounceProperties = []; } /** @@ -309,5 +286,75 @@ } ); } + +type AnnouncingControlPropertySelfOptions = { + dependentProperties?: IProperty[]; +} + +type AnnouncingControlPropertyParentOptions = DynamicPropertyOptions>; +type AnnouncingControlPropertyOptions = AnnouncingControlPropertySelfOptions & AnnouncingControlPropertyParentOptions; + +class AnnouncingControlProperty extends DynamicProperty> { + + // List of Properties that must all be true in order for the Utterance to be announced by the Announcer. + private _dependentProperties: IProperty[]; + + // A Property for the DynamicProperty. The value of this Property is the DerivedProperty.and of all + // canAnnounceProperties. The benefit of using a DynamicProperty is that dependency Properties of the + // implementation can change (new DerivedProperty in setCanAnnounceProperties) but the listeners will remain + // unaffected on the canAnnounceProperty. + private readonly implementationProperty: Property>; + + constructor( providedOptions?: AnnouncingControlPropertyOptions ) { + + const options = optionize()( { + dependentProperties: [] + }, providedOptions ); + + const implementationProperty = new Property>( new TinyProperty( false ) ); + + super( implementationProperty ); + + this._dependentProperties = []; + this.implementationProperty = implementationProperty; + this.setDependentProperties( options.dependentProperties ); + } + + /** + * Set the Properties controlling this Property's value. All Properties must be true for this Property to be true. + */ + public setDependentProperties( dependentProperties: IProperty[] ): void { + if ( this.implementationProperty.value ) { + this.implementationProperty.value.dispose(); + } + + // If no dependentProperties provided, use a dummy Property that will always allow this Utterance to announce. + const dependencyProperties = dependentProperties.length === 0 ? [ new TinyProperty( true ) ] : dependentProperties; + + this.implementationProperty.value = DerivedProperty.and( dependencyProperties ); + + this._dependentProperties = dependentProperties; + } + + + set dependentProperties( dependentProperties: IProperty[] ) { this.setDependentProperties( dependentProperties ); } + + get dependentProperties() { return this.getDependentProperties(); } + + /** + * Get the Properties that control whether the alert content for this Utterance can be announced. + * All must be true for the announcement to occur. + */ + public getDependentProperties(): IProperty[] { + return this._dependentProperties.slice( 0 ); // defensive copy + } + + override dispose(): void { + this.implementationProperty.dispose(); + this._dependentProperties = []; + super.dispose(); + } +} + utteranceQueueNamespace.register( 'Utterance', Utterance ); export default Utterance; \ No newline at end of file ```

[zepumph]

Ok, this was pretty straight forward once we factored out AnnouncingControlProperty. I am happy with the way it looks. I have confirmed that friction is fixed. @jessegreenberg, can you please review the implementation, and note that currently canAnnounceProperties and descriptionCanAnnounceProperties are not used at all in the project.

[jessegreenberg]

Things are looking good, well done on the abstraction of AnnouncingControlProperty. Updates to Voicing.ts look good.

It isn't working quite yet though. I hear voicing responses when voicingVisible if false. I think because UtteranceQueue wasn't updated to check on the new Properties here: https://github.com/phetsims/utterance-queue/blob/9a061765a425d1304ec70d0a8d6ee16fe7ffe6e5/js/UtteranceQueue.ts#L563-L565

[zepumph]

Oops! I'll take a look. Sorry about that.

[zepumph]

Ok, tested and ready for actual review.

[zepumph]

Disposal calls added above.

[jessegreenberg]

I tested this by watching responses in the a11y view and with ?printVoicingResponses and saw that this is working very well.

For my own understanding, can you describe the typing here

class Utterance implements FeatureSpecificAnnouncingControlPropertySupported {

How/why does Utterance implement FeatureSpecificAnnouncingControlPropertySupported? This enforces that Utterances has those fields?

Otherwise everything seems great here.

[zepumph]

The key feature is that we want the type FeatureSpecificAnnouncingControlProperty that UtteranceQueue can use. Having Utterance implement FeatureSpecificAnnouncingControlPropertySupported (the object) ensures that Utterance has those keys. Note the error when you commend out descriptionCanAnnounceProperty.

Index: js/Utterance.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/Utterance.ts b/js/Utterance.ts
--- a/js/Utterance.ts   (revision 7780764fe8ed6004ff75b2831c172a4f6b2093a0)
+++ b/js/Utterance.ts   (date 1656607778624)
@@ -115,7 +115,7 @@
   public readonly canAnnounceProperty: AnnouncingControlProperty;

   // If the value of this Property is false, this Utterance will never be announced by AriaLiveAnnouncer.
-  public readonly descriptionCanAnnounceProperty: AnnouncingControlProperty;
+  // public readonly descriptionCanAnnounceProperty: AnnouncingControlProperty;

   // If the value of this Property is false, this Utterance will never be announced by SpeechSynthesisAnnouncer.
   public readonly voicingCanAnnounceProperty: AnnouncingControlProperty;

The typescript error here is very informative:

TS2420: Class 'Utterance' incorrectly implements interface 'FeatureSpecificAnnouncingControlPropertySupported'.   Property 'descriptionCanAnnounceProperty' is missing in type 'Utterance' but required in type 'FeatureSpecificAnnouncingControlPropertySupported'.

[jessegreenberg]

Makes a lot of sense, thanks. That was all for this one, closing,.