Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pdomOrder is specified incorrectly #47

Closed
pixelzoom opened this issue Jun 28, 2022 · 5 comments
Closed

pdomOrder is specified incorrectly #47

pixelzoom opened this issue Jun 28, 2022 · 5 comments

Comments

@pixelzoom
Copy link
Contributor

For code review #41 ...

In https://github.com/phetsims/phet-info/blob/master/doc/alternative-input-quickstart-guide.md#traversal-order, 2 approaches are described for handling pdomOrder (aka traversal order).

IntroScreenView.ts currently appears to be leaning towards Approach 1, because it sets this.pdomPlayAreaNode.pdomOrder and this.pdomControlAreaNode.pdomOrder. But it does not follow the other requirement of Approach 1, which is to add children to this.pdomPlayAreaNode and this.pdomControlAreaNode, not directly to the ScreenView.

Unless "play area" and "control area" have explicity been part of the sim design, it's recommended to follow Approach 2.

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Jun 28, 2022

I also see that IntroScreenView extends MeanShareAndBalanceScreenView. But MeanShareAndBalanceScreenView currently ignores pdomOrder. Classes are typically responsible for specifying the order of Nodes that they create, so this is a little odd. On the other hand, it can be more straightforward for a subclass to specify the complete pdomOrder for its Nodes and its superclass' Nodes. But you have to be careful, it's a bit of a maintenance issue -- if you add a Node to the superclass, you'll need to change all of its subclasses.

So... You should probably investigate:

(1) setting pdomOrder in MeanShareAndBalanceScreenView for the Nodes that it creates
(2) adding to pdomOrder in IntroScreenView for the Nodes that it creates

@marlitas
Copy link
Contributor

This issue is on hold until designer feedback for #76 is provided

@marlitas
Copy link
Contributor

I believe I corrected pdomOrder specification in the commits in: #76

@pixelzoom can you review and provide feedback?

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Sep 15, 2022

Summary of Zoom discussion with @marlitas ...

The end result looks correct, but a few things in the implementation...

Delete this line in MeanShareAndBalanceScreenView.ts, it looks vestigial:

this.screenViewRootNode.pdomOrder = [ ...this.children, this.questionBar ];

Make this change in IntroScreenView.ts:

-    this.screenViewRootNode.pdomOrder = [ waterCupLayerNode, controlPanel, predictMeanSlider ];
+    this.screenViewRootNode.pdomOrder = [ ...waterCup3DNodes, ...pipeNodes, controlPanel, predictMeanSlider, this.resetAllButton ];

... so that you're not:

  • including waterCup2DNodes
  • not relying on the child order of waterCupLayerNode
  • and not implicitly adding this.resetAllButton

Feel free to close after changes.

@marlitas
Copy link
Contributor

Fixed in above commit. Closing. Thanks @pixelzoom!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants