validation and bug fixes, #701

phetsims · Jun 22, 2018 · 84454bb · 84454bb
1 parent 2da4f25
commit 84454bb
Show file tree

Hide file tree

Showing 3 changed files with 48 additions and 25 deletions.
diff --git a/js/accessibility/Accessibility.js b/js/accessibility/Accessibility.js
@@ -1308,6 +1308,7 @@ define( function( require ) {
          *                               see AccessiblePeer for valid element names.
          */
         addAriaLabelledbyAssociation: function( associationObject ) {
+          assert && AccessibilityUtil.validateAssociationObject( associationObject );
 
           this._ariaLabelledbyAssociations.push( associationObject ); // Keep track of this association.
 
@@ -1334,6 +1335,7 @@ define( function( require ) {
          *                               see AccessiblePeer for valid element names.
          */
         addAriaDescribedbyAssociation: function( associationObject ) {
+          assert && AccessibilityUtil.validateAssociationObject( associationObject );
 
           this._ariaDescribedbyAssociations.push( associationObject ); // Keep track of this association.
 
@@ -1398,23 +1400,30 @@ define( function( require ) {
           assert && assert( attribute === 'aria-describedby' || attribute === 'aria-labelledby', 'unsupported attribute name: ' + attribute );
           this.updateAccessiblePeers( function( peer ) {
 
-            // We are just using the first AccessibleInstance for simplicity, but it is OK because the accessible
-            // content for all AccessibleInstances will be the same, so the Accessible Names (in the browser's
-            // accessibility tree) of elements that are referenced by the attribute value id will all have the same content
-            var firstAccessibleInstance = associationObject.otherNode.getAccessibleInstances()[ 0 ];
+            var otherNodeAccessibleInstances = associationObject.otherNode.getAccessibleInstances();
 
-            var otherPeerElement = firstAccessibleInstance.peer.getElementByName( associationObject.otherElementName );
-            var thisPeerElement = peer.getElementByName( associationObject.thisElementName );
+            // if the other node hasn't been added to the scene graph yet, it won't have any accessible instances, so no op.
+            // This will be recalculated when that node is added to the scene graph
+            if ( otherNodeAccessibleInstances.length > 0 ) {
 
-            // only update associations if the requested peer element has been created
-            // NOTE: in the future, we would like to verify that the association exists but can't do that yet because
-            // we have to support cases where we set label association prior to setting the sibling/parent tagName
-            if ( thisPeerElement ) {
-              var previousAttributeValue = thisPeerElement.getAttribute( attribute ) || '';
-              assert && assert( typeof previousAttributeValue === 'string' );
+              // We are just using the first AccessibleInstance for simplicity, but it is OK because the accessible
+              // content for all AccessibleInstances will be the same, so the Accessible Names (in the browser's
+              // accessibility tree) of elements that are referenced by the attribute value id will all have the same content
+              var firstAccessibleInstance = otherNodeAccessibleInstances[ 0 ];
 
-              // add the id from the new association to the value of the HTMLElement's attribute.
-              thisPeerElement.setAttribute( attribute, [ previousAttributeValue.trim(), otherPeerElement.id ].join( ' ' ) );
+              var otherPeerElement = firstAccessibleInstance.peer.getElementByName( associationObject.otherElementName );
+              var thisPeerElement = peer.getElementByName( associationObject.thisElementName );
+
+              // only update associations if the requested peer element has been created
+              // NOTE: in the future, we would like to verify that the association exists but can't do that yet because
+              // we have to support cases where we set label association prior to setting the sibling/parent tagName
+              if ( thisPeerElement && otherPeerElement ) {
+                var previousAttributeValue = thisPeerElement.getAttribute( attribute ) || '';
+                assert && assert( typeof previousAttributeValue === 'string' );
+
+                // add the id from the new association to the value of the HTMLElement's attribute.
+                thisPeerElement.setAttribute( attribute, [ previousAttributeValue.trim(), otherPeerElement.id ].join( ' ' ) );
+              }
             }
           } );
         },

diff --git a/js/accessibility/AccessibilityUtil.js b/js/accessibility/AccessibilityUtil.js
@@ -370,6 +370,25 @@ define( function( require ) {
       return tagNameSupportsContent( tagName );
     },
 
+    /**
+     * Given an associationObject for either aria-labelledby or aria-describedby, make sure it has the right signature.
+     * @param associationObject
+     */
+    validateAssociationObject: function( associationObject ) {
+
+      var expectedKeys = [ 'thisElementName', 'otherNode', 'otherElementName' ];
+
+      var objectKeys = Object.keys( associationObject );
+
+      assert && assert( objectKeys.length === 3, 'wrong number of keys in associationObject, expected:', expectedKeys, ' got:', objectKeys );
+
+
+      for ( var i = 0; i < objectKeys.length; i++ ) {
+        var objectKey = objectKeys[ i ];
+        assert && assert( expectedKeys.indexOf( objectKey ) >= 0, 'unexpected key: ' + objectKey );
+      }
+    },
+
     TAGS: {
       INPUT: INPUT_TAG,
       LABEL: LABEL_TAG,

diff --git a/js/accessibility/AccessiblePeer.js b/js/accessibility/AccessiblePeer.js
@@ -87,7 +87,7 @@ define( function( require ) {
         // if undefined, the insertBefore method will insert the primarySiblingDOMElement as the first child
         var primarySiblingDOMElement = this.primarySibling;
         var firstChild = this.containerParent.children[ 0 ] || null;
-        this.containerParent.insertBefore( primarySiblingDOMElement, firstChild  );
+        this.containerParent.insertBefore( primarySiblingDOMElement, firstChild );
       }
 
       // @private {boolean} - Whether we are currently in a "disposed" (in the pool) state, or are available to be
@@ -143,9 +143,6 @@ define( function( require ) {
       return this.containerParent || this.primarySibling;
     },
 
-    getTopLevelElements: function(){
-    },
-
     /**
      * Get an element on this node, looked up by the association flag passed in.
      * @public (scenery-internal)
@@ -154,22 +151,20 @@ define( function( require ) {
      * @return {HTMLElement}
      */
     getElementByName: function( association ) {
-      var htmlElement = null;
-
       if ( association === AccessiblePeer.PRIMARY_SIBLING ) {
-        htmlElement = this.primarySibling;
+        return this.primarySibling;
       }
       else if ( association === AccessiblePeer.LABEL_SIBLING ) {
-        htmlElement = this.labelSibling;
+        return this.labelSibling;
       }
       else if ( association === AccessiblePeer.DESCRIPTION_SIBLING ) {
-        htmlElement = this.descriptionSibling;
+        return this.descriptionSibling;
       }
       else if ( association === AccessiblePeer.CONTAINER_PARENT ) {
-        htmlElement = this.containerParent;
+        return this.containerParent;
       }
 
-      return htmlElement;
+      assert && assert( false, 'invalid association name: ' + association );
     },
 
     /**