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

Go beyond edge context responses & special hint responses at edges #457

Closed
terracoda opened this issue Apr 4, 2022 · 26 comments
Closed

Comments

@terracoda
Copy link
Contributor

In #452 @zepumph and I verified that there are no special responses when a learner tries to go beyond the top or bottom of the visual Play Area when using the keyboard for Interactive Description. I just verified that this is the case for the Both Hands and the Indy Sliders - no special responses at the edges.

A hint about changing directions is only provided when ratio is locked. I think this is because a learner can't actually get to the bottom or get to Tick Mark zero (good indications that and edge has been reached) when the ratio is locked.

I think it would be worthwhile to stream line the designs for alternative input (focus-based input) when the Ratio is not locked, and to do the same for both Interactive Description and Voicing.

For Alternative Input (both Interactive Description & Voicing) we use the same strings to hint at changing directions. Instead of repeating that they are at the top (on Tick Mark 10) or at the bottom (on Tick Mark 0), we can provide a similar response that was suggested in #452 (comment)

I don't think the wording can be exactly the same because when the ratio is NOT locked, the user is moving an individual hand. I think this could work across all interactions and for both Interactive Description and Voicing:

  • {{Left}} Hand at {{bottom}}, Move hand {{up}} to explore more.
  • {{Right}} Hand at {{top}}, Move hand {{down}} to explore more.

This would result is 4 possible beyond-edge context responses for Interactive Description, and 4 hint responses for Voicing:

  • Left Hand at bottom, Move hand up to explore more.
  • Left Hand at top, Move hand down to explore more.
  • Right Hand at bottom, Move hand up to explore more.
  • Right Hand at top, Move hand down to explore more.

I think the same responses work regardless if Tick Marks are shown or hidden because on the last step that actually reaches the edge but does not go beyond, the response will description the dist and position of the hands like this:

Left hand slider reaches bottom or Tick Mark 0

  • {{proximityToRatio}}, Not so close to right hand, at bottom.
  • {{proximityToRatio}}, Not so close to right hand, at tick mark zero.

Left hand of Both Hands reaches bottom or Tick Mark 0

  • {{proximityToRatio}}, Hands not so close to each other, left at bottom, right in lower-middle region.
  • {{proximityToRatio}}, Hands not so close to each other, left at tick mark zero, right on fourth tick mark.

In all cases if the learner presses the Down Arrow or the W key (Both Hands), a shorter helpful response will let them know they need to change directions to do anything potentially productive. Of course they could just move their focus to the Right Hand and move it, or do something else entirely, but they won't keep trying to move the Left Hand down if they hear:

  • Left Hand at bottom, Move hand up to explore more.

When the ratio is locked, the logic is the same, but the second part of the phrase says "Move hands" rather than "Move hand".

@zepumph, is this straight forward to implement for both Interactive Description and Voicing?

@terracoda
Copy link
Contributor Author

@zepumph, please note that the above strings do not reflect other specific changes to context responses that we have already discussed for Voicing, e.g., removing the distance-region when Tick Marks are shown.

@terracoda
Copy link
Contributor Author

I think this issue is also directly related to #432 where you are implementing the Both Hands.

@zepumph
Copy link
Member

zepumph commented Apr 18, 2022

I didn't get to a commit point unfortunately.

Index: js/create/view/CreateScreenView.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/create/view/CreateScreenView.ts b/js/create/view/CreateScreenView.ts
--- a/js/create/view/CreateScreenView.ts	(revision 201a45e2a80657853f6cf93cda6e5eaec3e4016d)
+++ b/js/create/view/CreateScreenView.ts	(date 1650297704743)
@@ -50,9 +50,12 @@
 
     const tickMarkRangeComboBoxParent = new Node();
 
-    this.tickMarkRangeComboBoxNode = new TickMarkRangeComboBoxNode( this.tickMarkRangeProperty, tickMarkRangeComboBoxParent, this.tickMarkViewProperty );
+    this.tickMarkRangeComboBoxNode = new TickMarkRangeComboBoxNode( this.tickMarkRangeProperty,
+      tickMarkRangeComboBoxParent, this.tickMarkViewProperty );
 
-    const handPositionsDescriber = new HandPositionsDescriber( model.ratio.tupleProperty, new TickMarkDescriber( this.tickMarkRangeProperty, this.tickMarkViewProperty ), model.inProportionProperty );
+    const handPositionsDescriber = new HandPositionsDescriber( model.ratio.tupleProperty,
+      new TickMarkDescriber( this.tickMarkRangeProperty, this.tickMarkViewProperty ),
+      model.inProportionProperty, model.ratio.enabledRatioTermsRangeProperty );
 
     this.createScreenSummaryNode = new CreateScreenSummaryNode(
       model.ratioFitnessProperty,
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 201a45e2a80657853f6cf93cda6e5eaec3e4016d)
+++ b/js/common/view/describers/HandPositionsDescriber.ts	(date 1650303323310)
@@ -14,6 +14,7 @@
  * @author Michael Kauzmann (PhET Interactive Simulations)
  */
 
+import Range from '../../../../../dot/js/Range.js';
 import StringUtils from '../../../../../phetcommon/js/util/StringUtils.js';
 import ratioAndProportion from '../../../ratioAndProportion.js';
 import ratioAndProportionStrings from '../../../ratioAndProportionStrings.js';
@@ -26,6 +27,7 @@
 import IReadOnlyProperty from '../../../../../axon/js/IReadOnlyProperty.js';
 import DistanceResponseType from './DistanceResponseType.js';
 import optionize from '../../../../../phet-core/js/optionize.js';
+import RatioInputModality from './RatioInputModality.js';
 
 const leftHandLowerString = ratioAndProportionStrings.a11y.leftHandLower;
 const rightHandLowerString = ratioAndProportionStrings.a11y.rightHandLower;
@@ -131,18 +133,24 @@
 
   private previousDistance: number;
   static POSITION_REGIONS_DATA: PositionRegionsData[];
+  private previousEdgeCheckTuple: RAPRatioTuple;
+  enabledRatioTermsRangeProperty: IReadOnlyProperty<Range>;
 
-  constructor( ratioTupleProperty: Property<RAPRatioTuple>, tickMarkDescriber: TickMarkDescriber, inProportionProperty: IReadOnlyProperty<boolean> ) {
+  constructor( ratioTupleProperty: Property<RAPRatioTuple>, tickMarkDescriber: TickMarkDescriber,
+               inProportionProperty: IReadOnlyProperty<boolean>, enabledRatioTermsRangeProperty: IReadOnlyProperty<Range> ) {
 
     this.ratioTupleProperty = ratioTupleProperty;
     this.tickMarkDescriber = tickMarkDescriber;
     this.inProportionProperty = inProportionProperty;
+    this.enabledRatioTermsRangeProperty = enabledRatioTermsRangeProperty;
 
     this.previousDistanceRegionSingle = null;
     this.previousDistanceRegionBoth = null;
 
     this.previousDistance = ratioTupleProperty.value.getDistance();
 
+    this.previousEdgeCheckTuple = ratioTupleProperty.value;
+
     ratioTupleProperty.lazyLink( ( newValue, oldValue ) => {
       if ( oldValue ) {
         this.previousDistance = oldValue.getDistance();
@@ -412,11 +420,68 @@
     return distanceProgressString;
   }
 
+  getGoBeyondContextResponse( currentTuple: RAPRatioTuple, inputModality: RatioInputModality ): string | null {
+
+    const enabledRange = this.enabledRatioTermsRangeProperty.value;
+
+    const previousAntecedentAtMin = this.previousEdgeCheckTuple.antecedent === enabledRange.min;
+    const previousAntecedentAtMax = this.previousEdgeCheckTuple.antecedent === enabledRange.max;
+    const previousConsequentAtMin = this.previousEdgeCheckTuple.consequent === enabledRange.min;
+    const previousConsequentAtMax = this.previousEdgeCheckTuple.consequent === enabledRange.max;
+
+    let handAtExtremity = null; // what hand?
+    let extremityPosition = null; // where are we now?
+    let direction = null; // where to go from here?
+
+    if ( this.ratioTupleProperty.value.antecedent === enabledRange.min ) {
+      if ( previousAntecedentAtMin ) {
+        handAtExtremity = ratioAndProportionStrings.a11y.leftHand;
+        extremityPosition = ratioAndProportionStrings.a11y.handPosition.nearBottom;
+        direction = ratioAndProportionStrings.a11y.up;
+      }
+    }
+    else if ( this.ratioTupleProperty.value.antecedent === enabledRange.max ) {
+      if ( previousAntecedentAtMax ) {
+        handAtExtremity = ratioAndProportionStrings.a11y.leftHand;
+        extremityPosition = ratioAndProportionStrings.a11y.handPosition.atTop;
+        direction = ratioAndProportionStrings.a11y.down;
+      }
+    }
+    else if ( this.ratioTupleProperty.value.consequent === enabledRange.min ) {
+      if ( previousConsequentAtMin ) {
+        handAtExtremity = ratioAndProportionStrings.a11y.rightHand;
+        extremityPosition = ratioAndProportionStrings.a11y.handPosition.nearBottom;
+        direction = ratioAndProportionStrings.a11y.up;
+      }
+    }
+    else if ( this.ratioTupleProperty.value.consequent === enabledRange.max ) {
+      if ( previousConsequentAtMax ) {
+        handAtExtremity = ratioAndProportionStrings.a11y.rightHand;
+        extremityPosition = ratioAndProportionStrings.a11y.handPosition.atTop;
+        direction = ratioAndProportionStrings.a11y.down;
+      }
+    }
+
+    this.previousEdgeCheckTuple = currentTuple;
+
+    // Detect if we are at the edge of the range
+    if ( handAtExtremity && extremityPosition && direction ) {
+      return StringUtils.fillIn( ratioAndProportionStrings.a11y.ratio.ratioLockedEdgeContextResponse, {
+        position: extremityPosition,
+        hand: handAtExtremity,
+        direction: direction
+      } );
+    }
+
+    return null;
+  }
+
   reset(): void {
     this.previousDistanceRegionSingle = null;
     this.previousDistanceRegionBoth = null;
 
     this.previousDistance = this.ratioTupleProperty.value.getDistance();
+    this.previousEdgeCheckTuple = this.ratioTupleProperty.value;
   }
 }
 
Index: js/common/view/describers/BothHandsDescriber.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/view/describers/BothHandsDescriber.ts b/js/common/view/describers/BothHandsDescriber.ts
--- a/js/common/view/describers/BothHandsDescriber.ts	(revision 201a45e2a80657853f6cf93cda6e5eaec3e4016d)
+++ b/js/common/view/describers/BothHandsDescriber.ts	(date 1650297704726)
@@ -45,7 +45,7 @@
     this.enabledRatioTermsRangeProperty = enabledRatioTermsRangeProperty;
     this.tickMarkViewProperty = tickMarkViewProperty;
     this.ratioDescriber = ratioDescriber;
-    this.handPositionsDescriber = new HandPositionsDescriber( ratioTupleProperty, tickMarkDescriber, inProportionProperty );
+    this.handPositionsDescriber = new HandPositionsDescriber( ratioTupleProperty, tickMarkDescriber, inProportionProperty, enabledRatioTermsRangeProperty );
     this.ratioLockedProperty = ratioLockedProperty;
     this.previousAntecedentAtExtremity = false;
     this.previousConsequentAtExtremity = false;
Index: js/discover/view/DiscoverScreenView.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/discover/view/DiscoverScreenView.ts b/js/discover/view/DiscoverScreenView.ts
--- a/js/discover/view/DiscoverScreenView.ts	(revision 201a45e2a80657853f6cf93cda6e5eaec3e4016d)
+++ b/js/discover/view/DiscoverScreenView.ts	(date 1650297704748)
@@ -49,7 +49,9 @@
     this.addChild( comboBoxListBoxParent );
 
     this.pdomPlayAreaNode.pdomOrder = this.pdomPlayAreaNode.pdomOrder!.concat( [ this.comboBoxContainer, comboBoxListBoxParent ] );
-    const handPositionsDescriber = new HandPositionsDescriber( model.ratio.tupleProperty, new TickMarkDescriber( this.tickMarkRangeProperty, this.tickMarkViewProperty ), model.inProportionProperty );
+    const handPositionsDescriber = new HandPositionsDescriber( model.ratio.tupleProperty,
+      new TickMarkDescriber( this.tickMarkRangeProperty, this.tickMarkViewProperty ), model.inProportionProperty,
+      model.ratio.enabledRatioTermsRangeProperty );
 
     // set this after the supertype has initialized the view code needed to create the screen summary
     this.discoverScreenSummaryNode = new DiscoverScreenSummaryNode(
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 201a45e2a80657853f6cf93cda6e5eaec3e4016d)
+++ b/js/common/view/RatioHalf.ts	(date 1650297704738)
@@ -154,8 +154,8 @@
     this.ratio = options.ratio;
 
     const tickMarkDescriber = new TickMarkDescriber( options.tickMarkRangeProperty, options.tickMarkViewProperty );
-    this.descriptionHandPositionsDescriber = new HandPositionsDescriber( this.ratio.tupleProperty, tickMarkDescriber, options.inProportionProperty );
-    this.voicingHandPositionsDescriber = new HandPositionsDescriber( this.ratio.tupleProperty, tickMarkDescriber, options.inProportionProperty );
+    this.descriptionHandPositionsDescriber = new HandPositionsDescriber( this.ratio.tupleProperty, tickMarkDescriber, options.inProportionProperty, this.ratio.enabledRatioTermsRangeProperty );
+    this.voicingHandPositionsDescriber = new HandPositionsDescriber( this.ratio.tupleProperty, tickMarkDescriber, options.inProportionProperty, this.ratio.enabledRatioTermsRangeProperty );
     this.tickMarkViewProperty = options.tickMarkViewProperty;
     this.ratioTerm = options.ratioTerm;
 

zepumph added a commit that referenced this issue Apr 18, 2022
@zepumph
Copy link
Member

zepumph commented Apr 22, 2022

A challenging case for this is to convey what input value changed if both hands are at extremities. At the moment I am just going to go down the list "was left at top, if so alert that as the go beyond", "was right at top. . . .". But if left is at top, and right is at bottom, and then I move right down, it would alert as if I press "up" on the left.

@zepumph
Copy link
Member

zepumph commented Apr 22, 2022

I will be quite hard to convey "I tried to move the X hand" from input -> node -> describer -> context response. I'll have to think more about this part as I do the easier stuff.

@zepumph
Copy link
Member

zepumph commented Apr 22, 2022

I have the above case (#457 (comment)) working where edge responses know what term was most recently moved.

Here are two more challenging cases that we aren't currently supporting:

  1. With both hands keyboard, ratio unlocked, press 0, and then press 0 again. It says: hands, extremely close to challenge ratio, Left Hand, at bottom. Move hand up to explore more. This is because there is no "Both hands at bottom". Also the "Left Hand" case is checked in code first, so it is arbitrary that left hand is what is said. I could add "both hands" in this case if needed, but I want to check first.
  2. When ratio is locked, and moving in a direction with the hand that won't reach the extremity, you never hear go-beyond-edge responses. Example:
    1. Screen 2, lock ratio
    2. tab to control left hand
    3. press up arrow 6 times, you should be "at edge" but not beyond yet.
    4. Press up again as many times as you want, you will still hear the "at edge" response of: hands, at challenge ratio, Hands not so close to each other, left at middle, right at top.
  3. UPDATE, here is another: The edge responses are implemented in the "classic" way that each time you ask for an edge response, it compares to the value when you last asked. In order for this strategy to work generally, we have many instances of HandPositionsDescriber, basically one per input modality, this can cause this "bug" (probably not something we care about fixing. . . . When you get in a case where you have a go-beyond-edge response with one modality, then switch to another modality, it won't give the go-beyond-edge response until one extra press. It treats the first go-beyond as if it was you arriving at the edge. Example:
    1. use the left hand and move down to the edge, and go beyond the edge.
    2. Switch to "both hands" with the keyboard "S" key to press down. The first press with not yield a "go-beyond" response, as if it was just arriving at the edge (though it was already there), but the next one will yield a go-beyond edge response.

@zepumph
Copy link
Member

zepumph commented Apr 22, 2022

@terracoda please review.

Summary of changes:

  • Support for both hands go-beyond-edge responses when ratio is not locked
  • Support for indy hands go-beyond-edge responses when ratio is not locked
  • Refactor previous responses when ratio was locked to be the same function as the above.
  • Ensure that mouse/touch drag/endDrag responses don't have go-beyond-edge respones
  • go-beyond edge hint responses on start of mouse/touch.

The code changes weren't terrible, but I do feel like there is still a lot of room for regressions and bugs because of how many factors effect the response: When input modality, whether that input modality is controlling one or both terms of the ratio at the same time (like when ratio is locked), the previous value of the ratio when the context response was last asked for.

Anyway, let me know if you find anything that seems off to you.

@terracoda
Copy link
Contributor Author

terracoda commented Apr 25, 2022

Tests with VOICING

Both Hands unlocked & keyboard:

  • Right hand: working correctly at bottom
  • Right hand: working correctly at top
  • Left hand: working correctly at bottom
  • Left hand: working correctly at top

Indy Slider unlocked & keyboard:

  • Left Hand: working correctly at bottom
  • Left Hand: working correctly at top
  • Right Hand: is working correctly at top
  • Right Hand: is working correctly at bottom

Both Hands with ratio locked & keyboard:

  • Right Hand (at top): working correctly
  • Left Hand (closest it can get to bottom and still be at ratio): working correctly when S key
  • Right Hand (closest it can get to bottom and still be at ratio): I don't get the go beyond response when using the down arrow. Is this intentional?
  • Left Hand (closest it can get to top and still be at ratio): I don't get the go beyond response when using the W key. Is this intentional?

Indy Hands with ratio locked & keyboard (arrow keys only):

  • Right Hand (at top): working correctly
  • Left Hand (closest it can get to bottom and still be at ratio): working correctly
  • Right Hand (closest it can get to bottom and still be at ratio): I don't get the go beyond response. Is this intentional?
  • Left Hand (closest it can get to top and still be at ratio): I don't get the go beyond response Is this intentional?

Mouse Input

  • No go beyond edge responses with mouse input at end of grad - working correctly.
  • Right Hand: working correctly at top
  • Left Hand: working correctly at top
  • Left Hand: Inconsistent at bottom. Works sometimes at bottom if proximity to ratio is far enough off to not play any sound. I get the responses the first time I try to go beyond right after a reset. I can't seem to get it at other times.
  • Right Hand: Inconsistent at bottom. I don't seem to get the go beyond of the the screen is any shade of green & I start to drag Right Hand down . AND I seemed to have crashed the sim once (using Safari) - PhET Test just reloaded.

Sometimes hearing distance-progress:

  • sometimes I hear distance-progress when I start a drag down and I am starting at the bottom. Can we stop this? If the hands don't actually move, we should not hear distance-progress.

@terracoda
Copy link
Contributor Author

Testing with VoiceOver

Both Hands unlocked & keyboard:

  • Edges working correctly (same as Voicing)

Indy Slider unlocked & keyboard:

  • Edges working correctly with Indy sliders (same as Voicing)

Both Hands with ratio locked & keyboard:

  • Edges working correctly for the hand that can get to the top or get closest to bottom (same as Voicing)
  • I don't get the go beyond response when interacting with the hand that cannot get to the edge position (same as Voicing)
  • There seems to be a longish delay before the response fires, longer than when ratio is not locked. Let's check to make sure the delay is not too long.

Indy Hands with ratio locked & keyboard (arrow keys only):

  • Same as the Both Hand interaction & same as Voicing - The beyond edge responses are working correctly for the hand that can get to the top or get closest to the bottom
  • I don't get the beyond response when interacting with the hand that cannot get to the edge position
  • The delay is not long, when I get the beyond response

@zepumph, I think it might be best to leave out the beyond edge responses when a hand cannot get to the edge position. They all seem to be working consistently for the other hand that can actually reach the edge.

The responses with mouse input on Voicing are concerning because they are very inconsistent. maybe we can discuss the best solution there...like maybe no go beyond responses at all for mouse input.

@terracoda
Copy link
Contributor Author

You need to check both indy sliders and when the hands are locked - the hints at the top are not the special hints.

@zepumph
Copy link
Member

zepumph commented May 25, 2022

Alright. I found the bug. It is the exact same javascript imprecise number bug, but this time in the function when we are snapping to a tick mark. It basically mapped the number 1 -> .999999, after (in the code) when the dragListener ensured the value was correctly 1 :)

I am witnessing good coverage for mouse input go-beyond alerts now. Do you see any other issues?

@terracoda
Copy link
Contributor Author

You got it @zepumph! I am closing this issue!

@terracoda terracoda reopened this May 30, 2022
@terracoda
Copy link
Contributor Author

terracoda commented May 30, 2022

Apologies I closed prematurely.

@zepumph, for mouse input, I am getting the hint on mouse up (end of interaction) at the edge, and when I mouse down and a hand is starting at an edge.

Maybe I am miss-understanding something, but I thought for pointer input, we only hear the response on mouse down when starting at an edge, and we won't hear it at end of drag when ending at an edge. For mouse input, when ending at an edge, we have no hint response, though we will have position like "at top" or "near bottom" as part of the last context response.

@terracoda
Copy link
Contributor Author

We really are almost there, @zepumph... in comment #457 (comment)

We agreed that was possible to not hear the special go-beyond-edge hint response when ending a mouse drag at an edge.

@terracoda
Copy link
Contributor Author

Let me know if you need to pair to wrap this issue up.

@terracoda
Copy link
Contributor Author

terracoda commented May 30, 2022

@zepumph, the issue only happens when hands are locked at challenge ratio. The special hints for the indy sliders are working find with pointer input.

This is why I missed it on my first pass :-)

@terracoda
Copy link
Contributor Author

terracoda commented May 30, 2022

And it is happening in the context response, not the hint response. OK, best to document this again...

Mouse Input

  • Design Goal: No go beyond edge responses with mouse input at end of drag
    • NOT working correctly when ratio is locked. When ratio is locked and I use the mouse and end at an edge, I get a special edge response to move in the other direction at end of drag (i.e., when I lift my finger off the mouse), and I get this response in a context response without Helpful Hints checked.
    • Working correctly for indy hand sliders, and the special edge response is in the hint response category for the indy hand sliders.

I think everything else for mouse input is working correctly.

@zepumph
Copy link
Member

zepumph commented Jun 1, 2022

Ok. I found the bug, we were removing the go-beyond edge response for the indy hints manually, but not when the ratio was locked. We are now. Let me know if you see any other weirdness.

@terracoda
Copy link
Contributor Author

Verified Both Hands (ratio locked) for mouse all 3 modes are voicing correctly (No Tick Marks, Tick Marks and Numbered Tick Marks).

Thanks @zepumph. We did it!

Closing this issue.

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