Improve Accessibility.js API

[jessegreenberg]

We discussed potential ways to improve the API of Accessibility.js to make it so that the developer doesn't have to worry about the HTML in many cases. We are outlining the possibilities we discussed here. These would be added to Accessibility.js so it is possible to specify these for a Node. Other options are available in the cases where we need more direct control of the HTML for accessibility. We talked about using a mixin pattern for adding these to specific nodes, but adding directly to Accessibility is much more convenient.

/**
 * Allows one to set accessible name in a consistent way without worrying about the rest of the HTML.
 * Depending on the primary tag name it will create label
 * tags or set inner content or use aria-label to create an accessible name in the parallel DOM. Can be overridden by
 * subtypes of node if default method of setting accessible name is not acceptable.
 * @param {string} name
 * @overridable
 */
setAccessibleName: function( name ) {
  if ( this.tagName === 'input' ) {
    // create a label element and associate with the for attribute
  }
  if ( this.tagName can have inner content ) {
    // set the inner content inside the element
  }
  else {
    // use aria-label
  }
}

/**
* Manages the "description" for a node, specific to what PhET feels is best
* {Node} node
* {Property.<string>|String|Array.<string>} - description
*/
setDescriptionyContent: function( description ){
  if(property){
    addListener(new description){
      this.descriptionContent = newDescription;
    }
  }
  else if( string){
    this.description = description;
  }
  else if( array of string){
    list = makeAList;
    this.descriptionContent = list;
  }
}


/**
* extrapolate a layer to make "help text" consistent across devs and designers. This way if 
* help text changes in the future, we won't have to update `setDescriptionyContent`
* @overridable
*/
setHelpText: function( helpText){
  setDescriptionyContent( helpText);
}
set helpText( helpText){ this.setHelpText(helpText)};


/**
* handle the general heading structure for nodes in the scene graph. This pseudo code relies on 
* a h1 having a child that is an h2. Note that this parent/child hierarchical structure doesn't matter 
* for the PDOM.
* 
* @param {string} headingText
* @param {string} descriptionText
*/
setHeadingStructure: function( headingText, description ){
  
  while(){
    if(this._accessibleParent.labelTagName.startsWith('h')){ // TODO: not always labelTagName?
      parentHLevel = this._accessibleParent.labelTagName.replace( 'h',''); 
      break;
    }
  }

  this.labelTagName = 'h' + parentHLevel+1;
  this.labelContent = headingText;
  this.tagName = 'div';
  setDescriptionyContent( description);
}

// possible names for header option
n.iAmHeader('heading', 'description');
n.a11yHeader('heading', 'description');
n.createHeader('heading', 'description');
n.makeHeader('heading', 'description');
n.dynamicNoninteractiveObjectDammit('heading', 'description');

setAccessibleName and setHelpText are meant to be explicitly overridable so that common code components can set the structure if something more complicated is necessary.

[jonathanolson]

I think it will be valuable to talk a bit in person about this, but in general these feel a bit more like helper functions than abstractions.

I'm not sure if this is the intended purpose, but as coded above there are multiple "order of operation" issues. What happens if I specify an accessibleName before tagName (does this code belong in invalidateAccessibleContent)? setHeadingStructure would usually be run before its parents/accessibleParents would be connected (initializating as h1), etc. null also doesn't look like an option for most parameters.

Accessible names also seem more like a commonly-used part of component a11y options (where each component uses the option to properly name/label things), so I'm curious about the advantages of having a method/value for it.

Header usage generally sounds nice, but (a) there are probably cases where we don't want a description, and (b) if we had two separate options (headingLabel, headingDescription) then it would be easier to pass them into Node options. However since it "overrides" many things (which probably are not safe to override on most common components), maybe it deserves to be its own Node subtype for the purpose (e.g. Header( label, description ) extends Node)? Do we need to use these patterns for all cases (AccordionBox is likely to have a button inside any associated label/header)?

I'd be interested in looking at example usages of the API.

[samreid]

I agree with @jonathanolson's comments and questions, and additionally wanted to mention I don't really understand setHeadingStructure or where/why it would be used. Also, when we talked about making it "so that the developer doesn't have to worry about the HTML in many cases", I pictured this more at a component level, such as e.g., ComboBox would have ComboBox-specific options that ComboBox.js maps into the appropriate Accessibility.js calls. This doesn't preclude us from also working on simplifying/extending Accessibility.js though.

[jessegreenberg]

these feel a bit more like helper functions than abstractions.

Yes, that is the direction we were going here. We considered another style that felt more like abstraction, but it felt too heavy. This approach would let us use these helper functions everywhere without requiring building blocks.

There are multiple "order of operation" issues. What happens if I specify an accessibleName before tagName (does this code belong in invalidateAccessibleContent)?

Changing tagName would update the way the accessible name is set. invalidateAccessibleContent would handle this.

Accessible names also seem more like a commonly-used part of component a11y options...I'm curious about the advantages of having a method/value for it.

The benefit is that devs don't have to fully understand HTML and the ways in which you set an accessible name. We don't need to know that an input tag works and only works with an input. We don't need to understand when it is better to use innerHTML vs aria-lalbel. We have the freedom to set these with the current options, but if we don't want to worry think about it, accessibleName will do the work for us.

there are probably cases where we don't want a description

+1

if we had two separate options (headingLabel, headingDescription) then it would be easier to pass them into Node options. However since it "overrides" many things (which probably are not safe to override on most common components), maybe it deserves to be its own Node subtype for the purpose (e.g. Header( label, description ) extends Node)?

I would be worried about a proliferation of options like this, and if we had explicit options there isn't as much of a benefit over the current API. We felt that creating sub-types felt too heavy and could make instrumentation more difficult.

I pictured this more at a component level, such as e.g., ComboBox would have ComboBox-specific options that ComboBox.js maps into the appropriate Accessibility.js calls.

We will do that too, but that will be a different issue, we wanted to have these helper functions fleshed out first so that they could be used in the instrumentation of common code components like ComboBox.

I don't really understand setHeadingStructure or where/why it would be used

For instance, currently a11y is added to ohms-law/FormulaNode with options

      // a11y
      labelContent: ohmsLawEquationString,
      descriptionContent: ohmsLawDefinitionString,
      tagName: 'div',
      labelTagName: 'h3', // labels should come before other child content

These would be replaced with

this.setHeadingStructure( ohmsLawEquationString, ohmsLawDefinitionString ); // (name subject to change)

[zepumph]

TLDR: On a side note—not sure if we need an example of how nice it would be to abstract , but in phetsims/area-model-common#150 (comment):

This is a another case where a consistent API for "accessible name" (hiding the implementation details) would be very beneficial.

Devs in sims need to currently know exactly how the common code item is setting it's accessible name, so that they use those specific options.

[jessegreenberg]

We talked about this on 5/31/18 dev meeting:

Recommendations were to continue using options pattern wherever possible. For setters accessibleName and helpText, developers thought they were OK, but wanted to see more explicit examples. We will give them a try, test them out, and show usages in a future dev meeting.

[zepumph]

We also talked about turning the heading method into two options, so that you don't need to have a local var reference. Thus the following methods will be part of our "api level 2" and could be overridable in all Node subtypes.
The following would have es5 getters and setters:

• accessibleName
• helpText
• headingLabel
• headingDescription

We will try to implement something before too long and get back to the devs for more feedback.

[zepumph]

I'll try to work up some prototypes and see how things go. Then we can get more feedback and go from there.

[zepumph]

I ran into a problem while implementing accessibleName, this issue is on hold until then.

[zepumph]

I'm going to work on some of the other setters for right now until #811 is sorted out for accessibleName.

[zepumph]

Notes from a conversation with @jonathanolson today. I think we need to think about having a accessibleNameBehavior option instead of overwriting the setter in common code types. Here are @jonathanolson's notes:

// somewhere, called by peer when it needs to know whether it has a label element (and what tag name)
getLabelTagNameForPeers() {
    // using low-level
    if ( this._labelTagName !== null ) {
        return this._labelTagName;
    }
    else if ( this._accessibleName && this._tagName === 'input' ) {
        return 'label';
    }
    else {
        return null;
    }
}

pool: {
    div: [ .... ],

}


// in peer
invalidateLabelElement() {
    if ( we had one before && we want one && they had the same tag name ) {
        // do inline updates
    }
    else if ( we had one before ) {
        labelelement.freeToPool()
    }
    else if ( we want one ) {
        labeleElement = LabelElement.createFromPool( ... );
        // mutate it 
    }
}



// in Peer update:
function( node ) {
    var options = { // iterate through option keys instead of hardcoded. for efficiency, may be a subset of option keys.
      labelTagName: node._labelTagName,
      ...
      ..
      ...
      ...
    };
    options.accessibleNameBehavior( node, options, options.accessibleName );

    // THEN you create/mutate DOM based on these options
} 

// default behavior
accessibleNameBehavior( node, options, accessibleName ) {
    if ( node.labelTagName ) {
        options.labelContent = accessibleName;
    }
    else if ( node.tagName === 'input' ) {
        options.labelTagName = 'label';
        options.labelContent = accessibleName;
    }
    else {

    }
}

// custom behavior
accessibleNameBehavior( options, accessibleName ) {
    options.ariaLabel = accessibleName;
}



var accessibleOptions = {
    accessibleName: 'lksnrlaksnrt',
    tagName: 'input',
    accessibleNameBehavior: Accessibility.USE_LABEL_ELEMENT   Accessibility.USE_ARIA_LABEL
}

var acccessibleOptions = {
    accessibleName: this._accessibleName,
    tagName: this._tagName
}
accessibleOptions = accessibleNameBehavor( accessibleOptions, this._accessibleName );


function getLabelTagName( << of all of the internal Accessibility.js state >> ) {
    
}


// in Accessibility.js
setAccessibleName: {
    if ( ... ) {
        this._accessibleName = accessibleName;

        for ( var peer in this.peers ) {
            peer.totallyRebuildEverything();

            assert && assert( peer....srltnasrltknasr )
        }
    }
}

// in the peer
audit() {
    // browses the full DOM structure of the peer, and makes sure it is identical to what we want given the node's current state/model.
}





=> labelTagName: 'label'


{
    tagName: 'div'
}

=> labelTagName: null



{
}

=> labelTagName: 'foo'



model/peer:


node.getWhatOurActualLabelTagNameShouldBe() {
    
}


node.labelTagName = 'foo';
node.tagName = 'input';
node.accessibleName = 'laksrntalskrnt'; /// overrides the above labelTagName

[zepumph]

#814 has been completed enough to begin work on this again. Tagging @emily-phet so she is aware that I am starting on the API work again after spending the last month on scenery scaffolding and structure.

[zepumph]

I added help text and accessible name options in the above commits. This is a very nice approach, and I appreciate @jonathanolson's guidance in working with this.

Currently there are a few todos in the code that should be decided. Also there is no support for aria-labelledby or aria-describedby within these options. This means that:

            node.accessibleNameBehavior = function( node, options, accessibleName ) {
              options.ariaLabelledbyAssociations = [ {
                otherNode: node,
                otherElementName: AccessiblePeer.LABEL_SIBLING,
                elementName: AccessiblePeer.PRIMARY_SIBLING
              } ];
              options.labelContent = accessibleName;
              return options;
            };

although this should work, we currently don't support overriding ariaLabelledbyAssociations from these options.

I also ran into similar trouble with the aria-label attribute. So far it has worked nicely to just add it to the list of accessibleAttributes, but there is no easy way to, from the view, to set the "aria-label" attribute because the option key is called "ariaLabel." For now I hacked around it just for aria-label, since likely there won't be other attributes that need to be supported for accessibleName/helpText options. Although it would be nice to come up with a general solution before too long (there is a todo in the code for this also).

[samreid]

I removed a debugger statement that looked like it was pushed in this issue.

[zepumph]

This issue is ready to be reviewed by @jonathanolson. We have not yet worked on the "header" api, but I think that a review could happen first to make sure that scenery is working as expected.

EDIT: see #855 for header api implementation and discussion.

[zepumph]

see #832 for parent review issue.

[pixelzoom]

This issue is labeled ready-for-review, but it's unassigned. No comments since Aug 2018, no progress on "parent review issue" since Oct 2018.

@zepumph What should we do here?

[zepumph]

This was reviewed in #832, and the rest of the work is in side issues. Closing