phetsims / sun

User-interface components for PhET simulations, built on top of Scenery.
MIT License
4 stars 12 forks source link

Add voicing to Checkbox #742

Closed zepumph closed 2 years ago

zepumph commented 2 years ago

From https://github.com/phetsims/ratio-and-proportion/issues/363. This is pretty straight forward because of @jessegreenberg's work in https://github.com/phetsims/sun/commit/6afc6a986c4988c858dd3fc75360a33137103085.

zepumph commented 2 years ago

@jessegreenberg does this look right to you? You can test in RAP if you pull all.

Usage site:

https://github.com/phetsims/ratio-and-proportion/blob/4e406540591163ef905d196d5c10a5350c686065/js/create/view/CreateScreenView.ts#L71-L88

jessegreenberg commented 2 years ago

Looks really nice, and works well. Thanks @zepumph.

zepumph commented 2 years ago

@pixelzoom just found that the typing is wildly wrong for voicing. The checkedContextResponse is TAlertableDef for description, but basically just a string for voicing.

zepumph commented 2 years ago

It isn't clear to me how best to proceed with this. The easiest way would be to have each accept the same type, but there isn't really that support at this time, so I'm not really sure. I'm also quite surprised that we haven't run into this in any other common code just yet.

jessegreenberg commented 2 years ago

Maybe these options should take the subset of types that both Voicing and PDOM support, that would basically just be string or null. Or the Voicing implementation in Checkbox can turn the TAlertableDef into a VoicingResponse. Maybe we have a utility function to convert a TAlertableDef into a VoicingResponse.

pixelzoom commented 2 years ago

Here are the TODOs in CheckBox that need to be addressed:

      if ( property.value ) {
        options.checkedSoundPlayer.play();
        options.checkedContextResponse && this.alertDescriptionUtterance( options.checkedContextResponse );
        // @ts-ignore TODO https://github.com/phetsims/sun/issues/742
        this.voicingSpeakNameResponse( { contextResponse: options.checkedContextResponse } );
      }
      else {
        options.uncheckedSoundPlayer.play();
        options.uncheckedContextResponse && this.alertDescriptionUtterance( options.uncheckedContextResponse );
        // @ts-ignore TODO https://github.com/phetsims/sun/issues/742
        this.voicingSpeakNameResponse( { contextResponse: options.uncheckedContextResponse } );
      }
zepumph commented 2 years ago

Typing here has been solved over in https://github.com/phetsims/utterance-queue/issues/67. See https://github.com/phetsims/sun/commit/e218a680bafc4f60aa3486385dd9ed772a00bfb4