add ControlPanelNode/PlayAreaNode to ScreenView.js

[zepumph]

This name is a bloated way of asking, how do we want sims to use the a11y remapping implemented in #365 to look for phetsims? At the beginning of this issue, we were talking about putting "h2 level" nodes directly on the ScreenView.js. This could be pretty nice, especially with the remapping, and usages would look like:

ScreenView.call( this);
this.playArea.accessibleOrder = [ . . . ];
this.controlPanel.accessibleOrder = [ . . . ];

Note, these nodes wouldn't have to be in the sim's scene graph at all. so the scene graph would look like:

rootNode
  simScene
     button1
     accordionBox
          button2
  playArea

Tagging @jonathanolson @jessegreenberg for thoughts.

[zepumph]

Marking as high priority since we should really know this sooner rather than later. My thought is to discuss this in an a11y meeting, then take it to all the devs for opinions.

[jonathanolson]

It feels like a fairly nice pattern to me.

Note, these nodes wouldn't have to be in the sim's scene graph at all. so the scene graph would look like:

The playArea/controlPanel nodes definitely need to be in the scene graph (otherwise they wouldn't be tied to an accessible display).

[jonathanolson]

Also, let me know if there's a specific meeting where I should be available to discuss (I don't have strong preferences, so up to you).

[zepumph]

The playArea/controlPanel nodes definitely need to be in the scene graph (otherwise they wouldn't be tied to an accessible display).

Agreed, sorry for confusion, I meant they could be tacked onto the root node directly, and didn't have to be included in any sim structure that was set up prior to a11y implementation. Basically they can be nodes that are siblings to the node tree structure for the sim view.

[zepumph]

This is beginning to be accomplished in phetsims/joist#509 with ScreenSummaryNode (taking the place of SceneSummaryNode)

[zepumph]

Disussed in a11y dev today. We like that there is an option toggling the creation of the screenSummaryNode, and think that we should keep that option for the other two nodes as well. Eventually we want to have them all default to true, with the option to opt out, but for now, at least the screenSummaryNode needs to opt in, see #393 for changing usages of sceneSummary to this pattern.

[zepumph]

We want the names of the other nodes to be:
playAreaNode
controlAreaNode
screenSummaryNode

[pixelzoom]

I really don't think we should be doing this. This may be a good "conceptual" presentation for the end-user, but it's not generally appropriate for the structure of the scenegraph.

[jessegreenberg]

After #365, this should generally not require changing the structure of the scene graph. Additional nodes on the ScreenView will allow us to specify a separate hierarchy without changing the scene graph, but in a consistent way since we expect this structure to show up frequently.

ScreenView.call( this );

var otherNode = new Node();
var resetAllButton = new ResetAllButton();
this.addChild( resetAllButton )
this.addChild( otherNode );

this.playAreaNode.accessibleOrder = [ otherNode ];
this.controlAreaNode.accessibleOrder = [ resetAllButton ];

[jessegreenberg]

Adding developer meeting label since there will be strong opinions about this and this has long term impacts on a11y instrumentation. Current proposal for devs reading through this issue for the first time:

• ScreenView will have public Nodes:
  • screenSummaryNode
  • playAreaNode
  • controlAreaNode

Nodes can be added directly to these children if that is best. But generally, they will just be in place to control the structure of the PDOM, used like the example in #381 (comment). In that example, the ResetAllButton is added as a child of the ScreenView, but its accessible content will appear under the "Control Area" section.

[jonathanolson]

Sounds good to me. Do we run into any current name conflicts that will need to be handled? And can you overwrite their accessibleOrder, or would it already have things on it?

[zepumph]

Do we run into any current name conflicts that will need to be handled?

I haven't run into any, and didn't see any in a project wide search just now.

And can you overwrite their accessibleOrder, or would it already have things on it?

playAreaNode and controlAreaNode are just empty Nodes, but screenSummaryNode is different, and you cannot set it's accessibleOrder.

We could state an overarching convention that you can't set the play/controlArea's accessibleOrder also, and just have to add node's to them instead, but that may be confusing and dumb.

[jonathanolson]

We could state an overarching convention that you can't set the play/controlArea's accessibleOrder also, and just have to add node's to them instead, but that may be confusing and dumb.

That sounds too restrictive. Especially for older sims, I'd want to set the complete accessibleOrder of the node if possible.

[zepumph]

I'd want to set the complete accessibleOrder of the node if possible.

I agree that is ideal. For screenSummaryNode, there is generic text for an intro that we want all sims to have (for consistency). Perhaps we can make it so that the public facing node is a child to a node with the intro in it, so that you could still set the accessibleOrder on it.

[samreid]

I'm confused about the example in #381 (comment)

Does putting otherNode in playAreaNode.accessibleOrder mean it is no longer in the default order of this?

[jonathanolson]

Does putting otherNode in playAreaNode.accessibleOrder mean it is no longer in the default order of this?

Yes. Generally, if you have a node parent/child and the child is in ANY accessibleOrder, it will not show up in the normal "tree" location.

[mbarlow12]

I generally like the approach proposed in #381 (comment) as well. My one question is how we'll handle some of the more complex screen summaries that we've seen thus far like RIAW? Would it be a new Node (possibly with children) than handles its own content updates that is then added as a child to the sim's screenSummaryNode? e.g.

var mySummaryNode = new FancyScreenSummaryNode( ...importantArgs );
this.screenSummaryNode.addChild( mySummaryNode );

[samreid]

I'm also wondering what happens if you don't add anything to screenSummaryNode, does assistive technology still visit it?

[zepumph]

My changes above are as follows:

• I moved PDOM Nodes to be a child of a single parent, and then added a setChildren override, as recommended by @pixelzoom, that conditionally adds the parent back in if it wasn't added during the .children = [...] call. Note that it shouldn't ever be part of that, as pdomParent is actually a private member, and there is no reason or need to access it from outside of ScreenView.js
• I added the pdom prefix to each PDOM Node, and changed usages
• I removed the prefixed underscore on screen summary related fields.

@jessegreenberg ready for review.

[jessegreenberg]

This and changes after #381 (comment) look good @zepumph, thank you.

I also inspected usages in sims, I like this pattern. And I verified that order/structure in sims is correct with these Nodes. I think this can be closed.

[marlitas]

@pixelzoom recommended that I pass children into the ScreenView super constructor (phetsims/mean-share-and-balance#44), however @samreid and I ran into an assertion error. The assertion error was: hasChild needs to be called with a Node.

The lines in question were:

public override setChildren( children: Node[] ): this {
    Node.prototype.setChildren.call( this, children );
    if ( !this.hasChild( this.pdomParentNode ) ) {
      this.addChild( this.pdomParentNode );
      this.pdomParentNode.moveToBack();
    }
    return this;
  }

[marlitas]

The above commit fixed this behavior. @jessegreenberg, can you review and if all seems good close?

[jessegreenberg]

This makes sense, thanks!