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

Should a11yCreateAriaValueText be required to prevent description percents on iOS? #682

Closed
zepumph opened this issue Jan 30, 2021 · 6 comments
Assignees

Comments

@zepumph
Copy link
Member

zepumph commented Jan 30, 2021

From phetsims/ratio-and-proportion#283, my findings were basically that any AccessibleValueHandler that we want to support for Interactive Description needs to pass in a a11yCreateAriaValueText that differs from the current value. In that sim-specific issue, we got around this by spelling out the number as a word. I'm not sure if this is the most maintainable, but it fixed the bug.

Here is a note from MDN https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/range:

As a rule, if the user is more likely to be interested in the percentage of the distance between minimum and maximum values than the actual number itself, a range input is a great candidate. For example, in the case of a home stereo volume control, users typically think "set volume at halfway to maximum" instead of "set volume to 0.5".

So it is in the implementation of range that it is for percents. Part of some research done over in #681 was looking at input[type="number"], but I felt like the ability to input numbers directly is a non-starter for our NumberPicker implementation.

Basically I worry about any AccessibleValueHandler that we think of as "fully implemented" with Interactive Description if it doesn't set its own a11yCreateAriaValueText function. On iOS, we get the value stated as a percent (from min to max attribute values).

@jessegreenberg, can you think of a way that we can begin to require this feature to make sure that something like phetsims/ratio-and-proportion#283 doesn't occur again?

@jessegreenberg
Copy link
Contributor

jessegreenberg commented Feb 1, 2021

Instead of requiring this maybe we could try a different default function for a11yCreateAriaValueText. Rather than just toString we could try adding an extra 'hairspace' like we did to support _a11yRepeatEqualValueText?

However, if Apple has decided that the best output for sliders is to report the percentage, would it be acceptable for PhET to allow it unless it is pedagogically problematic? Or we could submit a feature request to Apple to remove it and accept it as the platform behavior for now.

@zepumph
Copy link
Member Author

zepumph commented Feb 1, 2021

To me the subtext of this issue is that PhET is using the wrong HTML input. Ideally it would be number instead of range, since range conveys percentage semantics, at least in many cases. NumberPicker is not meant to be used in a percentage case, so I don't think that a default of allowing percentages makes too much sense. In a perfect world, we would probably be using type number, but I don't think this issue should take on that work.

I tried converting the default to add a hairspace in this patch. It still gave percents in the my challenge number pickers in RAP.

Index: sun/js/accessibility/AccessibleValueHandler.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/sun/js/accessibility/AccessibleValueHandler.js b/sun/js/accessibility/AccessibleValueHandler.js
--- a/sun/js/accessibility/AccessibleValueHandler.js	(revision faf9b4055168c78b927b2a171a8fbe33464f8f05)
+++ b/sun/js/accessibility/AccessibleValueHandler.js	(date 1612208446158)
@@ -31,7 +31,8 @@
 
 // constants
 const DEFAULT_TAG_NAME = 'input';
-const toString = v => v + '';
+const hairSpace = '\u200A';
+const addHairSpace = v => v + hairSpace;
 
 const AccessibleValueHandler = {
 
@@ -149,7 +150,7 @@
            *                              to be called when the AccessibleValueHandler is reset.
            * @returns {string} - aria-valuetext to be set to the primarySibling
            */
-          a11yCreateAriaValueText: toString, // by default make sure it returns a string
+          a11yCreateAriaValueText: addHairSpace, // by default make sure it returns a string
 
           /**
            * Create content for an alert that will be sent to the utteranceQueue when the user finishes interacting
@@ -387,9 +388,8 @@
         // Make sure that the new aria-valuetext is different from the previous one, so that if they are the same
         // the screen reader will still read the new text - adding a hairSpace registers as a new string, but the
         // screen reader won't read that character.
-        const hairSpace = '\u200A';
         if ( this._a11yRepeatEqualValueText && this.ariaValueText && newAriaValueText === this.ariaValueText.replace( new RegExp( hairSpace, 'g' ), '' ) ) {
-          newAriaValueText = this.ariaValueText + hairSpace;
+          newAriaValueText = addHairSpace( this.ariaValueText );
         }
 
         this.ariaValueText = newAriaValueText;
Index: ratio-and-proportion/js/create/view/MyChallengeAccordionBox.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/ratio-and-proportion/js/create/view/MyChallengeAccordionBox.js b/ratio-and-proportion/js/create/view/MyChallengeAccordionBox.js
--- a/ratio-and-proportion/js/create/view/MyChallengeAccordionBox.js	(revision cdf28502ec162f2fecaf99a5e1bd5be18979962a)
+++ b/ratio-and-proportion/js/create/view/MyChallengeAccordionBox.js	(date 1612208446164)
@@ -96,7 +96,7 @@
       center: Vector2.ZERO,
       accessibleName: ratioAndProportionStrings.a11y.leftValue,
       a11yDependencies: [ targetConsequentProperty ],
-      a11yCreateAriaValueText: ratioDescriber.getWordFromNumber,
+      // a11yCreateAriaValueText: ratioDescriber.getWordFromNumber,
       a11yCreateContextResponseAlert: () => ratioDescriber.getTargetRatioChangeAlert( targetAntecedentProperty.value, targetConsequentProperty.value )
     } );
     const leftRatioSelector = new VBox( {
@@ -115,7 +115,7 @@
       center: Vector2.ZERO,
       accessibleName: ratioAndProportionStrings.a11y.rightValue,
       a11yDependencies: [ targetAntecedentProperty ],
-      a11yCreateAriaValueText: ratioDescriber.getWordFromNumber,
+      // a11yCreateAriaValueText: ratioDescriber.getWordFromNumber,
       a11yCreateContextResponseAlert: () => ratioDescriber.getTargetRatioChangeAlert( targetAntecedentProperty.value, targetConsequentProperty.value )
     } );
     const rightRatioSelector = new VBox( {

@zepumph zepumph assigned jessegreenberg and unassigned zepumph Feb 1, 2021
@jessegreenberg
Copy link
Contributor

jessegreenberg commented Feb 1, 2021

To me the subtext of this issue is that PhET is using the wrong HTML input.

I agree, that makes sense to me. Thanks for checking the hairspace workaround. In my opinion requiring a map that would often just be from number -> numeric word is a last resort. I would be more interested in maybe trying a different HTML element, if this is evidence that range input is semantically not a good fit for NumberPicker. Maybe that means having AccessibleValueHandler support more than input type='range', or maybe it means NumberPicker doesn't use AccessibleValueHandler. But I understand that broadens the scope quite a bit and involve checking with pdom designers.

@zepumph
Copy link
Member Author

zepumph commented Feb 1, 2021

This code in handleInput makes me worried that we would not be able to use too much of the same code if we went with inputType:'number':

if ( inputValue > mappedValue ) {
newValue = this._valueProperty.get() + stepSize;
}
else if ( inputValue < mappedValue ) {
newValue = this._valueProperty.get() - stepSize;
}

but even before that, inputType:'number' is a completely different UI, which allows for typing in numbers directly (which aren't visible on the NumberPicker). I'm worried about how we would support that at all. My vote is to create an issue and defer it. What do you think @jessegreenberg?

@zepumph zepumph assigned jessegreenberg and unassigned zepumph Feb 1, 2021
@jessegreenberg
Copy link
Contributor

I agree, I don't think we can use inputType: 'number' for the reason you described. That input also has native keyboard input that I recall being tricky to prevent. I was thinking we might either create something new with a div and aria-roledescription or maybe put the arrow buttons back in the navigation order. But I am also happy to create a new issue and defer.

@zepumph
Copy link
Member Author

zepumph commented Feb 1, 2021

I really feel like the problem doesn't outweigh the cost of a solution at this time. I'll make a new issue.

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

No branches or pull requests

2 participants