Skip to content

Commit

Permalink
removed commented code, refactored, added tests for ariaRole, Accessi…
Browse files Browse the repository at this point in the history 
…blePeer should use getters for node properties, see #814
  • Loading branch information
@zepumph
zepumph committed Jul 24, 2018
1 parent cdd68f4 commit ac38baa
Show file tree
Hide file tree
Showing 3 changed files with 100 additions and 127 deletions.
90 changes: 34 additions & 56 deletions js/accessibility/Accessibility.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 ); },
Expand Down Expand Up @@ -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 ); },
Expand Down Expand Up @@ -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 ); },
Expand Down Expand Up @@ -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 ); },

Expand All @@ -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
Expand All @@ -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 ); },

Expand All @@ -928,7 +903,7 @@ define( function( require ) {
getAppendDescription: function() {
return this._appendDescription;
},
get appendDescription() { this.getAppendDescription(); },
get appendDescription() { return this.getAppendDescription(); },


/**
Expand Down Expand Up @@ -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 ); },

Expand Down Expand Up @@ -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 ); },

Expand All @@ -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 ); },

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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 ) {
Expand Down Expand Up @@ -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 ); },

Expand Down
48 changes: 48 additions & 0 deletions js/accessibility/AccessibilityTests.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 ) {

Expand Down
89 changes: 18 additions & 71 deletions js/accessibility/AccessiblePeer.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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)
Expand All @@ -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;
Expand Down Expand Up @@ -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();
},

/**
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -669,7 +617,6 @@ define( function( require ) {
this.primarySibling.blur();
},


/**
* Responsible for setting the content for the label sibling
* @public (scenery-internal)
Expand Down

0 comments on commit ac38baa

Please sign in to comment.