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

HandPositionsDescriber.getDistanceProgressString() is not idempotent #416

Closed
zepumph opened this issue Dec 3, 2021 · 3 comments
Closed
Assignees

Comments

@zepumph
Copy link
Member

zepumph commented Dec 3, 2021

We are running into a problem here over in #413 because we want to use the "closer/farther" distance-progress strings in both description and voicing. When you call that function, it registers the previous value and overwrites the distance. It was designed to be called only when delivering an alert to the user during interaction. So in addition to the challenge of using it for both description and voicing, updating it eagerly, like we do for the object response, will not work.

@zepumph
Copy link
Member Author

zepumph commented Jan 14, 2022

This would be nice to do so that @terracoda can test voicing with distance-region AND distance-progress these days

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 761e35ea3f4c79a09fbc1350e89bc4a791f43c4d)
+++ b/js/common/view/RatioHalf.ts	(date 1642202950465)
@@ -291,7 +291,9 @@
 
         a11yCreateAriaValueText: createObjectResponse,
         voicingCreateObjectResponse: createObjectResponse,
-        a11yCreateContextResponseAlert: () => this.getSingleHandContextResponse(),
+
+        // TODO: comment back in once https://github.com/phetsims/ratio-and-proportion/issues/416 is fixed
+        // a11yCreateContextResponseAlert: () => this.getSingleHandContextResponse(),
         voicingCreateContextResponse: () => this.getSingleHandContextResponse(),
         a11yDependencies: options.a11yDependencies.concat( [ options.ratioLockedProperty ] ),
         voicingNameResponse: options.accessibleName // accessible name is also the voicing name response

@zepumph
Copy link
Member Author

zepumph commented Jan 21, 2022

Committed in 71ae824

@zepumph
Copy link
Member Author

zepumph commented Jan 21, 2022

Fixed above, it was easiest to create a separate instance of HandPositionsDescriber that was just for voicing. Then I also had to split up the "previous" region for both hands vs single.

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

1 participant