PhetMenu to run on popupable

[zepumph]

From https://github.com/phetsims/phet-io/issues/1913#issuecomment-1452546501, I think this may be relatively straight forward, and much better than just duplicating even more for our PhET-iO purposes (in this case isShowingProperty). I'll take a look!

[zepumph]

Alright. The conversion went better than expected. I spoke with @jessegreenberg about all the extra focus-management code that we were able to completely remove because Popupable has the right order of operations when showing and hiding. @AgustinVallejo, since you are on my team this week. It would be awesome if you could give this a review. I know you may not be too familiar with this code, but I'm here if you have any questions. It was a pretty straight forward refactor where we removed much of PhetMenu's code because it was duplicated with Popupable, but I'd love another set of eyes.

Here are a few questions to guide you in your review:

• Are there any improvements that you can see with the option pattern used?
• Are there cases where the PhetMenu could still be open (not hidden) when it shouldn't be?
• Please run a sim with accessibility and test keyboard navigation and how focus moves from the PhetButton to the PhetMenu and back (including when a dialog is opened/closes).
• Whatever else you are thinking!

Let me know if you have any questions or want to pair. I'm going to mark this refactor as blocking publication until someone can take a look. Thanks!

[samreid]

Testing with keyboard navigation in MSS seems good.

I noticed this code in Popuplable, is half of it redundant?

    private _focusOnHideNode: Node | null;

    // The Node to return focus to after the Popupable has been hidden. A reference to this Node is saved when
    // the Popupable is shown. By default, focus is returned to Node that has focus when the Popupable is open
    // but can be overridden with focusOnHideNode.
    private _nodeToFocusOnHide: Node | null;

The fact that Popupable is a mixin instead of a subtype made it difficult to navigate during the code review. I believe we should prefer subtypes to mixins, but that is probably outside the scope of this review/iteration.

Everything else seems good to me. The options pattern seems ok. I was not able to trigger any error cases during usage with keyboard or mouse or emulated touch. Back to @zepumph

[zepumph]

I noticed this code in Popuplable, is half of it redundant?

focusOnHideNode is the option to provide a hard coded value to always use, but nodeToFocusOnHide is the spot to store the value just for the specific show/hide call. If it was one variable, then you would lose the ability to know "should I use FocusManager.pdomFocusedNode or my hard coded value next time." Though you could manage it with a single Node variable and one boolean. I think it the way it is right now. You? (https://github.com/phetsims/sun/blob/8177da9ca3bf86f0813f4c47683569b3d2659d54/js/Popupable.ts#L130)

The fact that Popupable is a mixin instead of a subtype made it difficult to navigate during the code review. I believe we should prefer subtypes to mixins, but that is probably outside the scope of this review/iteration.

For this issue, it is so incredibly nice that Popupable is a mixin instead of a super type. If it wasn't, this issue would have been prohibitively challenging.

Dialog extends Panel, so if it had been factored out into a type instead of a mixin, then it would depend on Panel, and PhetMenu (extending Node) would not have been able to adopt the code without real refactoring. Instead, it took 30 minutes to add this mixin to a second spot.

[samreid]

Great, it helps to know the difference between those attributes. Everything seems good, closing.