fixed for various a11y options, #314

phetsims · Jan 22, 2019 · 0daad07 · 0daad07
1 parent 7c1e2ec
commit 0daad07
Show file tree

Hide file tree

Showing 4 changed files with 20 additions and 25 deletions.
diff --git a/js/ComboBox.js b/js/ComboBox.js
@@ -62,7 +62,8 @@ define( require => {
         listStroke: 'black', // {Color|string}
         listLineWidth: 1,
 
-        //TODO sun#314 need to add a11y options?
+        // a11y
+        buttonLabelContent: null, // {string|null} propagated to ComboBoxButton via its options.labelContent
 
         // phet-io
         tandem: Tandem.required,
@@ -101,7 +102,8 @@ define( require => {
         stroke: options.buttonStroke,
         lineWidth: options.buttonLineWidth,
 
-        //TODO sun#314 need to add a11y options?
+        // a11y
+        labelContent: options.buttonLabelContent,
 
         // phet-io
         tandem: options.tandem.createTandem( 'button' )

diff --git a/js/ComboBoxButton.js b/js/ComboBoxButton.js
@@ -56,7 +56,7 @@ define( require => {
         // a11y
         labelTagName: 'span',
         containerTagName: 'div',
-        a11yLabel: null // {string|null}
+        labelContent: null // {string|null}
 
       }, options );
 
@@ -136,15 +136,12 @@ define( require => {
         // remove the node for the previous item
         itemNodeWrapper.removeAllChildren();
 
-        // find and add the node for the new item
-        let node = null;
-        for ( var i = 0; i < items.length && !node; i++ ) {
-          if ( items[ i ].value === value ) {
-            node = items[ i ].node;
-          }
-        }
-        assert && assert( node, 'no item found for value: ' + value );
-        itemNodeWrapper.addChild( node );
+        // find the ComboBoxItem whose value matches the property's value
+        const item = _.find( items, item => item.value === value );
+        assert && assert( item, 'no item found for value: ' + value );
+
+        // add the associated node
+        itemNodeWrapper.addChild( item.node );
 
         // adjust alignment
         itemNodeWrapper.centerY = itemAreaStrut.centerY;
@@ -157,12 +154,12 @@ define( require => {
         else {
           itemNodeWrapper.centerX = itemAreaStrut.centerX;
         }
+
+        // a11y
+        this.innerContent = item.a11yLabel;
       };
       property.link( propertyObserver );
 
-      //TODO sun#314 is this handled by super? why not?
-      this.labelContent = options.a11yLabel;
-
       //TODO sun#314 expand on this comment
       // the button is labelledby its own label, and then (second) by itself. Order matters!
       assert && assert( !options.ariaLabelledbyAssociations, 'ComboBoxButton sets ariaLabelledbyAssociations' );

diff --git a/js/ComboBoxListBox.js b/js/ComboBoxListBox.js
@@ -135,8 +135,7 @@ define( require => {
           xMargin: 0.5 * options.xMargin,
           left: 0.5 * options.xMargin,
           top: ( 0.5 * options.yMargin ) + ( index * highlightHeight ),
-          tandem: item.tandemName ? tandem.createTandem( item.tandemName ) : Tandem.optional,
-          a11yLabel: item.a11yLabel
+          tandem: item.tandemName ? tandem.createTandem( item.tandemName ) : Tandem.optional
         } );
         listItemNodes.push( listItemNode );
 

diff --git a/js/ComboBoxListItemNode.js b/js/ComboBoxListItemNode.js
@@ -44,11 +44,14 @@ define( require => {
         // a11y
         tagName: 'li',
         focusable: true,
-        ariaRole: 'option',
-        a11yLabel: null // {string|null}
+        ariaRole: 'option'
 
       }, options );
 
+      // a11y: get innerContent from the item
+      assert && assert( options.innerContent === undefined, 'ComboBoxListItemNode sets innerContent' );
+      options.innerContent = item.a11yLabel;
+
       // Highlight rectangle
       const highlightRectangle = new Rectangle( 0, 0, width, height );
 
@@ -72,12 +75,6 @@ define( require => {
 
       super( options );
 
-      //TODO #314 why do we need to do this? isn't this handled by super(options) ?
-      if ( options.a11yLabel ) {
-        this.a11yLabel = options.a11yLabel;
-        this.innerContent = options.a11yLabel;
-      }
-
       // @public (read-only)
       this.item = item;