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

[AccessibleValueHandler] add option to map value before setting Property value #669

Closed
zepumph opened this issue Nov 13, 2020 · 8 comments
Closed

Comments

@zepumph
Copy link
Member

zepumph commented Nov 13, 2020

Over in phetsims/ratio-and-proportion#175, we need a bit more control over the valueProperty being set by AccessibleValueHandler. We need to detect a case before setting the value. I think the best way to do this is to create a new option that is called before constrainValue is called. I thought about altering constrainValue, but since Slider.js supports that option, it would be best to keep the implementations consistent between Slider and AccessibleValueHandler.

@zepumph zepumph self-assigned this Nov 13, 2020
zepumph added a commit to phetsims/inverse-square-law-common that referenced this issue Nov 13, 2020
zepumph added a commit to phetsims/gravity-force-lab-basics that referenced this issue Nov 13, 2020
zepumph added a commit to phetsims/resistance-in-a-wire that referenced this issue Nov 13, 2020
zepumph added a commit to phetsims/area-model-common that referenced this issue Nov 13, 2020
zepumph added a commit to phetsims/ohms-law that referenced this issue Nov 13, 2020
zepumph added a commit that referenced this issue Nov 13, 2020
@zepumph
Copy link
Member Author

zepumph commented Nov 13, 2020

I renamed the PDOM value mapper and created a11yMapValue I tried to explain how one maps the Property value as it pertains to keyboard input, and the other is for mapping the value to a number to be displayed by assistive technology. This work was necessary for phetsims/ratio-and-proportion#175, but it adds another option, and some complexity to AccessibleValueHandler. @jessegreenberg please review.

@zepumph
Copy link
Member Author

zepumph commented Nov 17, 2020

Please also note 278bd57 while reviewing.

@zepumph
Copy link
Member Author

zepumph commented Nov 17, 2020

While working on phetsims/ratio-and-proportion#175, I have asked myself the following question three times:

"Why have this "map" function? Why not just use constrain value?"

I'll answer it again explicitly here for the record, even though it is reiterating from the original comment in this issue. While the placement in AccessibleValueHandler makes me think that it could be interchangable, constrainValue is an option in Slider, so it is important to keep the same function signature for that function. this new "a11yMapValue` function has the flexibility of parameters we need to pass for particular a11y cases (like shiftKey).

@jessegreenberg
Copy link
Contributor

Changes look good, and see the change at 278bd57.

I am curious though if this could have been handled by the roundToStepSize option. I reviewed phetsims/ratio-and-proportion#175 and it seems similar to the behavior in ohms-law, where you can drag the sliders outside of the keyboard step intervals with a mouse, but next alternative input will snap the value to the nearest interval of keyboard step. @zepumph can you comment?

@jessegreenberg
Copy link
Contributor

Also, setMapValueFunction is unused should that be removed now?

zepumph added a commit that referenced this issue Jan 28, 2021
@zepumph
Copy link
Member Author

zepumph commented Jan 28, 2021

Yeah good idea. Let's simplify the API as we can. No need to add mutation until there is a good reason to.

@zepumph zepumph closed this as completed Jan 28, 2021
@zepumph zepumph reopened this Jan 28, 2021
@zepumph
Copy link
Member Author

zepumph commented Jan 28, 2021

roundToStepSize is not quite enough functionality, because for Ratio and Proportion we needed to also support snapping directly to a value in between snaps that would create the "in proportion" state. That was important for usability and modality equity.

Anything else here?

@zepumph zepumph assigned jessegreenberg and unassigned zepumph Jan 28, 2021
@jessegreenberg
Copy link
Contributor

OK, thanks for clarifying, understood. Closing.

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