phetsims / sun

User-interface components for PhET simulations, built on top of Scenery.
MIT License
4 stars 12 forks source link

Popupable shouldn't require that a Node is returned to focus when it is hidden/shown #723

Closed jessegreenberg closed 3 years ago

jessegreenberg commented 3 years ago

In https://github.com/phetsims/sun/issues/719 we added assertions that say when a Popupable closes, focus must go somewhere and the target must be focusable.

      // return focus to the Node that had focus when the Popupable was opened (or the focusOnHideNode if provided)
      if ( this.nodeToFocusOnHide ) {
        assert && assert( this.nodeToFocusOnHide.focusable, 'nodeToFocusOnHide is not focusable' );
        this.nodeToFocusOnHide.focus();
      }

But it is crashing in a case demonstrated in https://github.com/phetsims/fourier-making-waves/issues/207. In this case, the ScreenView loads with focus on its h1 Node. On the first mouse/touch press on an AmplitudeNumberDisplay the Dialog is shown and Popupable stores the h1 Node to focus later. The h1 Node is immediately removed from the focus order. We might have set the AmplitudeNumberDisplay as the focusOnHideNode but they are currently explicitly removed form the traversal order. We also shouldn't have to, the default focus behavior for Popupable would work well in this case except for this first mouse press into the sim.

I am going to remove the assertion for now.

jessegreenberg commented 3 years ago

I removed the assertion and made it so we only focus these Nodes when they are focusable. I want to make sure @zepumph is OK with this change since we discussed adding these assertions together initially in #719. @zepumph assigning you to review or lets take a look tomorrow during dev meeting.

zepumph commented 3 years ago

That looks great to me thanks. Ready to close, but I want to make sure you note this behavior while reviewing https://github.com/phetsims/sun/issues/724 to make sure that won't cause the same trouble.

jessegreenberg commented 3 years ago

OK thanks! Will keep an eye out in #724 but I think this can be closed.