From ac38baa985c2834ed025f9e91c7278a1ca151094 Mon Sep 17 00:00:00 2001 From: zepumph Date: Mon, 23 Jul 2018 16:30:57 -0800 Subject: [PATCH] removed commented code, refactored, added tests for ariaRole, AccessiblePeer should use getters for node properties, see #814 --- js/accessibility/Accessibility.js | 90 ++++++++++---------------- js/accessibility/AccessibilityTests.js | 48 ++++++++++++++ js/accessibility/AccessiblePeer.js | 89 ++++++------------------- 3 files changed, 100 insertions(+), 127 deletions(-) diff --git a/js/accessibility/Accessibility.js b/js/accessibility/Accessibility.js index 38107aa1a..571dcad49 100644 --- a/js/accessibility/Accessibility.js +++ b/js/accessibility/Accessibility.js @@ -720,13 +720,6 @@ define( function( require ) { // TODO: this could be setting a11y content twice this.onAccessibleContentChange(); - // if ( this._accessibleInstances.length > 0 ) { - // for ( var i = 0; i < this._accessibleInstances.length; i++ ) { - // var peer = this._accessibleInstances[ i ].peer; - // peer.onTagNameChange(); - // } - // } - } }, set tagName( tagName ) { this.setTagName( tagName ); }, @@ -766,11 +759,6 @@ define( function( require ) { this._labelTagName = tagName; this.onAccessibleContentChange(); - - // for ( var i = 0; i < this._accessibleInstances.length; i++ ) { - // var peer = this._accessibleInstances[ i ].peer; - // peer.onLabelTagNameChange(); - // } } }, set labelTagName( tagName ) { this.setLabelTagName( tagName ); }, @@ -812,11 +800,6 @@ define( function( require ) { this._descriptionTagName = tagName; this.onAccessibleContentChange(); - - // for ( var i = 0; i < this._accessibleInstances.length; i++ ) { - // var peer = this._accessibleInstances[ i ].peer; - // peer.onDescriptionTagNameChange(); - // } } }, set descriptionTagName( tagName ) { this.setDescriptionTagName( tagName ); }, @@ -879,10 +862,6 @@ define( function( require ) { // TODO: can we do this without recomputing everything? this.onAccessibleContentChange(); - // for ( var i = 0; i < this._accessibleInstances.length; i++ ) { - // var peer = this._accessibleInstances[ i ].peer; - // peer.onAppendLabelChange(); - // } }, set appendLabel( appendLabel ) { this.setAppendLabel( appendLabel ); }, @@ -893,7 +872,7 @@ define( function( require ) { getAppendLabel: function() { return this._appendLabel; }, - get appendLabel() { this.getAppendLabel(); }, + get appendLabel() { return this.getAppendLabel(); }, /** * By default the label will be prepended before the primary sibling in the PDOM. This @@ -914,10 +893,6 @@ define( function( require ) { // TODO: can we do this without recomputing everything? this.onAccessibleContentChange(); - // for ( var i = 0; i < this._accessibleInstances.length; i++ ) { - // var peer = this._accessibleInstances[ i ].peer; - // peer.onAppendDescriptionChange(); - // } }, set appendDescription( appendDescription ) { this.setAppendDescription( appendDescription ); }, @@ -928,7 +903,7 @@ define( function( require ) { getAppendDescription: function() { return this._appendDescription; }, - get appendDescription() { this.getAppendDescription(); }, + get appendDescription() { return this.getAppendDescription(); }, /** @@ -960,10 +935,6 @@ define( function( require ) { this._containerTagName = tagName; this.onAccessibleContentChange(); - // for ( var i = 0; i < this._accessibleInstances.length; i++ ) { - // var peer = this._accessibleInstances[ i ].peer; - // peer.onContainerTagNameChange(); - // } }, set containerTagName( tagName ) { this.setContainerTagName( tagName ); }, @@ -1098,8 +1069,18 @@ define( function( require ) { */ setAriaRole: function( ariaRole ) { assert && assert( ariaRole === null || typeof ariaRole === 'string' ); - this._ariaRole = ariaRole; - this.setAccessibleAttribute( 'role', ariaRole ); + + if ( this._ariaRole !== ariaRole ) { + + this._ariaRole = ariaRole; + + if ( ariaRole !== null ) { + this.setAccessibleAttribute( 'role', ariaRole ); + } + else { + this.removeAccessibleAttribute( 'role' ); + } + } }, set ariaRole( ariaRole ) { this.setAriaRole( ariaRole ); }, @@ -1122,17 +1103,29 @@ define( function( require ) { * * @param {string|null} ariaRole - role for the element, see * https://www.w3.org/TR/html-aria/#allowed-aria-roles-states-and-properties - * for a lsit of roles, states, and properties. + * for a list of roles, states, and properties. */ setContainerAriaRole: function( ariaRole ) { assert && assert( ariaRole === null || typeof ariaRole === 'string' ); - this._containerAriaRole = ariaRole; - this.onAccessibleContentChange(); - // for ( var i = 0; i < this._accessibleInstances.length; i++ ) { - // var peer = this._accessibleInstances[ i ].peer; - // peer.onContainerAriaRoleChange(); - // } + if ( this._containerAriaRole !== ariaRole ) { + + this._containerAriaRole = ariaRole; + + // clear out the attribute + if ( this._containerAriaRole === null ) { + this.removeAccessibleAttribute( 'role', { + elementName: AccessiblePeer.CONTAINER_PARENT + } ); + } + + // add the attribute + else { + this.setAccessibleAttribute( 'role', ariaRole, { + elementName: AccessiblePeer.CONTAINER_PARENT + } ); + } + } }, set containerAriaRole( ariaRole ) { this.setContainerAriaRole( ariaRole ); }, @@ -1165,11 +1158,8 @@ define( function( require ) { if ( this._accessibleNamespace !== accessibleNamespace ) { this._accessibleNamespace = accessibleNamespace; + // If the namespace changes, tear down the view and redraw the whole thing, there is no easy mutable solution here. this.onAccessibleContentChange(); - // for ( var i = 0; i < this._accessibleInstances.length; i++ ) { - // var peer = this._accessibleInstances[ i ].peer; - // peer.onAccessibleNamespaceChange(); - // } } return this; @@ -1235,12 +1225,6 @@ define( function( require ) { isFocused = true; } - this.onAccessibleContentChange(); - // for ( var i = 0; i < this._accessibleInstances.length; i++ ) { - // var peer = this._accessibleInstances[ i ].peer; - // peer.onFocusHighlightChange(); - // } - // if the focus highlight is layerable in the scene graph, update visibility so that it is only // visible when associated node has focus if ( this._focusHighlightLayerable ) { @@ -1283,12 +1267,6 @@ define( function( require ) { assert && assert( this._focusHighlight instanceof phet.scenery.Node ); this._focusHighlight.visible = this.focused; } - - this.onAccessibleContentChange(); - // for ( var i = 0; i < this._accessibleInstances.length; i++ ) { - // var peer = this._accessibleInstances[ i ].peer; - // peer.onFocusHighlightLayerableChange(); - // } }, set focusHighlightLayerable( focusHighlightLayerable ) { this.setFocusHighlightLayerable( focusHighlightLayerable ); }, diff --git a/js/accessibility/AccessibilityTests.js b/js/accessibility/AccessibilityTests.js index c11a1f509..159afcb1c 100644 --- a/js/accessibility/AccessibilityTests.js +++ b/js/accessibility/AccessibilityTests.js @@ -1423,6 +1423,54 @@ define( function( require ) { assert.ok( containerElement.childNodes[ 2 ].tagName.toUpperCase() === 'LI', 'primary sibling last' ); } ); + QUnit.test( 'containerAriaRole option', function( assert ) { + + // test the behavior of focusable function + var rootNode = new Node( { tagName: 'div' } ); + var display = new Display( rootNode ); + document.body.appendChild( display.domElement ); + + var a = new Node( { + tagName: 'div', + containerTagName: 'div', + containerAriaRole: 'application' + } ); + + rootNode.addChild( a ); + assert.ok( a.containerAriaRole === 'application', 'role attribute should be on node property' ); + var aElement = getPrimarySiblingElementByNode( a ); + assert.ok( aElement.parentElement.getAttribute( 'role' ) === 'application', 'role attribute should be on parent element' ); + + a.containerAriaRole = null; + assert.ok( a.containerAriaRole === null, 'role attribute should be cleared on node' ); + aElement = getPrimarySiblingElementByNode( a ); + assert.ok( aElement.parentElement.getAttribute( 'role' ) === null, 'role attribute should be cleared on parent element' ); + } ); + + QUnit.test( 'ariaRole option', function( assert ) { + + // test the behavior of focusable function + var rootNode = new Node( { tagName: 'div' } ); + var display = new Display( rootNode ); + document.body.appendChild( display.domElement ); + + var a = new Node( { + tagName: 'div', + ariaRole: 'application' + } ); + + rootNode.addChild( a ); + assert.ok( a.ariaRole === 'application', 'role attribute should be on node property' ); + var aElement = getPrimarySiblingElementByNode( a ); + assert.ok( aElement.getAttribute( 'role' ) === 'application', 'role attribute should be on element' ); + + a.ariaRole = null; + assert.ok( a.ariaRole === null, 'role attribute should be cleared on node' ); + aElement = getPrimarySiblingElementByNode( a ); + assert.ok( aElement.getAttribute( 'role' ) === null, 'role attribute should be cleared on element' ); + } ); + + // Higher level setter/getter options QUnit.test( 'accessibleName option', function( assert ) { diff --git a/js/accessibility/AccessiblePeer.js b/js/accessibility/AccessiblePeer.js index aded1c402..634ac3bb4 100644 --- a/js/accessibility/AccessiblePeer.js +++ b/js/accessibility/AccessiblePeer.js @@ -124,16 +124,6 @@ define( function( require ) { this.descriptionSibling = null; this.containerParent = null; - - // higher level api first, because it will effect the lower level setters. - // if ( node.accessibleName ) { - // node.setAccessibleNameImplementation( node.accessibleName ); // set it again to support any option order - // } - // - // if ( node.helpText ) { - // node.setHelpTextImplementation( node.helpText ); // set it again to support any option order - // } - var uniqueId = this.accessibleInstance.trail.getUniqueId(); // create the base DOM element representing this accessible instance @@ -187,18 +177,18 @@ define( function( require ) { // set the accessible label now that the element has been recreated again, but not if the tagName // has been cleared out - if ( this.node._labelContent && this.node._labelTagName !== null ) { - this.setLabelSiblingContent( this.node._labelContent ); + if ( this.node.labelContent && this.node.labelTagName !== null ) { + this.setLabelSiblingContent( this.node.labelContent ); } // restore the innerContent - if ( this.node._innerContent && this.node._tagName !== null ) { - this.setPrimarySiblingContent( this.node._innerContent ); + if ( this.node.innerContent && this.node.tagName !== null ) { + this.setPrimarySiblingContent( this.node.innerContent ); } // set the accessible description, but not if the tagName has been cleared out. - if ( this.node._descriptionContent && this.node._descriptionTagName !== null ) { - this.setDescriptionSiblingContent( this.node._descriptionContent ); + if ( this.node.descriptionContent && this.node.descriptionTagName !== null ) { + this.setDescriptionSiblingContent( this.node.descriptionContent ); } // set the accessible attributes, restoring from a defensive copy @@ -213,8 +203,8 @@ define( function( require ) { } // if element is an input element, set input type - if ( this.node._tagName.toUpperCase() === INPUT_TAG && this.node._inputType ) { - this.setAttributeToElement( 'type', this.node._inputType ); + if ( this.node.tagName.toUpperCase() === INPUT_TAG && this.node.inputType ) { + this.setAttributeToElement( 'type', this.node.inputType ); } // recompute and assign the association attributes that link two elements (like aria-labelledby) @@ -223,18 +213,20 @@ define( function( require ) { // add all listeners to the dom element - for ( i = 0; i < this.node._accessibleInputListeners.length; i++ ) { - this.addDOMEventListeners( this.node._accessibleInputListeners[ i ] ); + for ( i = 0; i < this.node.accessibleInputListeners.length; i++ ) { + this.addDOMEventListeners( this.node.accessibleInputListeners[ i ] ); } // update all attributes for the peer, should cover aria-label, role, input value and others this.onAttributeChange(); // Default the focus highlight in this special case to be invisible until selected. - if ( this.node._focusHighlightLayerable ) { - this.node._focusHighlight.visible = false; - } + if ( this.node.focusHighlightLayerable ) { + // if focus highlight is layerable, it must be a node in the scene graph + assert && assert( this.node.focusHighlight instanceof phet.scenery.Node ); + this.node.focusHighlight.visible = false; + } // TODO: this is hacky, because updateOtherNodes. . . could try to access this peer from its accessibleInstance. this.accessibleInstance.peer = this; @@ -266,36 +258,11 @@ define( function( require ) { // Wean out any null siblings this.topLevelElements = truthySiblings; } - // insert the label and description elements in the correct location if they exist - this.labelSibling && this.arrangeContentElement( this.labelSibling, this.node._appendLabel ); - this.descriptionSibling && this.arrangeContentElement( this.descriptionSibling, this.node._appendDescription ); - - }, - - onTagNameChange: function() { - - this.setHasAccessibleContent(); - }, - - onLabelTagNameChange: function() { - - this.setHasAccessibleContent(); - }, - onDescriptionTagNameChange: function() { - - this.setHasAccessibleContent(); - }, - onAppendLabelChange: function() { - this.setHasAccessibleContent(); - }, - onAppendDescriptionChange: function() { - - this.setHasAccessibleContent(); - }, - onContainerTagNameChange: function() { + // insert the label and description elements in the correct location if they exist + this.labelSibling && this.arrangeContentElement( this.labelSibling, this.node.appendLabel ); + this.descriptionSibling && this.arrangeContentElement( this.descriptionSibling, this.node.appendDescription ); - this.setHasAccessibleContent(); }, /** @@ -346,25 +313,6 @@ define( function( require ) { this.setAttributeToElement( dataObject.attribute, dataObject.value, dataObject.options ); } }, - onContainerAriaRoleChange: function() { - - this.setHasAccessibleContent(); - }, - onAccessibleNamespaceChange: function() { - - this.setHasAccessibleContent(); - }, - onFocusHighlightChange: function() { - - this.setHasAccessibleContent(); - }, - onFocusHighlightLayerableChange: function() { - - this.setHasAccessibleContent(); - }, - - setHasAccessibleContent: function() { - }, /** * Called when our parallel DOM element gets focused. @@ -669,7 +617,6 @@ define( function( require ) { this.primarySibling.blur(); }, - /** * Responsible for setting the content for the label sibling * @public (scenery-internal)