phetsims / buoyancy

"Buoyancy" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
2 stars 2 forks source link

Add accessible names to the PDOM? #109

Open zepumph opened 2 months ago

zepumph commented 2 months ago

From https://github.com/phetsims/buoyancy/issues/76, should we do this? And if so, what should the names be.

samreid commented 1 week ago

Maybe https://github.com/phetsims/scenery/issues/1526#issuecomment-1989274343 is a good way to proceed here? But I'm not sure how screen reader support works if we have keyboard navigation (which I believe is enabled via turning on interactive description) but no description.

zepumph commented 1 week ago

I'd love to experiment with that, and see if there is an easy solution for this sim. Better than nothing!

zepumph commented 2 days ago

This is what the PDOM looks like for PDL, so this wasn't required for that publication.

image
zepumph commented 2 days ago

But here is My Solar System. So there isn't consistency in the alt input feature.

image
AgustinVallejo commented 1 day ago

We'll poke around with the automatic solution and see how it goes.

samreid commented 1 day ago

I'm working on this.

samreid commented 1 day ago

Here is a proposal that creates English keys from the tandems. It is straightforward and I could see taking this to production, but we will need a different approach if we want to automatically support translations other than English.

image
```diff 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( { - tandem: options.tandem.createTandem( 'forcesCheckbox' ) + tandem: options.tandem.createTandem( 'forceValuesCheckbox' ) }, checkboxOptions ) ), ...( model.supportsDepthLines ? [ new Checkbox( model.showDepthLinesProperty, new Text( DensityBuoyancyCommonStrings.depthLinesStringProperty, labelOptions ), combineOptions( { 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 | 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( () => {
samreid commented 1 day ago

Here's an internationalized one that is a little more prototype-y but may lead to a way we can get i18n text in the accessible names. This patch has aqua radio buttons and checkboxes but no combo box.

image
```diff 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 1715914505201) @@ -10,7 +10,7 @@ import validate from '../../axon/js/validate.js'; import { m3 } from '../../dot/js/Matrix3.js'; import InstanceRegistry from '../../phet-core/js/documentation/InstanceRegistry.js'; -import { FireListener, isWidthSizable, LayoutConstraint, Node, NodeOptions, Path, Rectangle, SceneryConstants, TPaint, Voicing, VoicingOptions, WidthSizable, WidthSizableOptions } from '../../scenery/js/imports.js'; +import { FireListener, isWidthSizable, LayoutConstraint, Node, NodeOptions, Path, Rectangle, RichText, SceneryConstants, Text, TPaint, Voicing, VoicingOptions, WidthSizable, WidthSizableOptions } from '../../scenery/js/imports.js'; import checkEmptySolidShape from '../../sherpa/js/fontawesome-4/checkEmptySolidShape.js'; import checkSquareOSolidShape from '../../sherpa/js/fontawesome-4/checkSquareOSolidShape.js'; import checkboxCheckedSoundPlayer from '../../tambo/js/shared-sound-players/checkboxCheckedSoundPlayer.js'; @@ -26,6 +26,7 @@ import { Shape } from '../../kite/js/imports.js'; import StrictOmit from '../../phet-core/js/types/StrictOmit.js'; import PhetioProperty from '../../axon/js/PhetioProperty.js'; +import TReadOnlyProperty from '../../axon/js/TReadOnlyProperty.js'; // constants const BOOLEAN_VALIDATOR = { valueType: 'boolean' }; @@ -69,6 +70,24 @@ export type CheckboxOptions = SelfOptions & StrictOmit; +// TODO: https://github.com/phetsims/buoyancy/issues/109 listen for changes? +export const discoverAccessibleName = ( content: Node ): TReadOnlyProperty => { + + // Recursively search over the content to accumulate all Text or RichText instances: + const textContent: TReadOnlyProperty[] = []; + const discoverText = ( node: Node ): void => { + if ( node instanceof Text || node instanceof RichText ) { + textContent.push( node.stringProperty ); + node.children.forEach( child => discoverText( child ) ); + } + }; + + discoverText( content ); + + // take the first one? + return textContent[ 0 ]; +}; + export default class Checkbox extends WidthSizable( Voicing( Node ) ) { private readonly backgroundNode: Rectangle; @@ -121,6 +140,7 @@ tagName: 'input', inputType: 'checkbox', appendDescription: true, + accessibleName: discoverAccessibleName( content ), // 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 1715914379862) @@ -24,6 +24,7 @@ import ExpandCollapseButton, { ExpandCollapseButtonOptions } from './ExpandCollapseButton.js'; import sun from './sun.js'; import TReadOnlyProperty from '../../axon/js/TReadOnlyProperty.js'; +import { discoverAccessibleName } from './Checkbox.js'; type SelfOptions = { // If not provided, a Text node will be supplied. Should have and maintain well-defined bounds if passed in @@ -198,6 +199,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: providedOptions && providedOptions.titleNode ? discoverAccessibleName( providedOptions.titleNode ) : null, // 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 1715914303747) @@ -18,6 +18,7 @@ import sun from './sun.js'; import StrictOmit from '../../phet-core/js/types/StrictOmit.js'; import TEmitter from '../../axon/js/TEmitter.js'; +import { discoverAccessibleName } from './Checkbox.js'; type SelfOptions = { @@ -119,7 +120,8 @@ containerTagName: 'li', labelTagName: 'label', appendLabel: true, - appendDescription: true + appendDescription: true, + accessibleName: discoverAccessibleName( labelNode ) }, 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( { - tandem: options.tandem.createTandem( 'forcesCheckbox' ) + tandem: options.tandem.createTandem( 'forceValuesCheckbox' ) }, checkboxOptions ) ), ...( model.supportsDepthLines ? [ new Checkbox( model.showDepthLinesProperty, new Text( DensityBuoyancyCommonStrings.depthLinesStringProperty, labelOptions ), combineOptions( { 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 | 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( () => {
samreid commented 1 day ago

By the way, we could just pass the actual accessible names manually without automatically discovering them and it would be very easy.

zepumph commented 22 hours ago

I think we should move towards a commit point on the tandem-name-only one. Here are some review comments, and let's discuss in person:

Let's keep discussing!