Dialog should have an option to put the close button last in the pdomOrder

[zepumph]

While discussing phetsims/joist#750 in design meeting, we thought that some dialogs would seem better to have the close button last. It seems like a good use case to support, and we likely will for in the vegas reward dialog (over in phetsims/vegas#96).

Tagging @jessegreenberg because he is a boss, but I'll take the lead first.

[zepumph]

@jessegreenberg and I like closeButtonLastInPDOMOrder defaulting to false.

I also noticed that the current PDOM order can leak a null into it if the title is not provided. Let's make sure we get rid of that.

[zepumph]

@jessegreenberg please review this. I added assertions because I had to do some hackary to come up with a good default focusOnShowNode when the close button is last.

[jessegreenberg]

 // pdom - focus the CloseButton when the Dialog is open, unless another focusOnShowNode is provided
    options.focusOnShowNode = options.focusOnShowNode || pdomOrder[ 0 ];

    assert && assert( options.focusOnShowNode instanceof Node, 'should be non-null and defined' );
    assert && assert( options.focusOnShowNode.focusable,
      'focusOnShowNode. This may mean that the first item in pdomOrder is not focusable. In this case please adjust ' +
      'closeButtonLastInPDOM or provide a custom focusOnShowNode.' );

This seems a bit too strict to me. Are we sure that we never want the close button last if the Dialog has no interactive content? What about if the content Node does have focusable content but it is not focusable itself? The close button still seems like a reasonable default.

Otherwise, the option looks good!

[zepumph]

How would you recommend this then? I don't know how to choose a focusOnShowNode if the closeButton is last and there is no interactive content. What get's focused? Should I just go through the entire PDOMOrder and pick the first focusable item?

[jessegreenberg]

No, I would recommend just focusing the closeButton in this case even though it is last. I think that is a reasonable default. Before reading the added assertions I happened to test closeButtonLastInPDOM in the UpdateDialog and this assertion was hit. That didn't seem beneficial and possibly too strict which is why I mentioned it.

[zepumph]

Hows that?

Tested with:

Index: js/AboutDialog.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/AboutDialog.js b/js/AboutDialog.js
--- a/js/AboutDialog.js	(revision cd9dc495092421afbbf827ff0c9dbf5888fa09ec)
+++ b/js/AboutDialog.js	(date 1634251723326)
@@ -45,6 +45,7 @@
       xSpacing: 26,
       topMargin: 26,
       bottomMargin: 26,
+      closeButtonLastInPDOM: true,
       leftMargin: 26,
       rightMargin: 26,
       phetioReadOnly: true, // the AboutDialog should not be settable

[jessegreenberg]

I think thats great, thanks!