buggy placement of Dialog close button

[pixelzoom]

Example in plinko-probability:

new Dialog( new MultiLineText( 'Out\nof\nBalls!' ), { font: new PhetFont( 25 ) } ), {
  modal: true,
  focusable: true,
  xMargin: 40,
  yMargin: 20
} ).show();

Results in:

Looks like the problem is here in Dialog:

126 closeButton.right = dialogContent.right + options.xMargin - options.closeButtonMargin;
127 closeButton.top = dialogContent.top - options.xMargin + options.closeButtonMargin;

I don't believe that the placement of the close button should have anything to do with the dialogContent. The close button should always be inside the dialog. And it should simply be placed in the upper-right corner of the dialog, with the margin specified by options.closeButtonMargin.

Assigning to @jonathanolson since he added this bit of code in @13b93ab2ed26820eb95553be3e9bdd0508536f05.

[pixelzoom]

Noted while working on phetsims/plinko-probability#72.

This problem seems to occur when you try to use the xMargin and yMargin options. I've worked around it in plinko-probability by avoiding those options, and wrapping my dialog in a sun.Panel with the desired margins.

[jonathanolson]

I don't believe that the placement of the close button should have anything to do with the dialogContent

There's the complication that for dialog content that spans the entire width, we don't want the close button overlapping the content (thus either content would need to be pushed down, or the dialog would need to be wider).

I'd prefer to defer until Dialog design is completed (reassign if something is desired/needed before then).

[pixelzoom]

I'd prefer to defer until Dialog design is completed

Agreed.

[pixelzoom]

Moved this to #359. Closing.

[pixelzoom]

A close button on dialogs is apparently required for a11y standards, see #413. Raising the priority of this issue.

[pixelzoom]

This got moved to #359, but has not been addressed. Reopening because the "closed" state gives the illusion that this was fixed, and it is not.

[jonathanolson]

This actually looks buggy because xMargin is used for the vertical computation:

closeButton.top = dialogContent.top - options.xMargin + options.closeButtonMargin;

Since by default xMargin and yMargin are the same, I believe this is how it snuck through.

I'll look into testing the fix to change it to yMargin (looks good in my use-case fix in pendulum-lab so far).

[jonathanolson]

It looks like we've been avoiding this, so my new usage that looked broken (where I noticed) is the only case where this even comes up. Will push fix.

[jonathanolson]

@jessegreenberg, you had recent work in the Dialog, do you mind verifying that this should be fine?

[jessegreenberg]

The change looks good, this does look like a pretty clear mistake. I tested a bunch of Dialogs that are using yMargin including AboutDialog and UpdateDialog. This also fixes the case in plinko-probability that opened this issue, when I remove the workaround added in phetsims/plinko-probability@0ceac3b, the Dialog looks good:

The close button should always be inside the dialog.

This is always be true except when closeButtonMargin is negative. Closing.