review of review, with JO and JG, #832

js/accessibility/Accessibility.js

@@ -168,7 +168,7 @@ define( function( require ) {
   // see setAccessibleHeadingBehavior for more details
   var DEFAULT_ACCESSIBLE_HEADING_BEHAVIOR = function( node, options, heading ) {
 
-    options.labelTagName = 'h' + node.headingLevel;
+    options.labelTagName = 'h' + node.headingLevel; // TODO: make sure heading level change fires a full peer rebuild, see https://github.com/phetsims/scenery/issues/867
     options.labelContent = heading;
     return options;
   };
@@ -1494,11 +1494,7 @@ define( function( require ) {
               focusHighlight.visible = this.focused;
             }
 
-            // REVIEW: This value is used by AccessiblePeer's update (creation), but changing this doesn't seem to
-            // REVIEW: update the peers. Should it call onAccessibleContentChange(), or do a more fine-grained change?
-            // ZEPUMPH: I deleted the usage in update because it is covered here in node. I don't think this needs
-            // ZEPUMPH: to effect the peer at all.
-            // ZEPUMPH: NOTE: this should apply to setFocusHighlightLayerable too.
+            // REVIEW: Should it call onAccessibleContentChange()
           }
         },
         set focusHighlight( focusHighlight ) { this.setFocusHighlight( focusHighlight ); },
@@ -1651,6 +1647,8 @@ define( function( require ) {
         addAriaLabelledbyAssociation: function( associationObject ) {
           assert && AccessibilityUtil.validateAssociationObject( associationObject );
 
+          // TODO: assert if this associationObject is already in the association objects list! https://github.com/phetsims/scenery/issues/832
+
           this._ariaLabelledbyAssociations.push( associationObject ); // Keep track of this association.
 
           // Flag that this node is is being labelled by the other node, so that if the other node changes it can tell
@@ -1795,6 +1793,7 @@ define( function( require ) {
         addAriaDescribedbyAssociation: function( associationObject ) {
           assert && AccessibilityUtil.validateAssociationObject( associationObject );
 
+          // TODO: assert if this associationObject is already in the association objects list! https://github.com/phetsims/scenery/issues/832
           this._ariaDescribedbyAssociations.push( associationObject ); // Keep track of this association.
 
           // Flag that this node is is being described by the other node, so that if the other node changes it can tell
@@ -1853,6 +1852,7 @@ define( function( require ) {
 
           // if any other nodes are aria-describedby this Node, update those associations too. Since this node's
           // accessible content needs to be recreated, they need to update their aria-describedby associations accordingly.
+          // TODO: only use unique elements of the array (_.unique)
           for ( var i = 0; i < this._nodesThatAreAriaDescribedbyThisNode.length; i++ ) {
             var otherNode = this._nodesThatAreAriaDescribedbyThisNode[ i ];
             otherNode.updateAriaDescribedbyAssociationsInPeers();

js/accessibility/AccessibleInstance.js

@@ -166,10 +166,7 @@ define( function( require ) {
       else {
         this.peer = AccessiblePeer.createFromPool( this );
 
-        // ZEPUMPH:
-        // TODO: This function constructs the peer and must be called with createFromPool, are we
-        // sure that we want the "constructor" for the peer (this function) to be outside of the constructor for the type?
-        // https://github.com/phetsims/scenery/issues/832
+        // The peer is not fully constructed until this update function is called, see https://github.com/phetsims/scenery/issues/832
         this.peer.update();
 
         assert && assert( this.peer.primarySibling, 'accessible peer must have a primarySibling upon completion of construction' );

js/accessibility/AccessiblePeer.js

@@ -45,7 +45,9 @@ define( function( require ) {
 
     /**
      * Initializes the object (either from a freshly-created state, or from a "disposed" state brought back from a
-     * pool)
+     * pool).
+     *
+     * NOTE: the AccessiblePeer is not fully constructed until calling AccessiblePeer.update() after creating from pool.
      * @private
      *
      * @param {AccessibleInstance} accessibleInstance
@@ -102,28 +104,15 @@ define( function( require ) {
         // @private {HTMLElement} - The main element associated with this peer. If focusable, this is the element that gets
         // the focus. It also will contain any children.
         this._primarySibling = options.primarySibling;
-        return this;
-      }
-
-      //REVIEW: Why doesn't this need to be done for "root" peers where the primarySibling is passed in? Seems like
-      //REVIEW: it would still be needed.
-      //ZEPUMPH: Since the root doesn't have a node, it couldn't have a container or sibling elements, so this all isn't
-      //ZEPUMPH: needed I think.
-      // for each accessible peer, clear the container parent if it exists since we will be reinserting labels and
-      // the dom element in createPeer
-      while ( this._containerParent && this._containerParent.hasChildNodes() ) {
-        this._containerParent.removeChild( this._containerParent.lastChild );
       }
 
       return this;
     },
 
     /**
-     * Update the content of the peer
+     * Update the content of the peer. This must be called after the AccessibePeer is constructed from pool.
      * @private
      *
-     * REVIEW: This appears to be only called from initializeAccessiblePeer, and never from anywhere else. Is that
-     * REVIEW: correct, and if so could they be combined?
      */
     update: function() {
       var i;
@@ -181,6 +170,7 @@ define( function( require ) {
       //REVIEW: able to modify whether something is focusable?
       //ZEPUMPH: I think we need to have a larger discussion about what behavior functions' role should be, I totally
       //ZEPUMPH: understand your thought here.
+      // TODO: why not just options.focusable?
       this._primarySibling = AccessibilityUtil.createElement( options.tagName, this.node.focusable, {
         namespace: options.accessibleNamespace
       } );
@@ -234,7 +224,7 @@ define( function( require ) {
       // if element is an input element, set input type
       if ( options.tagName.toUpperCase() === INPUT_TAG && options.inputType ) {
         // REVIEW: This looks like something that should be a behavior?
-        //ZEPUMPH: I'm not sure I understand
+        //ZEPUMPH: TODO: Let's talk about this more as part of https://github.com/phetsims/scenery/issues/867
         this.setAttributeToElement( 'type', options.inputType );
       }
 
@@ -396,12 +386,6 @@ define( function( require ) {
         }
         this.setAttributeToElement( attribute, value, dataObject.options );
       }
-
-      // REVIEW: How are "removed" attributes handled here? Do we never need to worry about it?
-      // ZEPUMPH: right now this is only called from `update()` (basically from AccessiblePeer constructor). When
-      // ZEPUMPH: removing attributes, we use removeAttributeFromElement, see setInputType or removeAccessibleAttribute.
-      // ZEPUMPH: This is a little confusing (since it is called onAttribute"Change"), but it is less expensive to run
-      // ZEPUMPH: this loop for every individual change. Right now it is messy though.
     },
 
     /**