Discuss aria-labelledby api in mixin

[zepumph]

@jessegreenberg and I just spoke, and we found that the setters for aria-labelledby and aria-describedby are confusing to outside devs. Let's spend some a11y dev meeting time to discuss. A reason for the confusion is because you set node you want to describe your node, but you also need to set the individual tag that you want. Since a single node can have a "label" "description" "parent" and itself (4 html elements total) we need a way to link the two nodes, and then link the specific tab. Current implementation does this in two methods.

Possible thoughts to improve:

• turn this into one, well named (that's optimistic as naming is hard), function that takes in the node, and the tag that it wants the be describedby.

• maybe we just need to rename the current existing function to make it a bit more clear. Again I have no good ideas, but brainstorming will be nice.

• What kind of option support do we want with these. If we go with the 1 function 2 args approach, we won't be able to make it an option. Also how do you specify yourself as an argument default? That seems weird:
  options = _.extend( { ariaLabelledByNode: this }, options);

I

[jessegreenberg]

@zepumph was wondering about options/setters in the Accessibility trait that would set aria-labelledby and aria-describedby relations between a Node and its own labels/descriptions. This would be more direct than the current API, but would not cover cases where the association goes beyond a single node and its labels.

[zepumph]

We discussed this even further today in the a11y dev meeting. At first I thought that it may be nice to be able to use an option (rather than a setter) to assign the aria-labelledby attribute to point to that node tagName's own label (label meaning the h3 or somthing set by accessibleLabel option).

We haven't made any decisions yet, but it would be nice if we could handle all aria-labelledby situations with an option if we wanted. We experimented with the following options (each has a parallel option for aria-describedby)

new Node( {
  
  // would by default assign the aria-labelledby attribute to the label of this node
  // this is no good because it isn't an option that could be used to assign labelledby another node
  useAriaLabelledBy: true 

  // this signifies that I should be labeled by my own label.
  ariaLabelledBy: AccessiblePeer.LABEL

 });

I'm sure there were other ones too, but I don't remember all iterations. We found that it is important that you can not only specify a node, but what tag of the node you want to be able to be labelled by.

The goal is to be explicit here, without having an overly complicated api. We don't want clever.

As a wise man (@jessegreenberg) once said, and a less wise one (@zepumph) paraphrased:
"being clever is never smart"

[zepumph]

We will keep discussing.

[zepumph]

@jessegreenberg and I went at this today, discussing the best way to proceed.

One of our biggest complaints about the way it is now, is that in order to use aria-labelledby with dom elements other than the main Node's, you have to have two lines like so, with operations on both Nodes involved (yuck).

      // this node is labelledby its own label
      node1.setAriaLabelledByNode( node2 );
      node2.setAriaLabelContent( AccessiblePeer.LABEL );

Translation: We want node1 to be labelled by node two's label dom element (not node 2 itself).

This usage is annoying because we don't like that it requires a call on both nodes, so on idea on how to proceed is to change the second line to node1.ariaLabelledByThisParticularDOMElementOfNode2 = Accessibility.LABEL.

Another complaint is its general complexity. There are so many options with who can be aria labelledby whom. (see ariaLabelledContent vs ariaLabelContent).

---

We discovered that we will not be able to reference the DOM element directly because the PDOM creates/destroys dom elements too often (see invalidateAccessibleContent).

---

We discussed just setting aria labelled by on nodes, and not providing the support to specific related peer content like a Node's LABEL, DESCRIPTION, or PARENT_CONTAINER. This will require reworking of some classes in some cases. For example if we want to keep the same structure as a Node with a label that is prepended, we would have to wrap all of the content of this node in a containing node, and then make a label node ourselves that would be a sibling to this. You could also bump the logic up to the parent. Basically leveraging the scene graph to support this rather than the accessibility api.

---

We don't know what to do yet, but the last discussion point seems like it could be nice.

[zepumph]

We really like the last suggestion. . .

[jessegreenberg]

Another option - We have explicit functions for all combinations of aria-labelledby and aria-describedby on all sibling elements for a Node?

nodeA.setAriaLabelledBy( nodeB )
nodeA.setParentContainerAriaLabelledByOtherNodeLabel( nodeB )
nodeA.setLabelAriaDescribedByOtherNodeDescription( nodeB )
... and so on

... an option, but I dislike it so much that I don't even want to keep typing examples.

[jessegreenberg]

We continued to discuss this today at 4/4/18 meeting.

We took a look at some current usages of setAriaLabelledByNode with setAriaLabelContent to see if it could be replaced with the recommendation of handling the association with scene graph structure. Initial investigation made this seem very complicated and not acceptable for general instrumentation.

We considered changing the current API to use the new naming scheme chosen for AccessiblePeer. While this made things easier to follow, it is still very complex and hard to understand each function.

At the end, we talked about having one function to specify the label node, label sibiling, and labelled sibling. It might look something like this:

/**
 * @param {Node} otherNode - the Node labelling this node
 * @param {string} labelSibling - sibling of otherNode that will act as the label for this node
 * @param {string} labelledSibling - sibling on this node that will be labelled by the aria-labelledby attribute
 */
this.setAriaLabelledBy: function( otherNode, labelSibling, labelledSibling ){...}

for readability, we also considered making this a utility function rather than an instance method, like so:

/**
 * @param {Node} labelledNode - the node that will somehow be labelledby another node's AccessiblePeer DOM element
 * @param {string} labelledSibling - the sibling element of the labelledNode that will receive the aria-labelledby attribute
 * @param {Node} otherNode - another node whose accessible content will somehow label the labelledNode
 * @param {string} labelSibling - the sibling of the otherNode whose content will be the value of the aria-labelledBy attribute
 */
AccessibilityUtil.setAriaLabelledBy: function( labelledNode, labelledSibling, otherNode, labelSibling ){...}

These options are superior to others that have been thought of so far. It has been requested that aria-labelledby can support more than one id at a time (so more than one Node, and/or more than one sibling element). It is not clear how the above strategy would handle this yet.

[jessegreenberg]

Adding high priority, I am seeing aria-labelledby is showing up more frequently in design docs.

[zepumph]

@mbarlow12 and I talked about this today. We springboarded off of the above static function for setting aria-labelledby.

in #795 we may very well go with an accessibleName setter, that knows how to set an accessible name for a node. This could potentially set html content on different html elements depending on what the node's HTML representation is. Thus, to have a similarly abstracted api for aria-labelledby we talked about having a method like whatSiblingProducesAccessibleName() that can, from a node, get what html element it is so that we can get the html ID from that specific element.

otherNode.accessibleName = 'hello I am name'; // need a way to get this.

/**
 * @param {Node} labelledNode - the node that will somehow be labelledby another node's AccessiblePeer DOM element
 * @param {string} labelledSibling - the sibling element of the labelledNode that will receive the aria-labelledby attribute
 * @param {Node} otherNode - another node whose accessible content will somehow label the labelledNode
 */
AccessibilityUtil.setAriaLabelledBy: function( labelledNode, labelledSibling,  otherNode ){

	// this way we don't need to be passed 'LABEL' or 'PRIMARY' because we can get it from the parallel method to accessibleName setter.
	otherNode.whatSiblingProducesAccessibleName(); // ---> 'PRIMARY'

	labelledNode._ariaLebelledByNode = node2;
	otherNode._ariaLabelsNode = labelledNode;
	labelledNode._setAriaLabelledContent = labelledSibling; // "use labelledNode's primary"
	otherNode._setAriaLabelContent = otherSiblingToBeLabel; //"use otherNode's label"


	otherNode.onchange = funciton(){
		// tell node1 to update the id accordingly.
	}

}

This is pretty tentative, but seems like it could work. The main bummer would be that places where accessibleName is overridden, and we want to use the aria-labelledby api, we would need to override this separate method to get the id of the proper html element.

A few notes from the discussion:
NOTE: accessibleName changed emitter emitter that the arialabelledby method
NOTE: accessibleName shouldn't be allowed to be set if you are setting arialabelledby on something
NOTE: we just need the id connection, maybe this is too complicated?
NOTE: this doesn't support dag.

[jessegreenberg]

We talked about this again, going over the discussion from last week and some alternatives.

One concern that we had with using whatSiblingProducesAccessibleName() is that it doesn't easily provide the flexibility to set which sibling on otherNode to set aria-labelledby.

We wondered if data attributes could be used for this. We weren't sure exactly how this would work, but it would allow us to have a way to create an identifier that can be used to associate two different elements in the DOM. It would add complexity because we would still need a way to set the data attribute on multiple siblings.

We reviewed the current API and wanted to reconsider it. After much discussion in this issue, we have yet to come up with alternatives that seem much better. Perhaps with much better setter names it would easier to understand and use.

[jessegreenberg]

We also thought that it would be helpful to discuss this issue with @jonathanolson and possibly others. It would be most helpful to have some dedicated call time to go over this problem in detail and what we have already considered. @ariel-phet can we have permission to get 30-60 minutes from @jonathanolson for this?

[mbarlow12]

I don't believe this was mentioned yet, but as we further develop this solution, we need to be sure to remember that the API needs the ability to set multiple other nodes in the aria-labelledby attribute. This would produce a space-separated list of ids the associated AccessiblePeer.

[zepumph]

In dev meeting today we worked hard on this issue, and got it to a really good place!! I made a few more adjustments this evening and got things working pretty well.

There are only a few tests but basically here is how this works, you now add association objects to a node, and for each you are saying "add the aria-labelledby attribute to one of my peer elements, then get the id from another Node's specified peer Element (can be any of them), and add that id to the value of the aria-labelledby attribute.

This way we support any number of ids as the value of This Node's AccessiblePeer's HTMLElement's aria-labelledby attribute. Terminology is fun.

From here:

• There aren't a ton of unit tests, so it would be good to add more
• There are a few inefficiencies:
  • updateAriaLabelledbyAssociations has to totally clear the previous aria-labelled by values, before resetting things, the third unit test exemplifies the need for that.
  • Because of the above bullet, we couldn't use @mbarlow12's nice improvement of passing in an optional node, and only updating the aria-labelled by attribute for that Node. Since the aria-labelledby value on an HTMLElement is just a space separated list, there was no obvious way to clear an old id, and update it to a new id.

I hope to talk about this/review this more next week, but @jessegreenberg or @mbarlow12 get to this before then, that is great too!

NOTE: I merged this into master.

[jessegreenberg]

This is looking really good @zepumph, the only change I made so far is to use removeAttribute instead of setting attribute to '' in `updateAriaLabelledbyAssociations.

Removing assignment because I probably won't be able to review in detail until next meeting.

[jessegreenberg]

We discussed this today, it is working really well. We added a few more unit tests for aria-labelledby to make sure associations are correct between more than just the primary siblings, and after the scene graph changes. I think we are ready to proceed with the same pattern for aria-describedby. We discussed that addAriaLabelledbyAssociationImplementation could be refactored and used for both labelledby and describedby.

This is as far as we got tonight. @zepumph mentioned that he could continue with this since he worked on aria-labelledby. Thanks, and let us know if we can assist.

[zepumph]

In the above commits I:

1. Added a unit test that makes sure all is well when creating an association between two elements of a single node.
2. Added aria-describedby api
3. Consolidated and refactored the new api so that labelledby and describedby use the same code to create and update their associations.
4. Adapted the unit tests in the same way as 3 because they both support the same feature set.
5. Replaced all usages of the old api in the project with the new api.
6. Removed the old api.

This is all on master.

The ways I tested:

• I tested by manually checking the PDOM in about 1/3rd of the places where I updated usages, to make sure that nothing changed.
• I added errors to the top of all old api functions to make sure I got all usages.
• I aqua fuzz tested all modes of our sim.
• I made sure scenery unit tests were passing.

@jessegreenberg this is a lot of code to review, let me know if you want to sit down together and talk some of it out. Thanks!

[terracoda]

@zepumph, I did a quick skim of this issue, and while I do not fully understand everything in this issue, I am wondering if are you still using "setAriaLabelledBy"? And was wondering if you considered something like:

• hasAriaLabelledBy? True/False
• hasMultipleAssociations or hasMultipleAssociatedElements? True/False
• getIDRefsOfAssociatedElements and put them in a comma separated list on the element with aria-labelledby attribute?

Perhaps that is what the API does?

[zepumph]

The api leans in the same direction you are thinking, we are no longer using setAriaLabelledBy, and instead have created a way that you can specify any number of associations between a Node and another Node you want to be aria-labelledby (or described by). I can explain more if you desire, but I think the most important thing for you to know is that the api will now support any number of items "aria-labelling" an item in the sim, and for the developers it just got much easier to add an one of these labelledby/describedby associations (yay!)

[jessegreenberg]

This issue is super old now and I missed the boat on catching anything in a review. But I still went through changes and things look great. I saw a couple of TODOs in the commits referenced here that have since been resolved.

The API has been serving us well for many years. Closing.