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

Add accessible names to the PDOM? #109

Closed
zepumph opened this issue Mar 1, 2024 · 17 comments
Closed

Add accessible names to the PDOM? #109

zepumph opened this issue Mar 1, 2024 · 17 comments
Assignees
Labels
dev:help-wanted Extra attention is needed

Comments

@zepumph
Copy link
Member

zepumph commented Mar 1, 2024

From #76, should we do this? And if so, what should the names be.

@samreid
Copy link
Member

samreid commented May 9, 2024

Maybe phetsims/scenery#1526 (comment) 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
Copy link
Member Author

zepumph commented May 9, 2024

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

@zepumph
Copy link
Member Author

zepumph commented May 16, 2024

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

image

@zepumph
Copy link
Member Author

zepumph commented May 16, 2024

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

image

@AgustinVallejo
Copy link
Contributor

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

@samreid
Copy link
Member

samreid commented May 17, 2024

I'm working on this.

@samreid
Copy link
Member

samreid commented May 17, 2024

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
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( () => {

@samreid
Copy link
Member

samreid commented May 17, 2024

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
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<ParentOptions, 'children' | 'mouseArea' | 'touchArea'>;
 
+// TODO: https://github.com/phetsims/buoyancy/issues/109 listen for changes?
+export const discoverAccessibleName = ( content: Node ): TReadOnlyProperty<string> => {
+
+  // Recursively search over the content to accumulate all Text or RichText instances:
+  const textContent: TReadOnlyProperty<string>[] = [];
+  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<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( () => {

@samreid
Copy link
Member

samreid commented May 17, 2024

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

@samreid samreid removed their assignment May 17, 2024
@zepumph
Copy link
Member Author

zepumph commented May 17, 2024

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:

  • toAccessibleName should return undefined instead of null, so that it doesn't override a potential supertype default.
  • I see hard coded suffixes that I want to be able to use PhetioObject.tandemNameSuffix for.

Let's keep discussing!

@zepumph zepumph added the dev:help-wanted Extra attention is needed label May 17, 2024
@zepumph
Copy link
Member Author

zepumph commented May 22, 2024

@AgustinVallejo @samreid @zepumph really like to proceed with 2 things:

  1. Commit the patch that provides default accessible names to common code from the tandem names. This default will be great to have both for development, and in cases where we publish sims that otherwise have a completely label-less PDOM.
  2. @samreid recommends to manually apply accessibleNames from translated strings in DBC. In the majoirity of cases we already have the i18n string because of visual text anyways. This sounds great!]
  3. In cases where there isn't i18n strings already ( like a radio button group for scenes), @samreid can choose if he wants to use the tandem name default, or add a new i18n string.

@zepumph zepumph removed the dev:help-wanted Extra attention is needed label May 22, 2024
@zepumph zepumph assigned samreid and unassigned zepumph May 22, 2024
@AgustinVallejo AgustinVallejo self-assigned this May 23, 2024
@AgustinVallejo
Copy link
Contributor

After talking with @samreid we decided that it's best to manually apply the fields that already have i18n, and automate the rest with tandem. Assigning myself to start on (2)

AgustinVallejo added a commit to phetsims/density-buoyancy-common that referenced this issue May 23, 2024
AgustinVallejo added a commit to phetsims/density-buoyancy-common that referenced this issue May 27, 2024
@AgustinVallejo
Copy link
Contributor

Should we add a tag name for scales? Currently they show up in PDOM as NONE

AgustinVallejo added a commit to phetsims/density-buoyancy-common that referenced this issue May 27, 2024
AgustinVallejo added a commit to phetsims/density-buoyancy-common that referenced this issue May 27, 2024
AgustinVallejo added a commit to phetsims/density-buoyancy-common that referenced this issue May 27, 2024
AgustinVallejo added a commit to phetsims/density-buoyancy-common that referenced this issue May 28, 2024
@samreid samreid added the dev:help-wanted Extra attention is needed label May 28, 2024
samreid added a commit to phetsims/density-buoyancy-common that referenced this issue May 28, 2024
samreid added a commit to phetsims/sun that referenced this issue May 28, 2024
samreid added a commit to phetsims/tandem that referenced this issue May 28, 2024
@samreid
Copy link
Member

samreid commented May 28, 2024

I added the first occurrence of defaulting the accessibleName based on the tandem, if it is supplied. To begin, I added this for RectangularRadioButton which is a component that doesn't have a pre-existing string in the translation file.

Note this also benefited other sims like Faraday's Electromagnetic Lab.

Before:

image

After:

image

Heads up to @zepumph and @pixelzoom.

samreid added a commit to phetsims/density-buoyancy-common that referenced this issue May 28, 2024
@samreid
Copy link
Member

samreid commented May 28, 2024

I checked through density, buoyancy and density-buoyancy and all the a11y views look complete. For this issue, I do not feel we should add more tandem-based automation that will not be exercised in this context. But we have established the convention and made it easy to supplement as we proceed. @AgustinVallejo can this issue be closed? Should we put the remainder of the tandem patch above into a sun issue?

@AgustinVallejo
Copy link
Contributor

I think the scale height slider could benefit from this as well, and then this issue can be closed.

@AgustinVallejo
Copy link
Contributor

This is done :) Please reopen if you consider there's more work to do here

samreid added a commit to phetsims/sun that referenced this issue Oct 21, 2024
…sed on a tandem name, if it is supplied, see phetsims/buoyancy#109"

This reverts commit 018120a.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dev:help-wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants