Should TransitionNode set focus when it completes a transition?

[pixelzoom]

For https://github.com/phetsims/fourier-making-waves/issues/53, and as noted in https://github.com/phetsims/vegas/issues/90#issuecomment-850636738:

So in the meantime, I’ll modify TransitionNode to move focus to the first element in pDomOrder for the Node that it has transitioned to. For Fourier, that’s the Back button for a level, and the “Level 1” button for the level-selection UI.

If a Node has no pDomOrder, TransitionNode will do nothing.

@jessegreenberg FYI.

[pixelzoom]

This got complicated really quickly.

In Fourier, TransitionNode is used like this in WaveGameScreenView.js:

    // Handles the animated 'slide' transition between level-selection and levels
    this.transitionNode = new TransitionNode( this.visibleBoundsProperty, {
      content: ( model.levelProperty.value === null ) ? levelSelectionNode : levelsParent,
      cachedNodes: [ levelsParent ]
    } );

levelsParent has 5 children, 1 for each game level (WaveGameLevelNode). One of them is visible, based on which level-selection button was pressed. So TransitionNode knows only about levelsParent, so it can reach down and set focus for the Back button of the WaveGameLevelNode that is currently visible.

If TransitionNode was provided with 2 Nodes that had pDomOrder, I'm also not clear on how to set focus to the first element. Is it something like this?

node.pDomOrder[ 0 ].peer.focus();

[pixelzoom]

TransitionNode typically does removeChild for the Node that it's transitioning from, and addChild for the Node that it's transitioning to. But I discovered that TransitionNode has this option, which uses visibility instead of add/removeChild:

      // {Array.<Node>} - Any node specified in this array will be added as a permanent child internally, so that
      // transitions to/from it don't incur higher performance penalties. It will instead just be invisible when not
      // involved in a transition. Performance issues were initially noted in
      // https://github.com/phetsims/equality-explorer/issues/75. Additional notes in
      // https://github.com/phetsims/twixt/issues/17.
      cachedNodes: []

So in the above commit, I tried adding both of my Nodes to cachedNodes:

    // Handles the animated 'slide' transition between level-selection and levels
    this.transitionNode = new TransitionNode( this.visibleBoundsProperty, {
      content: ( model.levelProperty.value === null ) ? levelSelectionNode : levelsParent,
      cachedNodes: [ levelSelectionNode, levelsParent ]
    } );

When transitioning from levelSelectionNode to levelsParent, I can now press tab and the Back button gets focus (instead of the navbar). This is a minor improvement.

But when transitioning from levelsParent to levelSelectionNode, pressing tab still sets focus to the navabar.

[pixelzoom]

I tried a few other things here (in Fourier code) and got nowhere. I'm stumped, and will need to pair with @jessegreenberg on this.

[jessegreenberg]

One thing we could do is use PDOMUtils.getNextFocusable() to move focus to the first focusable element at the end of the transition.

Use PDOMUtils.getNextFocusable

```patch Index: js/TransitionNode.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- js/TransitionNode.js (revision 6cbaa586ddcf96ab04203e8937487c35a7f917ae) +++ js/TransitionNode.js (date 1622599673456) @@ -9,6 +9,8 @@ import Shape from '../../kite/js/Shape.js'; import merge from '../../phet-core/js/merge.js'; +import PDOMUtils from '../../scenery/js/accessibility/pdom/PDOMUtils.js'; +import Display from '../../scenery/js/display/Display.js'; import Node from '../../scenery/js/nodes/Node.js'; import Transition from './Transition.js'; import twixt from './twixt.js'; @@ -301,6 +303,11 @@ self.fromContent = self.toContent; self.toContent = null; + + // pdom - At the end of the transition, move focus to the first focusable element in the new content. No effect + // if there is no focusable element in the page. + Display.focus = null; + PDOMUtils.getNextFocusable().focus(); } ); transition.start(); ```

[pixelzoom]

I tested the approach proposed in https://github.com/phetsims/twixt/issues/30#issuecomment-852662036, and it works nicely. Two concerns:

(1) It's looks a little klunky to me. Maybe not an issue, but is setting Display.focus directly a good idea? Seems like it would be better to have a PDOMUtils method for that. And I don't understand how PDOMUtils.getNextFocusable().focus() works in this context. Maybe an expanded comment would help me here, but it's all a little unclear.

(2) It might be OK to use this as the default, but it doesn't support this requirement from https://github.com/phetsims/vegas/issues/90#issuecomment-854034816:

• After transitioning from the level scene to the level selection scene, focus should be on the button for the level that we just left.

A couple of possibilities that I can think of for meeting this requirement:

(A) Add additional param/config that allows the client to specify (dynamically) what should get focus when the transition ends. This looks complicated.

(B) Have the client listen to transition.endedEmitter, and handle the focus change. This looks relatively easy, but has a couple of problems. TransitionNode would set default focus first, then the client would change it, which isn't so bad. But it creates an ordering dependency, since TransitionNode is adding its own listener to transition.endedEmitter.

[pixelzoom]

(B) Have the client listen to transition.endedEmitter ...

There's currently no way to do this. The TransitionNode API does not make transition public. The client provides @param {Transition} transition if they call startTransition directly. Otherwise the convenience methods (slideLeftTo, etc.) create it, and it remains private.

[pixelzoom]

There's currently no way to do this.

I take that back. startTransition and the convenience methods return the Transition.

 * @returns {Transition} - Available to add end listeners, etc. (chained)

[pixelzoom]

After muddling around in this issue for awhile... I'm not sure what the general focus behavior should be for TransitionNode, or whether it should even be responsible for focus. And I think it would be preferrable for the a11y team to discuss and address.

In the meantime, I'm going to investigate a sim-specific solution in Fourier, to meet the behavior requirements described in https://github.com/phetsims/vegas/issues/90#issuecomment-854034816. That will likely involve listening to transition.endedEmitter, and setting specific focus. What we learn there may inform other game screens, and/or what is ultimately done for TransitionNode.

[pixelzoom]

@jessegreenberg and I talked about this today on Zoom, in the context of Fourier. That discussion reinforced by feeling that this doesn't belong in TransitionNode, and that I should address this in Fourier (which I've done in the commit above).

While games typically use TransitionNode to totally change what's visiible for a Screen, that's not always the case. For example, there may be one section of the Screen that is animating/changing, and there may be no focus involved. And in that case, the approach that @jessegreenberg proposed in https://github.com/phetsims/twixt/issues/30#issuecomment-852662036 is definitely not appropriate, not even as a default behavior. In general, I think we'd need something much more Node-specific, not PDOMUtils.getNextFocusable.

So for now, I think it would be best to table the idea of adding this functionality to TransitionNode. I'll leave it up to @jessegreenberg to decide whether to keep this issue open, or close it.