Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Voicing] Implement individual hands #413

Closed
zepumph opened this issue Nov 19, 2021 · 17 comments
Closed

[Voicing] Implement individual hands #413

zepumph opened this issue Nov 19, 2021 · 17 comments
Assignees

Comments

@zepumph
Copy link
Member

zepumph commented Nov 19, 2021

No description provided.

@zepumph zepumph self-assigned this Nov 19, 2021
@zepumph
Copy link
Member Author

zepumph commented Nov 19, 2021

@zepumph
Copy link
Member Author

zepumph commented Dec 2, 2021

Above is a start. I only have responses on end drag right now, not "during" drag. I still need to work out how GFLB was doing that and make things right.

I also am just using exact alerts from description, and I think they are a bit too verbose.

@terracoda this is as much as I could get to today, and likely won't have more time before our meeting Friday. Here is a dev version now even though it is really quite rough.

https://phet-dev.colorado.edu/html/ratio-and-proportion/1.2.0-dev.2/phet/ratio-and-proportion_all_phet.html

@terracoda
Copy link
Contributor

This is a great start @zepumph! I am glad you have just implemented the default descriptions for now. I think that is an excellent approach. Here are few things I am noticing right off the bat:

  1. When clicking or tapping a hand without making it move (column 1 in the table), I only want name & object response (proximity to ratio). I don't want position information (or progress information) unless the position changes. You can think of this type of event very similar to what a learner would hear upon focus (column 4 in the table).
  2. On Discover Screen, I get stale information upon refocus if I change the Challenge. For example, hands are at challenge ratio. I change from Challenge 1 to Challenge 2, so the hands are no longer at challenge ratio, but when I return to a hand with the keyboard I hear "hands, at challenge ratio". Note that the ratio will update for a single hand as soon as I click on a single hand. If I only click on one hand, then the proximity to ratio becomes different for each hand until I click, drag or move with the keyboard. Also note that a reset does not clear up the situation.
  3. PDOM & Voicing - sometimes I get "Closer" or "Farther away" progress when I moved the opposite way toward the other hand. I have not figured out exactly when this occurs. It is a bit random, and it happens in the PDOM, too. I'll check to see if there is an open issue for that.

Again, a really great start!
I agree the responses are long. I hope issue 1 above, eventual changes to position regions, and some refinement of progress indicators will address "length" of context responses. Let's be patient.

@terracoda
Copy link
Contributor

And just an FYI, I really like that I can turn off context changes (position information) and just hear ratio information.

@zepumph
Copy link
Member Author

zepumph commented Dec 3, 2021

I think I just implemented (1) above, but I could be wrong

@zepumph
Copy link
Member Author

zepumph commented Dec 3, 2021

For 2, we created a whole set of outside Properties that could change the description (aria-valuetext) of the sliders, so I just linked to all of those. I think it is working better now to update the context and object responses.

@zepumph
Copy link
Member Author

zepumph commented Dec 3, 2021

(3) will get worked on over in #416. There are two issues with that one. The most pressing is that by asking for the distance progress with voicing, we nullify the distance progress for description. (basically the second time we call it, the function thinks there has been no change).

@terracoda
Copy link
Contributor

@zepumph, I am still hearing position or progress information when I click on a hand and do not move it. I checked this version https://bayes.colorado.edu/dev/html/ratio-and-proportion/1.2.0-dev.2/phet/ratio-and-proportion_en_phet.html

I'll check phettest :-)

@terracoda
Copy link
Contributor

Another issue with mouse input:
When no checkboxes are checked and I click on the hands, I hear their name reponses:

  • Left Hand
  • Right Hand

When I check any of the checkboxes in the preferences menu, I no longer hear the name response, I just hear the responses associated with the checkboxes.

Note that for keyboard input, the responses that fire on keyboard focus are working correctly and as expected when different checkboxes are checked.

@zepumph, can you make the mouse click with no value change work the same, like this:

  1. deliver name response only when no checkboxes are checked
  2. deliver name responses & object response (proximity-to-ratio) when object responses are checked
  3. When hint responses are checked in preferences add that response in at the end of the other responses (name response, object repose, hint response) .
  4. Never deliver a context response unless the hands actually move when clicked, and hint response is not needed upon successful interaction, so no hint response of the hands actually move.

@zepumph
Copy link
Member Author

zepumph commented Dec 17, 2021

Today @terracoda and I spoke about this further, going through the table column by column. We realized it would be best if we focused on implementing the keyboard interaction before worrying about the timing of the mouse. This will be able to occur after #417 #416 and mostly phetsims/sun#730

@zepumph
Copy link
Member Author

zepumph commented Jan 5, 2022

Here is a first pass at using common code to achieve this. Here is the work that is done, and behavior expected:

  • On focus hear the default name/object/hint
  • On end drag (keyboard) hear the object/context, don't hear this if you didn't move.
  • On mouse start, hear name/object
  • On mouse move, hear object
  • on mouse end, hear object/context.

NOTE: The context responses for distance progress are broken, so you only hear distance from other hand.
NOTE: I know we discussed not doing the mouse just yet, but I thought I would try to get something easy in there so we could discuss it next week.

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

@terracoda
Copy link
Contributor

hmm...

  • On focus hear the default name/object/hint
  • On end drag (keyboard) hear the object/context, don't hear this if you didn't move.
    • Feedback: I hear object and context response, but I only hear the context response that includes progress. I think there are cases when I should not hear progress, certain object or context landmarks, like landing at ratio, landing even with other hand, landing at/hitting an edge.
    • proximity to ratio of second hand not updating when opposite hand moves. E.g. move Left Hand into ratio, move focus to Right Hand, and the Right Hand says it is "close to challenge ratio"
  • On mouse start, hear name/object
  • On mouse move, hear object
    • Feedback: the progress string, might be better placed on the object response??
  • On mouse end, hear object/context.
    • Feedback: I don't like the progress string on end of drag. That's not when you need to hear it.

@zepumph
Copy link
Member Author

zepumph commented Jan 20, 2022

I was making progress on implementing the voicing we talked about that is specific to mouse drags, but I couldn't really test because of phetsims/scenery#1337.

Here is the current patch of work:

Index: js/common/view/RatioHalf.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/view/RatioHalf.ts b/js/common/view/RatioHalf.ts
--- a/js/common/view/RatioHalf.ts	(revision 71ae824c11bc540f702d072dd474551814c719ea)
+++ b/js/common/view/RatioHalf.ts	(date 1642721270169)
@@ -292,9 +292,8 @@
         a11yCreateAriaValueText: createObjectResponse,
         voicingCreateObjectResponse: createObjectResponse,
 
-        // TODO: comment back in once https://github.com/phetsims/ratio-and-proportion/issues/416 is fixed
-        // a11yCreateContextResponseAlert: () => this.getSingleHandContextResponse(),
-        voicingCreateContextResponse: () => this.getSingleHandContextResponse(),
+        a11yCreateContextResponseAlert: () => this.getSingleHandContextResponse(),
+        // voicingCreateContextResponse: () => this.getSingleHandContextResponse(),
         a11yDependencies: options.a11yDependencies.concat( [ options.ratioLockedProperty ] ),
         voicingNameResponse: options.accessibleName // accessible name is also the voicing name response
       } );
@@ -396,9 +395,16 @@
         viewSounds.tickMarkBumpSoundClip.onInteract( positionProperty.value.y );
 
         // @ts-ignore
-        this.ratioHandNode.voicingOnChangeResponse( {
-          utterance: voicingUtteranceForDrag
-      } );
+        this.ratioHandNode.voicingObjectResponse = createObjectResponse();
+
+        // @ts-ignore
+        this.ratioHandNode.voicingSpeakResponse( {
+          utterance: voicingUtteranceForDrag,
+          contextResponse: this.handPositionsDescriber.getSingleHandDistanceProgressSentence( this.ratioTerm ),
+
+          // @ts-ignore
+          objectResponse: this.ratioHandNode.voicingObjectResponse
+        } );
       },
 
       end: () => {
@@ -426,10 +432,16 @@
         // Only voice a response if the value changed
         if ( startingY !== positionProperty.value.y ) {
 
-          // TODO: should this have the object response too, or just the context repsonse?? https://github.com/phetsims/ratio-and-proportion/issues/413
+          // @ts-ignore
+          this.ratioHandNode.voicingObjectResponse = createObjectResponse();
+
           // @ts-ignore
-          this.ratioHandNode.voicingOnEndResponse( {
-            onlyOnValueChange: false // don't use the AccessibleValueHandler's start, and instead handle it ourselves
+          this.ratioHandNode.voicingSpeakResponse( {
+            utterance: voicingUtteranceForDrag,
+            contextResponse: this.handPositionsDescriber.getSingleHandDistanceRegionSentence( this.ratioTerm ),
+
+            // @ts-ignore
+            objectResponse: this.ratioHandNode.voicingObjectResponse
           } );
         }
         else {
Index: js/common/view/describers/HandPositionsDescriber.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/view/describers/HandPositionsDescriber.ts b/js/common/view/describers/HandPositionsDescriber.ts
--- a/js/common/view/describers/HandPositionsDescriber.ts	(revision 71ae824c11bc540f702d072dd474551814c719ea)
+++ b/js/common/view/describers/HandPositionsDescriber.ts	(date 1642721073532)
@@ -73,7 +73,7 @@
 
   private ratioTupleProperty: Property<RAPRatioTuple>;
   private tickMarkDescriber: TickMarkDescriber;
-  private previousDistanceRegion: null | string;
+  private previousDistanceLowercaseRegion: null | string;
   private previousDistance: number;
   public static QUALITATIVE_POSITIONS: string[];
 
@@ -89,8 +89,13 @@
 
     // @private - keep track of previous distance regions to track repetition, and alter description accordingly. This
     // is used for any modality getting a distance region in a context response.
-    this.previousDistanceRegion = null;
+    this.previousDistanceLowercaseRegion = null;
     this.previousDistance = ratioTupleProperty.value.getDistance();
+
+    ratioTupleProperty.lazyLink( ( newValue, oldValue ) => {
+      this.previousDistance = oldValue.getDistance();
+      this.previousDistanceLowercaseRegion = this.getDistanceRegion( false );
+    } );
   }
 
   /**
@@ -162,8 +167,7 @@
   /**
    * NOTE: These values are copied over in RAPPositionRegionsLayer, consult that Node before changing these values.
    */
-  public getDistanceRegion( lowercase = false ): string {
-    const distance = this.ratioTupleProperty.value.getDistance();
+  public getDistanceRegion( lowercase: boolean, distance: number = this.ratioTupleProperty.value.getDistance() ): string {
 
     assert && assert( TOTAL_RANGE.getLength() === 1, 'these hard coded values depend on a range of 1' );
 
@@ -204,12 +208,32 @@
     return ( lowercase ? DISTANCE_REGIONS_LOWERCASE : DISTANCE_REGIONS_CAPITALIZED )[ index ];
   }
 
+  public getSingleHandDistanceRegionSentence( ratioTerm: RatioTerm ): string {
+    const otherHand = ratioTerm === RatioTerm.ANTECEDENT ? rightHandLowerString : leftHandLowerString;
+
+    const distanceRegion = this.getDistanceRegion( false );
+
+    return StringUtils.fillIn( ratioAndProportionStrings.a11y.handPosition.distanceOrDistanceProgressClause, {
+      otherHand: otherHand,
+      distanceOrDistanceProgress: distanceRegion
+    } );
+  }
+
+  public getSingleHandDistanceProgressSentence( ratioTerm: RatioTerm ): string {
+    const otherHand = ratioTerm === RatioTerm.ANTECEDENT ? rightHandLowerString : leftHandLowerString;
+
+    return StringUtils.fillIn( ratioAndProportionStrings.a11y.handPosition.distanceOrDistanceProgressClause, {
+      otherHand: otherHand,
+      distanceOrDistanceProgress: this.getDistanceProgressString()
+    } );
+  }
+
   public getSingleHandDistance( ratioTerm: RatioTerm ): string {
     const otherHand = ratioTerm === RatioTerm.ANTECEDENT ? rightHandLowerString : leftHandLowerString;
 
-    const distanceRegion = this.getDistanceRegion();
+    const distanceRegion = this.getDistanceRegion( false );
 
-    if ( distanceRegion === this.previousDistanceRegion ) {
+    if ( this.getDistanceRegion( true ) === this.previousDistanceLowercaseRegion ) {
       const distanceProgressPhrase = this.getDistanceProgressString();
       if ( distanceProgressPhrase ) {
 
@@ -219,13 +243,10 @@
         } );
 
         // Count closer/farther as a previous so that we don't ever get two of them at the same time
-        this.previousDistanceRegion = distanceProgressDescription;
         return distanceProgressDescription;
       }
     }
 
-    this.previousDistanceRegion = distanceRegion;
-
     return StringUtils.fillIn( ratioAndProportionStrings.a11y.handPosition.distanceOrDistanceProgressClause, {
       otherHand: otherHand,
       distanceOrDistanceProgress: distanceRegion
@@ -236,7 +257,7 @@
     const distanceRegion = this.getDistanceRegion( true );
 
     if ( overrideWithDistanceProgress ) {
-      if ( distanceRegion === this.previousDistanceRegion ) {
+      if ( distanceRegion === this.previousDistanceLowercaseRegion ) {
         assert && assert( capitalized, 'overriding with distance-progress not supported for capitalized strings' );
 
         const distanceProgressPhrase = this.getDistanceProgressString( {
@@ -249,11 +270,9 @@
           } );
 
           // Count closer/farther as a previous so that we don't ever get two of them at the same time
-          this.previousDistanceRegion = distanceProgressDescription;
           return distanceProgressDescription;
         }
       }
-      this.previousDistanceRegion = distanceRegion;
     }
 
     const pattern = capitalized ? ratioAndProportionStrings.a11y.bothHands.handsDistancePatternCapitalized :
@@ -277,7 +296,6 @@
       distanceProgressString = filledOptions.fartherString;
     }
 
-    this.previousDistance = currentDistance;
     return distanceProgressString;
   }
 }

@terracoda
Copy link
Contributor

terracoda commented Jan 21, 2022

Testing Notes from latest versions Jan 21, 2022

  1. on first keyboard focus after adding object & context responses, I don' hear the ratio value. This work fine for mouse click, though. Issue likely caused by un-updated aria-valuetext.
  2. sim crashed when I switched from keyboard to mouse input

@zepumph
Copy link
Member Author

zepumph commented Jan 21, 2022

  • Note that the hints for mouse need to be changed from endDrag to startDrag.

@terracoda
Copy link
Contributor

terracoda commented Feb 18, 2022

Testing Notes from latest versions Feb 18, 2022

@zepumph
Copy link
Member Author

zepumph commented Feb 23, 2022

At this point, I think the last item here will be covered over in #429. I'm going to close this, please reopen if there is more to discuss, or feel free to open new issues to report bugs you find.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants