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

Issues related to pdomPlayAreaNode and pdomControlAreaNode #721

Closed
pixelzoom opened this issue Jun 10, 2021 · 5 comments
Closed

Issues related to pdomPlayAreaNode and pdomControlAreaNode #721

pixelzoom opened this issue Jun 10, 2021 · 5 comments
Assignees

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented Jun 10, 2021

Discovered while working on phetsims/fourier-making-waves#76 (play area and control area for Fourier).

In ScreenView.js,

    this.pdomParentNode = new Node( {
      children: options.includePDOMNodes ? [
        this.pdomTitleNode,
        this.pdomScreenSummaryNode,
        this.pdomPlayAreaNode,
        this.pdomControlAreaNode
      ] : [ this.pdomTitleNode ]
    } );

So the pdomPlayAreaNode is behind the pdomControlAreaNode. This seems backwards. In all of the sims that I've implemented, draggable objects (which I suspect would be in the play area) need to stay in front of controls. A couple of general reasons for this: First, it's a much better UX if you moveToFront when you start dragging an object. Second, you don't want to release a draggable object behind a control panel, where it would then be "lost" (unreachable).

@jessegreenberg thoughts?

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Jun 10, 2021

I should also point out that (with the current implementation) if any Nodes are added to the ScreenView directly, they will be (by default) in front of pdomPlayAreaNode and pdomControlAreaNode. So even if we change the order that pdomPlayAreaNode and pdomControlAreaNode are added, it's going to be problematic to (for example) move a draggable object in the play area "to the front".

While I understand the concepts of "play area" and "control area" and their benefit, I think that the current implementation is going to cause many UX and implementation problems. Have you considered decoupling these concepts from the scene graph order?

@pixelzoom pixelzoom changed the title Should "play area" be in front of "control area"? Issues related to pdomPlayAreaNode and pdomControlAreaNode Jun 10, 2021
@pixelzoom
Copy link
Contributor Author

pixelzoom commented Jun 10, 2021

6/10/2021 design meeting @jessegreenberg @arouinfar @kathy-phet @KatieWoe @pixelzoom

The current was specified based on pdomOrder needs. But this.pdomPlayAreaNode should be in front of this.pdomControlAreaNode, and pDomOrder should be set explicitly for this.pdomParentNode.

Over in phetsims/vegas#93, @jessegreenberg said that in situations like this, rather than adding children to pdomPlayAreaNode and pdomControlAreaNode, we could set pdomOrder for those Nodes.

@jessegreenberg suggested that, going forward, setting pdomOrder for pdomPlayAreaNode and pdomControlAreaNode is a better approach than addChild for those Nodes. @kathy-phet asked @jessegreenberg to make a PSA about this new approach for "play area" and "control area".

@jessegreenberg
Copy link
Contributor

I just updated documentation in the interactive-description-technical-guide.md to reflect this change, Ill add to dev meeting agenda to describe this and the benefits to the team.

@jessegreenberg
Copy link
Contributor

This was discussed during developer meeting today. We reviewed that pdomOrder should generally be set explicitly unless it is for a component whose children order naturally aligns with traversal order (such as layout managers).

I made #722 to swap the order of pdomControlAreaNode and pdomControlAreaNode as children but keeping their same order in the PDOM.

Reviewing comments here I think that is all that is left for this general issue, closing.

@jessegreenberg
Copy link
Contributor

@zepumph and I reviewed this during a catch-up meeting today and discussed what was decided in this issue. We found a few places where the documentation could be improved related to this in interactive-description-technical-guide.md in the section related to pdomOrder. @zepumph is going to work on bringing the whole section up to speed with this issue. Thanks!

@jessegreenberg jessegreenberg reopened this Jul 6, 2021
zepumph added a commit to phetsims/phet-info that referenced this issue Jul 6, 2021
@zepumph zepumph closed this as completed Jul 6, 2021
samreid pushed a commit to phetsims/phet-info that referenced this issue Apr 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants