Support an independent tree structure for a11y from the scene graph

[zepumph]

We should work to make sure that all sims create the three main sections (Scene Summary, Play Area, and Control Panel) in the same way. I recently added specific nodes for those in scenery-phet/js/accessibility/nodes but perhaps there is a better way to do it. Then we should change all sims to do this. Ideally this would happen before we teach other devs how to create static descriptions. Assigning to all three of us.

One thought.

What if we added three public nodes to ScreenView.js, so that you always have access to them if you had access to the screen view. like

this.sceneSummary.descriptionContent = "My Summary"
this.playArea.addChild()

Worst case this may be a little strange to then have to pass them far down the view to where they are needed. I'm not sure I even like this idea, just getting the conversation going.

[jessegreenberg]

I Iike this idea.

Worst case this may be a little strange to then have to pass them far down the view to where they are needed.

I am not too worried about this, it is not a problem that we have run into so far. Also, it is an unlikely case because descendants of ScreenView children that are in one of these sections should have ancestors that exist in the same section.

[jessegreenberg]

Removing my assignment, but adding a11y meeting label to talk about more.

[mbarlow12]

I don't know if this made it into yesterday's discussion, but I'm leaning toward @jessegreenberg 's assessment. It also seems like this might ease in explaining descriptions to other devs.

One thought, and this is mostly a style sort of thing, but would it be preferable to add nodes to the different sections through an option? Something like:

var aNode = new Node( [args], { summaryParent: 'playArea' } );
this.addChild( aNode );

I really don't know why this would be useful, but perhaps it'd be useful if a given node's child needed to be added to a different section than its parent.

For simplicity's/consistency's sake, I'm totally ok with keeping the addChild structure as outlined above.

[zepumph]

Discussed in a11y dev meeting today, we like the idea of adding three accessible section nodes the ScreenView.js. These public nodes will act as the main section structure for the pDOM, supplying the consistency desired for the pDOM. As for changes to ScreenView.js, the three nodes would look like:

this.sceneSummaryNode = new Node();
this.playAreaNode = new Node();
this.controlPanelNode = new Node();

We could alternatively call them *SectionNode, instead of just Node, but I don't think we need that verbosity.

As we are converting sims to accessibility, we have been manually creating these three nodes, and reorganizing the ScreenView's children into one of these three nodes anyways, so it would be generally easier to factor them out to the parent type.

Since this would change code in joist that touches many simulations, marking for dev meeting.

[samreid]

In some sims, I've needed to have some play area objects go behind the control panel and some go in front of the control panel.

How would we categorize "floating" buttons? The ResetAllButton is typically not in a Panel instance.

[zepumph]

The "control panel" is loosely defined, and is implemented in what makes the most pedagogical sense. For example, in Ohm's Law, none of the sliders are in the control panel, but rather in the play area, since that is all of the "playing" for the sim. Only the sound and reset buttons are in the control panel.

[jonathanolson]

I'm mainly concerned about @samreid's thoughts. We need to use node ordering to control visual stacking, but the described separation is incompatible. I've also run into the layering case a lot.

I'm curious to learn more about how this would help for a11y, as we probably can't support this as a general pattern. Area Model: Introduction's a11y support didn't require this, and the tab order switches back-and-forth between play area and control panel content anyways.

[samreid]

Would it be possible to add metadata to Node that indicates what "family" or "a11y parent" it should be associated with? Then scenery a11y can take care of the implementation details?

[Denz1994]

I'm curious to learn more about how this would help for a11y.

I'm not too versed in a11y requirements for a sim with 3 sections.

[jessegreenberg]

We talked about this at dev meeting. Ideally, we need to come up with a solution for the layering problem where we structure the scene graph one way for visual layering, but need a separate structure for the accessible HTML. @jessegreenberg @jonathanolson @zepumph and @mbarlow12 (if available, he is out of town) to brainstorm so possibilities for this issue.

[jonathanolson]

Snapshot of my horrible scribble notes (from screen sharing):

I'll be writing up things tonight, including the desired API, how it would remap the node tree to the a11y tree, and a broad overview of how the implementation would be done.

[jonathanolson]

On another note, I should find a better "note sharing" solution that includes a touch device with a stylus.

[jonathanolson]

API proposal:

accessibleOrder's effects would change somewhat to be like getNestedAccessibleOrder but with the order effectively pulling entire subtrees of nodes out of their usual place in the tree, and placing them as if they were direct children of the node the accessibleOrder is defined on. Additionally, a placeholder would potentially represent "the rest" of the children not specified by the order.

For example, if we have the Node tree:

a
  (b) <-- e.g. our play area
    (d)
    (e)
  c
    g <-- e.g. a panel
      (h)
    (f) <-- e.g. our voltmeter
      (i)

where parentheses indicate it the node has accessible content (i.e. will get an AccessibleInstance)

This would generate the following AccessibleInstance tree:

root
  b
    d
    e
  h
  f
    i

however, if an accessibleOrder of [ d, e, f ] was set on b, the subtree of f would be pulled into a different section, resulting in the AccessibleInstance tree:

root
  b
    d
    e
    f
      i
  h

We would presumably have nodes for the accessible sections. Depending on the simulation, we would either place the actual content in those nodes, or place the content elsewhere (and point into the content with accessible orders). It would technically be possible to have a separated accessible node tree (a root node with nodes for each section) that is disconnected from the actual display tree, but specifies everything with accessibleOrder, and I'm not sure if that would be the correct choice. Presumably we'd decide that when we start using this new feature.

The structure of the AccessibleInstance tree is essentially described by the recursive (kind-of pseudocode) function:

function toInstances( node ) {
  // prune subtrees that are used in an order
  var effectiveChildren = node.children.filter( isAccessibleOrdered );

  if ( node.accessibleOrder ) {
    // fill the placeholder with the remaining children (notes below about placeholder)
    effectiveChildren = node.accessibleOrder.slice().splice(
      node.accessibleOrder.indexOf( PLACEHOLDER ), 1, effectiveChildren );
  }

  var instances = _.flatten( effectiveChildren.map( toInstances ) );
  if ( this.accessibleContent ) {
    return [ new AccessibleInstance( { children: instances } ) ];
  }
  else {
    return instances;
  }
}

(this would be way too inefficient, and is also the "immutable" form, where we need efficient mutable operations).

There are some open questions:

• For the placeholder, would we use null, or some specified constant? We could also totally avoid the use of a placeholder with:
  • If we specify a beforeAccessibleOrder and afterAccessibleOrder. I don't like this.
  • If we enforce a "total order", such that no accessible content is "left out" of the order. This may be useful to make sure that we have properly ordered (and thought about) everything, but I'm not sure.
• Do we allow including totally unconnected subtrees in the accessible order? (i.e. things that won't even show up in the visual instance tree, and aren't connected somehow to our display).
• Do we want to consider either allowing "some" duplicates in orders, and how to we handle it if someone tries to set a duplicate?
  • If you set e.g. a.accessibleOrder = [ d, e ], then b.accessibleOrder = [ e, f ], do we error out, or "move" e to the new one (so that a's order is now just [ d ] )?
  • If a node p specifies q in its order, but p is NOT in the displayed tree/DAG, do we exclude it with the pseudocode isAccessibleOrdered function? Then it wouldn't show up at all in the a11y tree. Presumably dispose on a node will clear its order, "releasing" those, but how much would we need to clear orders when things like that get removed?

We'll potentially need to be careful to dispose/clear orders in nodes that we toss permanently (depending on our choices) so we either don't leak memory OR have things not show up at all. How we handle "outside the display" content (both as effective ancestors or descendants) will have a lot of impact on how things get implemented. The placeholder choice won't change the implementation much (it's mostly aesthetic and client-code maintenance related).

For example, does isAccessibleOrdered( child ) need to check if the "parent" (who has the "child" in the order) is actually displayed in the scene graph? It's faster to not check, and the consequence is "if you specify b in a.accessibleOrder and a isn't displayed, then b's accessible content just won't show up". This COULD be desired sometimes if you have multiple displays, so it's a trade-off.

Additionally, most of our current AccessibleInstance infrastructure ignores node trees "outside" of our display tree, so if we want to include something not displayed in an order, it will require reworking of that logic.

Notably, our current system starts with the following 4 events that can cause a change to the AccessibleInstance tree:

1. addChild
2. removeChild
3. accessibleOrder changed
4. presence of accessibleContent changed (i.e. whether the node has accessible instances or not), or potentially just a content change.

The logic starts with onAccessibleAddChild/onAccessibleRemoveChild/setAccessibleOrder/setAccessibleContent, which look up the Trail and identify displays used (may need to change if we allow out-of-display content in orders).

This forwards to methods on the Display (addAccessibleTrail/removeAccessibleTrail/changedAccessibleContent/changedAccessibleOrder), which look up the "base" accessible instance (the one responsible for that trail, or if that node has no accessibleContent, then the instance for the ancestor that does). Add/remove/content changes fire off addSubtree/removeSubtree calls on the instance, whereas order changes call markAsUnsorted.

No matter what choices we make, we'll need to handle trails being pruned/unpruned (say they were added/removed from an order, or if we ignore non-display orders, whether the order they are in is now displayed / not displayed). This would basically look like addSubtree/removeSubtree, but with some qualifiers. Order changes would presumably call the necessary adds/removes wherever, and anything with an "add" gets marked with an "order dirty" flag (similar to how it is now). Then synchronously, the order would need to be updated once all the adds/removes fire.

Overall I think the current system is set up quite well to handle the main logic needed for our changes. We'll also need to check whether there is anything depending on accessibleInstance.trail being related to the trail of its parent/child (since now they will dramatically vary sometimes when subtrees are moved). Furthermore, we'll need to track visibility (or anything that affects subtrees) slightly differently (e.g. the voltmeter parent being invisible should "hide" it, even though the invisible parent is not in the order).

As part of development, I highlight recommend making an audit() function that basically uses the immutable toInstances method above (turned into actual code), and checks against the current accessible order. Scenery unit tests should be able to apply those 4 operations (add/remove/order/content-change) with a bunch of nodes randomly (like fuzzing) as long as a proper DAG structure and order rules are followed, and an audit() after every operation should show that the accessible instance tree matches the specification. Additionally, something like assertSlow (if enabled with ?eall) would presumably run the audit for displays.

I think discussing the open questions (and their trade-offs) needs to be done before I can provide more detail on any implementation of this, and a screen-sharing call (or in-person meeting) is probably needed to properly communicate. So is there a time where this can be discussed? Or should I fully think through the consequences of the trade-offs and provide more details (which will probably come with recommendations)?

[jonathanolson]

Also @ariel-phet, how involved should I be in the implementation of this? It mirrors a lot of the trade-offs and implementation patterns with the scenery "display" code, so it (a) would benefit from me being somewhat involved, but (b) having someone else be the full lead on implementation would be a valuable experience for understanding the constraints / patterns used elsewhere in scenery. Also priority/timeline would be good to know.

[zepumph]

Today, while @jessegreenberg and I were giving an a11y architecture briefing to the team, @pixelzoom brought this up as a large hindrance to converting sims. We should try to get this going before the code sprint. Renaming the issue.

[ariel-phet]

@jonathanolson as discussed yesterday, it would be great to have you involved. I believe that plan was a bit of intellectual collaboration with @jessegreenberg and @zepumph today, and then you helping to work on the issue once Area Model 1.0 is released and iOS maintenance release has occurred.

[jonathanolson]

Do we allow including totally unconnected subtrees in the accessible order?

No, only connected content for now (simplifies implementation, can change later)

If you set e.g. a.accessibleOrder = [ d, e ], then b.accessibleOrder = [ e, f ], do we error out

Yes, we error out.

If a node p specifies q in its order, but p is NOT in the displayed tree/DAG, do we exclude it with the pseudocode isAccessibleOrdered function? Then it wouldn't show up at all in the a11y tree.

It would not show up in its "orginial" place in the tree if it is specific in any other order.

Presumably dispose on a node will clear its order, "releasing" those, but how much would we need to clear orders when things like that get removed?

Dispose gets rid of the accessibleOrder's effect.

[jonathanolson]

I believe this is basically ready enough to work for prototyping for the sprint (if needed for sims other than area-model). Using the a11y-tree branch (for scenery and area-model-common), I'm able to successfully use the following pattern:

// "Play Area" (a11y)
this.addChild( new Node( {
  tagName: 'div',
  labelTagName: 'h2',
  labelContent: playAreaString,
  accessibleOrder: [
    this.areaDisplayNode,
    this.factorsBox,
    this.areaBox,
    this.productsSelectionPanel,
    this.calculationSelectionPanel,
    this.partitionSelectionPanel,
    this.calculationNode
  ].filter( function( node ) { return node !== undefined; } ) // this.partitionSelectionPanel may not exist
} ) );

// "Control Panel" (a11y)
this.addChild( new Node( {
  tagName: 'div',
  labelTagName: 'h2',
  labelContent: controlPanelString,
  accessibleOrder: [
    gridCheckbox,
    tileCheckbox,
    countingCheckbox,
    this.sceneSelectionNode,
    this.resetAllButton
  ]
} ) );

and behaves normally.

I have a decent list of questions and discussion items for @jessegreenberg and @zepumph (can wait until after the sprint), and this work is NOT yet ready to be merged into master. It will take a combination to discussion, some refinement, adding unit tests / more inline tests / fuzz tests, QA, etc.

[jonathanolson]

Also, it's not quite passing all unit tests yet (4 fail). At least one test will need to have its behavior changed to match our "new" understanding of how visibility works, but the other three seems like a buggy situation where I don't understand why things aren't focusing.

[zepumph]

We are meeting to discuss a few loose ends of this tomorrow. @jonathanolson, let me know if there is anything you want from me to do save that.

[jonathanolson]

NOTE: When this comes up for review, going to the branch on GitHub (https://github.com/phetsims/scenery/tree/a11y-tree) and clicking the "Compare" button (under the "Clone or download" button), then going to "Files changed" and then looking at the diffs should be invaluable. I've made significant changes along the way, so viewing commit-by-commit would be super-inefficient.

[jonathanolson]

Or there may be a good way of doing that in Intellij.

[jonathanolson]

I've merged the a11y-tree branch into master (it's passing aqua fuzzing tests, aqua a11y tests, unit tests, a11y-specific fuzzing test, sim spot tests, builds, etc.)

I'll be creating more specific "additional changes" issues in the scenery repo (tagging here) for more work.

I'm leaving this open for the "original" discussion about how to structure the scene graph to do this best. #365 (comment) seems to be working nicely for area-model, but I don't have a problem with having everything as children under those if the structure supports it.

[zepumph]

I'm leaving this open for the "original" discussion about how to structure the scene graph to do this best. #365 (comment) seems to be working nicely for area-model, but I don't have a problem with having everything as children under those if the structure supports it.

I think that we should move this to another issue as well, this issue has done it's job well, but seems old and tired. See #381 for continued discussion. Thanks @jonathanolson for the good work, anyone please feel free to reopen if there is more to discuss.