diff --git a/js/accessibility/Accessibility.js b/js/accessibility/Accessibility.js index 55de0df7f..44a25a190 100644 --- a/js/accessibility/Accessibility.js +++ b/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(); diff --git a/js/accessibility/AccessibleInstance.js b/js/accessibility/AccessibleInstance.js index 4b706fb17..89d39c180 100644 --- a/js/accessibility/AccessibleInstance.js +++ b/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' ); diff --git a/js/accessibility/AccessiblePeer.js b/js/accessibility/AccessiblePeer.js index 98eaac28d..a4b272423 100644 --- a/js/accessibility/AccessiblePeer.js +++ b/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. }, /**