phetsims / sun

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

RadioButtons: use labelContent for voicing name response #699

Closed zepumph closed 2 years ago

zepumph commented 3 years ago

From https://github.com/phetsims/ratio-and-proportion/issues/384, it seems that we can get an on-focus object response put into common code for all radio buttons. For selection, we usually defer to the usage to implement (for interactive description and voicing), but it should be easy enough to use labelContent to supply the voicing object response.

I'll do this for aqua and rectangular radio buttons.

zepumph commented 3 years ago

This went smoothly for RectangularRadioButton. I committed the simple line change above. @jessegreenberg it would be good to take a look at that to confirm.

It was much weirder for AquaRadioButton. I made a patch below to note my progress. It is not a ButtonNode, so I tried to compose Voicing into AquaRadioButton.

The issue was that upon selecting new items with arrow keys, the next AquaRadioButtons gets focused. I traced it back to this line in Input.js. I'm unsure why there is this discrepancy between radio button implementations.

https://github.com/phetsims/scenery/blob/a7bfc37d6d51d261d6cb7e314317ed8a0d73cf5c/js/input/Input.js#L643-L659

Steps to reproduce:

  1. Apply this patch when sun/joist/tambo are on preferences branch
  2. Go to BASE
  3. With dev tools open, go to the aqua radio buttons with keyboard, focus them. Note that the object response works correctly here.
  4. Then arrow key down twice to "Show Charge Differences", there should be a debugger and in it you can see that from the selection change, there is a focus change based on the relatedTarget code in Input.js

The issue here is that we can't hard code this in common code if it happens on selection too, because selection often removes the object response because it is part of the context response. @jessegreenberg please advise. Note that this isn't blocking Ratio and proportion, but means that the radio buttons in common code have an inconsistent state and API between each other.

```diff Index: sun/js/AquaRadioButtonGroup.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/sun/js/AquaRadioButtonGroup.js b/sun/js/AquaRadioButtonGroup.js --- a/sun/js/AquaRadioButtonGroup.js (revision 1b9f94afce534dc27912b6303e304d8ee241dc82) +++ b/sun/js/AquaRadioButtonGroup.js (date 1620075246258) @@ -90,13 +90,20 @@ new Node( { children: [ new HStrut( maxItemWidth ), item.node ] } ) : item.node; - const radioButton = new AquaRadioButton( property, item.value, content, - merge( {}, options.radioButtonOptions, { - tandem: item.tandemName ? options.tandem.createTandem( item.tandemName ) : Tandem.REQUIRED, - labelContent: item.labelContent || null, - soundPlayer: multiSelectionSoundPlayerFactory.getSelectionSoundPlayer( i ), - a11yNameAttribute: CLASS_NAME + instanceCount - } ) ); + const aquaRadioButtonOptions = merge( {}, options.radioButtonOptions, { + tandem: item.tandemName ? options.tandem.createTandem( item.tandemName ) : Tandem.REQUIRED, + soundPlayer: multiSelectionSoundPlayerFactory.getSelectionSoundPlayer( i ), + a11yNameAttribute: CLASS_NAME + instanceCount + } ); + + // If there is labelContent, then apply it for interactive description (pdom) and voicing (object response) + if ( item.labelContent ) { + aquaRadioButtonOptions.labelContent = item.labelContent; + aquaRadioButtonOptions.voicingCreateObjectResponse = event => event.type === 'focus' ? item.labelContent : null; + } + + const radioButton = new AquaRadioButton( property, item.value, content, + aquaRadioButtonOptions ); // set pointer areas if ( options.orientation === 'vertical' ) { Index: sun/js/AquaRadioButton.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/sun/js/AquaRadioButton.js b/sun/js/AquaRadioButton.js --- a/sun/js/AquaRadioButton.js (revision 1b9f94afce534dc27912b6303e304d8ee241dc82) +++ b/sun/js/AquaRadioButton.js (date 1620073898099) @@ -8,6 +8,7 @@ import InstanceRegistry from '../../phet-core/js/documentation/InstanceRegistry.js'; import merge from '../../phet-core/js/merge.js'; +import Voicing from '../../scenery/js/accessibility/speaker/Voicing.js'; import FireListener from '../../scenery/js/listeners/FireListener.js'; import Circle from '../../scenery/js/nodes/Circle.js'; import Node from '../../scenery/js/nodes/Node.js'; @@ -70,6 +71,9 @@ super(); + // voicing + this.initializeVoicing(); + // @public (read-only) this.value = value; @@ -177,5 +181,7 @@ AquaRadioButton.DEFAULT_RADIUS = DEFAULT_RADIUS; +Voicing.compose( AquaRadioButton ); + sun.register( 'AquaRadioButton', AquaRadioButton ); export default AquaRadioButton; \ No newline at end of file Index: scenery/js/accessibility/speaker/webSpeaker.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/scenery/js/accessibility/speaker/webSpeaker.js b/scenery/js/accessibility/speaker/webSpeaker.js --- a/scenery/js/accessibility/speaker/webSpeaker.js (revision 69220950bad3891beb0e4016a065938008c1c816) +++ b/scenery/js/accessibility/speaker/webSpeaker.js (date 1620073765266) @@ -55,7 +55,7 @@ this.initialized = false; // whether or ot the webSpeaker is enabled - if false, there will be no speech - this.enabledProperty = new BooleanProperty( false ); + this.enabledProperty = new BooleanProperty( true ); // @private {DerivedProperty} - Controls whether or not speech is allowed with synthesis. // This controlling Property can be set with setCanSpeakProperty. Index: balloons-and-static-electricity/js/balloons-and-static-electricity-main.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/balloons-and-static-electricity/js/balloons-and-static-electricity-main.js b/balloons-and-static-electricity/js/balloons-and-static-electricity-main.js --- a/balloons-and-static-electricity/js/balloons-and-static-electricity-main.js (revision 6853e1abaf23506b503bb08fac97462a86c6c0a0) +++ b/balloons-and-static-electricity/js/balloons-and-static-electricity-main.js (date 1620073774549) @@ -6,15 +6,16 @@ * @author Sam Reid (PhET Interactive Simulations) */ +import PreferencesConfiguration from '../../joist/js/preferences/PreferencesConfiguration.js'; import Screen from '../../joist/js/Screen.js'; import Sim from '../../joist/js/Sim.js'; import simLauncher from '../../joist/js/simLauncher.js'; -import SoundOptionsDialogContent from './balloons-and-static-electricity/view/SoundOptionsDialogContent.js'; import Tandem from '../../tandem/js/Tandem.js'; import BASEConstants from './balloons-and-static-electricity/BASEConstants.js'; import BASEModel from './balloons-and-static-electricity/model/BASEModel.js'; import BASEKeyboardHelpContent from './balloons-and-static-electricity/view/BASEKeyboardHelpContent.js'; import BASEView from './balloons-and-static-electricity/view/BASEView.js'; +import SoundOptionsDialogContent from './balloons-and-static-electricity/view/SoundOptionsDialogContent.js'; import balloonsAndStaticElectricityStrings from './balloonsAndStaticElectricityStrings.js'; // constants @@ -36,6 +37,14 @@ graphicArts: 'Sharon Siman-Tov', thanks: 'Thanks to Mobile Learner Labs for working with the PhET development team to convert this simulation to HTML5.' }, + preferencesConfiguration: new PreferencesConfiguration( { + audioOptions: { + supportsVoicing: true + }, + visualOptions: { + supportsInteractiveHighlights: true + } + } ), hasKeyboardHelpContent: true, createOptionsDialogContent: () => SOUND_OPTIONS_DIALOG_CONTENT }; Index: utterance-queue/js/UtteranceQueue.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/utterance-queue/js/UtteranceQueue.js b/utterance-queue/js/UtteranceQueue.js --- a/utterance-queue/js/UtteranceQueue.js (revision 35ec385a6ef4b6515024277ea1befb125fb8e3dd) +++ b/utterance-queue/js/UtteranceQueue.js (date 1620075246268) @@ -155,6 +155,10 @@ utterance = new Utterance( { alert: utterance } ); } + if ( utterance.alert === 'Show charge differences' ) { + debugger; + } + // If there are any other items in the queue of the same type, remove them immediately because the added // utterance is meant to replace it this.removeUtterance( utterance, { ```
zepumph commented 3 years ago

Actually, @jessegreenberg and I just paired and this does in fact block Ratio and Proportion. I found that RectangularRadioButton was also getting these focus callbacks, but the Property lazyLink voicing utterance was overwriting it immediately.

zepumph commented 3 years ago

@jessegreenberg and I discussed, and feel like it makes sense that the object response for the buttons would always be on focus, even on activation. After the above bug fix, we are able to use cancelOther: false to allow both the object response from the focus listener and the context response from the Property listener.

This is a bit inflexible, but I would like to see how far we can get with it.

I will commit the addition to AquaRadioButtonGroup as well.

zepumph commented 3 years ago

ARG!! @jessegreenberg I just realized this approach won't work for mouse/interactive highlights to give the object response. I'll keep thinking about it. I can't think of a way to have focus take the object response until it "can't." And then to have the Property handle it instead.

jessegreenberg commented 3 years ago

I guess in this case we need the object response on both focus and down. This wouldn't work after what was proposed in https://github.com/phetsims/scenery/issues/1217, but would it work if you removed the check for event.type === 'focus' in https://github.com/phetsims/sun/commit/861e30d87b75b71e7e0608f30176759249977f6a?

You would get the object response on press and on focus. Then the context response that follows from the Property listener with cancelOther: false on the Utterance.

terracoda commented 3 years ago

@jessegreenberg, have we solved this issue by creating the "Name Response"? On click or upon focus, the user will always get the Name Response, and then also get the Context Response if that checkbox is checked?

zepumph commented 2 years ago

Yes @terracoda, I believe so. I renamed the issue to be align with the newer voicing api. @jessegreenberg, I hit this working on RaP, so I'll go ahead and take the lead on seeing what I can bring over from the preferences branch.

zepumph commented 2 years ago

Right now I'm blocked on https://github.com/phetsims/scenery/issues/1349 for this.

zepumph commented 2 years ago

This is all I found from the branch:


Index: js/buttons/RectangularRadioButton.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/buttons/RectangularRadioButton.js b/js/buttons/RectangularRadioButton.js
--- a/js/buttons/RectangularRadioButton.js  (revision f1ee5bc82974cf0265aeef998ad88ab8fbb88783)
+++ b/js/buttons/RectangularRadioButton.js  (date 1645636413893)
@@ -129,6 +129,9 @@
     buttonModel.downProperty.link( down => {
       if ( !down && ( buttonModel.overProperty.get() || buttonModel.focusedProperty.get() ) && !buttonModel.interrupted ) {
         this.fire();
+
+        // voicing
+        this.voicingSpeakFullResponse();
       }
     } );
zepumph commented 2 years ago

This was pretty straight forward. While I was in here, I also added support for context responses to be passed in the "Item" metadata. @jessegreenberg can you please review. Any more thoughts here?

jessegreenberg commented 2 years ago

Voicing change looks good to me! Passing through "item" is the API I would expect. Thanks!

Is there a need for hint responses for these buttons? Seems unlikely because you would have to select the button to hear it at which point it would be too late to be helpful. If no, feel free to close.

zepumph commented 2 years ago

There was no hint response for the radio buttons in RAP. Perhaps we will want that support at somepoint, but it would be so easy to add that I feel like we shouldn't prematurely open up that API.

zepumph commented 2 years ago

image

For clarity, note how the radio button rules recommend no hint response across the board.

terracoda commented 2 years ago

Many radiobutton groups, do indeed have help text, but RaP's does not.

terracoda commented 2 years ago

I think, help text will only be available for keyboard users for radio button groups. There seems like no need for help text for mouse and touch users.

zepumph commented 2 years ago

I can plug in a way to have the group take the hint response, and then pass it to every radio button as it goes, then it will automatically show up on focus, but won't bleed in anywhere else.

terracoda commented 2 years ago

Yep, would just want a hint on focus, if hints are checked.

zepumph commented 2 years ago

Added above. This works as easily as with this patch:

Index: js/common/view/TickMarkViewRadioButtonGroup.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/view/TickMarkViewRadioButtonGroup.ts b/js/common/view/TickMarkViewRadioButtonGroup.ts
--- a/js/common/view/TickMarkViewRadioButtonGroup.ts    (revision d2d7c89ee21d9b24bea875e4a3110c304b9ea18b)
+++ b/js/common/view/TickMarkViewRadioButtonGroup.ts    (date 1645727075661)
@@ -37,7 +37,9 @@

       // pdom
       labelContent: ratioAndProportionStrings.a11y.tickMark.heading,
-      helpTextBehavior: ParallelDOM.HELP_TEXT_BEFORE_CONTENT
+      helpTextBehavior: ParallelDOM.HELP_TEXT_BEFORE_CONTENT,
+
+      voicingHintResponse: 'oh what a hint it is'
     }, options );

     const radioButtonItemData = [ {

Thanks for chiming in @terracoda, I should have asked you about it instead of assuming.

terracoda commented 2 years ago

Does this only announce the hint (if checked in preferences) on the initial focus - when focus enters the group and focus on the initially selected radio button? We do not want a hint upon a new selection when using the arrow keys.

zepumph commented 2 years ago

Yes, only when hints are enabled with the checkbox.

terracoda commented 2 years ago

And only on initial focus?

zepumph commented 2 years ago

Ahhh! You got me! I had to omit the hint from the selection response. I made you a dev version, and put in a temporary hint for the tick mark radio buttons. Will you please review and see if it sounds correct for these cases? (It should be right now)

https://phet-dev.colorado.edu/html/ratio-and-proportion/1.2.0-dev.8/phet/ratio-and-proportion_all_phet.html?voicingInitiallyEnabled

terracoda commented 2 years ago

This is perfect @zepumph for when there is a hint for the radio group. Thanks for verifying with a test. You can close this issue now. We have a radio group/radio button pattern that should work well.

zepumph commented 2 years ago

Excellent. I reverted the temp hint. Closing