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

Automatically discover a11y text #1526

Open
samreid opened this issue Jan 21, 2023 · 12 comments
Open

Automatically discover a11y text #1526

samreid opened this issue Jan 21, 2023 · 12 comments

Comments

@samreid
Copy link
Member

samreid commented Jan 21, 2023

In discussion with @jessegreenberg, we saw that visual textual information was not exposed to the a11y tree by default. We had an idea to pluck button/checkbox labels and move them to the PDOM automatically as a default.

@samreid
Copy link
Member Author

samreid commented Feb 17, 2023

It seems like a small amount of effort could dramatically improve our screen reader coverage. But this issue seems like it is complex enough that it should be moved to an iteration + subteam. So I'll propose it on the upcoming priorities board, and unassign myself.

@samreid samreid removed their assignment Feb 17, 2023
@jessegreenberg jessegreenberg removed their assignment Feb 22, 2023
@terracoda
Copy link

This is a great idea @samreid and could lead to knowledge sharing around creating names for objects that work well in both the visual space and the described space. Totally, agree that default on-screen text is better than no text and step in the right direction.

@samreid
Copy link
Member Author

samreid commented Feb 14, 2024

@terracoda indicated difficulty in designing the keyboard traversal order and separating play area vs control area (see phetsims/projectile-data-lab#107) in Projectile Data Lab because the a11y text is all blank:

image

Here is a patch that takes information from the PhET-iO IDs to populate the a11y view:

Subject: [PATCH] Pass launchButtonEnabledProperty directly to the launchButton.enabledProperty, see https://github.com/phetsims/projectile-data-lab/issues/141
---
Index: js/accessibility/pdom/ParallelDOM.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/accessibility/pdom/ParallelDOM.ts b/js/accessibility/pdom/ParallelDOM.ts
--- a/js/accessibility/pdom/ParallelDOM.ts	(revision 42fefc31fc2910edfd75a72c69e33e78b088c84a)
+++ b/js/accessibility/pdom/ParallelDOM.ts	(date 1707927532191)
@@ -141,6 +141,7 @@
 import TProperty from '../../../../axon/js/TProperty.js';
 import isSettingPhetioStateProperty from '../../../../tandem/js/isSettingPhetioStateProperty.js';
 import Bounds2 from '../../../../dot/js/Bounds2.js';
+import { animationFrameTimer } from '../../../../axon/js/imports.js';
 
 const INPUT_TAG = PDOMUtils.TAGS.INPUT;
 const P_TAG = PDOMUtils.TAGS.P;
@@ -632,6 +633,16 @@
     this.focusHighlightChangedEmitter = new TinyEmitter();
     this.pdomDisplaysEmitter = new TinyEmitter();
     this.pdomBoundInputEnabledListener = this.pdomInputEnabledListener.bind( this );
+
+    animationFrameTimer.setTimeout( () => {
+      if ( this.accessibleName === null && this.tandem && this.tandem.supplied ) {
+        let text = this.phetioID.substring( this.phetioID.lastIndexOf( '.' ) + 1 );
+        if ( text.endsWith( 'RadioButton' ) ) {
+          text = text.substring( 0, text.length - 'RadioButton'.length );
+        }
+        this.accessibleName = text;
+      }
+    }, 1000 );
   }
 
   /***********************************************************************************************************/

With this patch, the a11y view looks like this:

image

Would be good to get thoughts from @terracoda about whether this is helpful enough or from @jessegreenberg about how we might bring an idea like this to production (even if only for team/development purposes).

@terracoda
Copy link

Thanks @samreid, I'll look again at the A11y View.

@samreid
Copy link
Member Author

samreid commented Feb 14, 2024

The proposal above is not committed, it is a proposal to get the ball rolling toward a committable solution. Does it seem useful?

@terracoda
Copy link

terracoda commented Feb 14, 2024

I think it could be useful. I see some useful overlap with the description design tool. It pre-populates the accessible name with something unique and human readable.

@terracoda
Copy link

Upgrading to very useful.

@jessegreenberg
Copy link
Contributor

jessegreenberg commented Feb 14, 2024

Yes, cool idea! This seems great for internal use, I could see adding a query parameter that does this.

Thinking about the best place to put it, maybe somewhere in joist/SimDisplay? Like maybe we could override updateDisplay and add a walk down the AccessibleInstance tree to set these names?

I will add this to the alt-input Monday board and there we can determine a priority for adding this (unless it gets added organically sooner though I don't think I will have time for a while).

@samreid
Copy link
Member Author

samreid commented Feb 15, 2024

Adding to the Monday board and scheduling for a future subteam sounds good. Some questions for then:

  • Is there a way we can make this the default behavior, not behind a query parameter? For instance, if we automatically change the camel casing to spaced, like "Mystery Launcher 1" radio button instead of mysteryLauncher1 radio button, could it be good enough to use without a query parameter? What else would need to be done before we make it available to screen reader users? Is it "something is better than nothing" or "we shouldn't add anything if we can't meet a certain quality/quantity threshold"?
  • I was unclear about the ParallelDOM lifecycle. I thought tapping in to the constructor or mutate would be the correct time to know that accessibleName is null but the tandem is specified. But I couldn't get that to work. But likely something along those lines could be helpful.
  • There are ideas at the top of the issue about using the translatable visually presented strings here. We could design a scaffolded system where: If the accessibleName is provided, it takes precedence. If not, check if the human visible string is accessible, such as a label on a button or next to a checkbox. If not, check if the tandem was supplied, and convert it from camelcase to readable.

@samreid samreid removed their assignment Feb 15, 2024
@jessegreenberg jessegreenberg removed their assignment Feb 22, 2024
@zepumph
Copy link
Member

zepumph commented Feb 23, 2024

The alt-input management team discussed this today and we would like to loop this into the greater description work:
phetsims/joist#941

@marlitas
Copy link
Contributor

Meeting 3/11/24

  • We need to explore how screen readers will treat camelCase names.
  • @terracoda mentioned that a name is better than no name.
  • It would most definitely be helpful at least in the a11y view. We can do this via a query parameter.
  • Further discussion on wether we want to publish these names with sims is needed.

@AgustinVallejo
Copy link
Contributor

@samreid @zepumph and I worked on this as part of phetsims/buoyancy#109 and they wrote a patch for doing this automatically in some sun components. Leaving it here in case it's useful for future work

Subject: [PATCH] Pass launchButtonEnabledProperty directly to the launchButton.enabledProperty, see https://github.com/phetsims/projectile-data-lab/issues/141
---
Index: sun/js/Checkbox.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/sun/js/Checkbox.ts b/sun/js/Checkbox.ts
--- a/sun/js/Checkbox.ts	(revision 3cd5c702159e13688afd1b0b9da468f129ccee71)
+++ b/sun/js/Checkbox.ts	(date 1715911723171)
@@ -121,6 +121,7 @@
       tagName: 'input',
       inputType: 'checkbox',
       appendDescription: true,
+      accessibleName: Tandem.toAccessibleName( providedOptions, 'Checkbox' ),
 
       // voicing
       voicingCheckedObjectResponse: null,
Index: sun/js/ComboBoxListItemNode.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/sun/js/ComboBoxListItemNode.ts b/sun/js/ComboBoxListItemNode.ts
--- a/sun/js/ComboBoxListItemNode.ts	(revision 3cd5c702159e13688afd1b0b9da468f129ccee71)
+++ b/sun/js/ComboBoxListItemNode.ts	(date 1715912676366)
@@ -69,6 +69,7 @@
       tagName: 'li',
       focusable: true,
       ariaRole: 'option',
+      accessibleName: Tandem.toAccessibleName( providedOptions, 'Item' ),
 
       // the `li` with ariaRole `option` does not get click events on iOS VoiceOver, so position
       // elements so they receive pointer events
Index: sun/js/ComboBoxButton.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/sun/js/ComboBoxButton.ts b/sun/js/ComboBoxButton.ts
--- a/sun/js/ComboBoxButton.ts	(revision 3cd5c702159e13688afd1b0b9da468f129ccee71)
+++ b/sun/js/ComboBoxButton.ts	(date 1715912953495)
@@ -105,7 +105,8 @@
       // pdom
       containerTagName: 'div',
       labelTagName: 'p', // NOTE: A `span` causes duplicate name-speaking with VO+safari in https://github.com/phetsims/ratio-and-proportion/issues/532
-      accessibleNameBehavior: ACCESSIBLE_NAME_BEHAVIOR
+      accessibleNameBehavior: ACCESSIBLE_NAME_BEHAVIOR,
+      // accessibleName: Tandem.toAccessibleName( providedOptions, 'ComboBoxButton' )
     }, providedOptions );
 
     assert && assert( _.includes( ALIGN_VALUES, options.align ),
Index: sun/js/AccordionBox.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/sun/js/AccordionBox.ts b/sun/js/AccordionBox.ts
--- a/sun/js/AccordionBox.ts	(revision 3cd5c702159e13688afd1b0b9da468f129ccee71)
+++ b/sun/js/AccordionBox.ts	(date 1715912565653)
@@ -198,6 +198,7 @@
       tagName: 'div',
       headingTagName: 'h3', // specify the heading that this AccordionBox will be, TODO: use this.headingLevel when no longer experimental https://github.com/phetsims/scenery/issues/855
       accessibleNameBehavior: AccordionBox.ACCORDION_BOX_ACCESSIBLE_NAME_BEHAVIOR,
+      accessibleName: Tandem.toAccessibleName( providedOptions, 'AccordionBox' ),
 
       // voicing
       voicingNameResponse: null,
Index: sun/js/ComboBox.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/sun/js/ComboBox.ts b/sun/js/ComboBox.ts
--- a/sun/js/ComboBox.ts	(revision 3cd5c702159e13688afd1b0b9da468f129ccee71)
+++ b/sun/js/ComboBox.ts	(date 1715913290569)
@@ -244,6 +244,7 @@
       buttonLabelTagName: 'p',
       accessibleNameBehavior: ACCESSIBLE_NAME_BEHAVIOR,
       helpTextBehavior: HELP_TEXT_BEHAVIOR,
+      accessibleName: Tandem.toAccessibleName( providedOptions, 'ComboBox' ),
 
       comboBoxVoicingNameResponsePattern: SunConstants.VALUE_NAMED_PLACEHOLDER,
       comboBoxVoicingContextResponse: null,
@@ -565,6 +566,9 @@
       else if ( typeof item.a11yName === 'string' ) {
         property = new TinyProperty( item.a11yName );
       }
+      else if ( item.tandemName ) {
+        property = new TinyProperty( Tandem.tandemNameToAccessibleName( item.tandemName, 'Item' ) );
+      }
       else {
         property = new TinyProperty( null );
       }
Index: sun/js/AquaRadioButton.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/sun/js/AquaRadioButton.ts b/sun/js/AquaRadioButton.ts
--- a/sun/js/AquaRadioButton.ts	(revision 3cd5c702159e13688afd1b0b9da468f129ccee71)
+++ b/sun/js/AquaRadioButton.ts	(date 1715912110736)
@@ -119,7 +119,8 @@
       containerTagName: 'li',
       labelTagName: 'label',
       appendLabel: true,
-      appendDescription: true
+      appendDescription: true,
+      accessibleName: Tandem.toAccessibleName( providedOptions, 'RadioButton' )
 
     }, providedOptions );
 
Index: density-buoyancy-common/js/common/view/BuoyancyDisplayOptionsPanel.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/density-buoyancy-common/js/common/view/BuoyancyDisplayOptionsPanel.ts b/density-buoyancy-common/js/common/view/BuoyancyDisplayOptionsPanel.ts
--- a/density-buoyancy-common/js/common/view/BuoyancyDisplayOptionsPanel.ts	(revision f4549040101bae01a024fc86025259d7e038e20d)
+++ b/density-buoyancy-common/js/common/view/BuoyancyDisplayOptionsPanel.ts	(date 1715912187354)
@@ -136,7 +136,7 @@
                   tandem: options.tandem.createTandem( 'massesCheckbox' )
                 }, checkboxOptions ) ),
                 new Checkbox( model.showForceValuesProperty, new Text( DensityBuoyancyCommonStrings.forceValuesStringProperty, labelOptions ), combineOptions<CheckboxOptions>( {
-                  tandem: options.tandem.createTandem( 'forcesCheckbox' )
+                  tandem: options.tandem.createTandem( 'forceValuesCheckbox' )
                 }, checkboxOptions ) ),
                 ...( model.supportsDepthLines ?
                   [ new Checkbox( model.showDepthLinesProperty, new Text( DensityBuoyancyCommonStrings.depthLinesStringProperty, labelOptions ), combineOptions<CheckboxOptions>( {
Index: tandem/js/Tandem.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/tandem/js/Tandem.ts b/tandem/js/Tandem.ts
--- a/tandem/js/Tandem.ts	(revision 27c3e2c9daa7165780d04465016c2e898a96c192)
+++ b/tandem/js/Tandem.ts	(date 1715913246619)
@@ -11,9 +11,10 @@
 import arrayRemove from '../../phet-core/js/arrayRemove.js';
 import merge from '../../phet-core/js/merge.js';
 import optionize from '../../phet-core/js/optionize.js';
-import PhetioObject from './PhetioObject.js';
+import PhetioObject, { PhetioObjectOptions } from './PhetioObject.js';
 import TandemConstants, { PhetioID } from './TandemConstants.js';
 import tandemNamespace from './tandemNamespace.js';
+import PickOptional from '../../phet-core/js/types/PickOptional.js';
 
 // constants
 // Tandem can't depend on joist, so cannot use packageJSON module
@@ -597,6 +598,29 @@
    * Use this as the parent tandem for Properties that are related to sim-specific preferences.
    */
   public static readonly PREFERENCES = Tandem.GLOBAL_MODEL.createTandem( 'preferences' );
+
+  /**
+   * Tandem names can be used to create accessible names for screen readers. This method will convert a tandem name to
+   * a human-readable name. For example, 'resetAllButton' would become 'Reset All'.
+   */
+  public static toAccessibleName( providedOptions: PickOptional<PhetioObjectOptions, 'tandem'> | undefined, suffix: string ): string | null {
+    if ( providedOptions && providedOptions.tandem ) {
+      return Tandem.tandemNameToAccessibleName( providedOptions.tandem.name, suffix );
+    }
+    return null;
+  }
+
+  public static tandemNameToAccessibleName( tandemName: string, suffix: string ): string | null {
+    assert && assert( tandemName.toLowerCase().endsWith( suffix.toLowerCase() ), `suffix should be at the end of the tandem name: ${tandemName}` );
+
+    // trim the suffix
+    const withoutSuffix = tandemName.slice( 0, -suffix.length );
+
+    const whitespaceName = withoutSuffix.replace( /([A-Z])/g, ' $1' ).trim();
+
+    // capitalize the first letter of each word, no matter how many words
+    return whitespaceName.replace( /\b\w/g, c => c.toUpperCase() );
+  }
 }
 
 Tandem.addLaunchListener( () => {

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

6 participants