Buttons with no content don't lay out correctly

[jbphet]

Saw this in the sun demo just now while testing changes for phetsims/sun#292.

[jbphet]

I got some help from @marlitas on this, and we discovered that the preferred height and width are zero for these buttons. Here's a screen shot:

We also found that adding any content to the buttons makes them lay out correctly, though they seem to size themselves a little oddly. Here is a screenshot of a case where I added the option content: new Text( 'X' ) to each of the momentary buttons in demoMomentaryButtons.ts:

This led @marlitas and I to conclude that this is a general problem with WidthSizable, specifically its handling of the case where a button has no content. With this in mind, I'm going to move this issue to the scenery repo, change the name, and assign it to @marlitas and @jonathanolson.

[marlitas]

I ran into a buttonConstraint layout issue in Number Line Integers as well. I don't think they're exactly related, but seem to be in the same area. In this case ButtonNodeConstraint is not setting the correct minimumHeight for a RoundButton if the radius option is not passed in by the client.

The below patch fixes the bug I encountered, but also feels like it might be potentially mis-using minUnstrokedHeight and minUnstrokedWidth.

Would like to discuss with @jonathanolson

Subject: [PATCH] Center text dynamically, see: https://github.com/phetsims/number-line-integers/issues/108
---
Index: js/buttons/RoundButton.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/buttons/RoundButton.ts b/js/buttons/RoundButton.ts
--- a/js/buttons/RoundButton.ts	(revision 26851caba268b49f4abbd8a50e4ce4372cc625b6)
+++ b/js/buttons/RoundButton.ts	(date 1699573275871)
@@ -78,14 +78,6 @@
       assert && assert( typeof options.radius === 'number', `invalid radius: ${options.radius}` );
     }
 
-    if ( options.radius ) {
-      assert && assert( options.xMargin < options.radius, 'xMargin cannot be larger than radius' );
-      assert && assert( options.yMargin < options.radius, 'yMargin cannot be larger than radius' );
-
-      options.minUnstrokedWidth = options.radius * 2;
-      options.minUnstrokedHeight = options.radius * 2;
-    }
-
     // If no options were explicitly passed in for the button appearance strategy, pass through the general appearance
     // options.
     if ( !options.buttonAppearanceStrategyOptions ) {
@@ -99,6 +91,18 @@
     const buttonRadius = options.radius ||
                          Math.max( options.content!.width + options.xMargin * 2, options.content!.height + options.yMargin * 2 ) / 2;
 
+    if ( options.radius ) {
+      assert && assert( options.xMargin < options.radius, 'xMargin cannot be larger than radius' );
+      assert && assert( options.yMargin < options.radius, 'yMargin cannot be larger than radius' );
+
+      options.minUnstrokedWidth = options.radius * 2;
+      options.minUnstrokedHeight = options.radius * 2;
+    }
+    else {
+      options.minUnstrokedWidth = buttonRadius * 2;
+      options.minUnstrokedHeight = buttonRadius * 2;
+    }
+
     if ( options.content && options.radius ) {
       const previousContent = options.content;
       const minScale = Math.min(

[jonathanolson]

Tracking progress in button-layout branch

[jonathanolson]

Sandbox link initial: http://localhost/scenery/tests/sandbox.html?script=%2F%2F%20import%20sun%2Fjs%2Fbuttons%2FRoundPushButton%0A%2F%2F%20import%20sun%2Fjs%2Fbuttons%2FRectangularPushButton%0A%0Aconst%20roundPushButton%20%3D%20new%20RoundPushButton(%20%7B%20radius%3A%2050%20%7D%20)%3B%0Aconst%20rectangularPushButton%20%3D%20new%20RectangularPushButton(%20%7B%20size%3A%20new%20Dimension2(%20150%2C%20150%20)%20%7D%20)%3B%0A%0AroundPushButton.heightSizable%20%3D%20true%3B%0AroundPushButton.layoutOptions%20%3D%20%7B%20stretch%3A%20true%20%7D%3B%0A%0ArectangularPushButton.layoutOptions%20%3D%20%7B%20grow%3A%201%20%7D%3B%0A%0Ascene.addChild(%20new%20HBox(%20%7B%0A%20%20spacing%3A%2010%2C%0A%20%20children%3A%20%5B%20roundPushButton%2C%20rectangularPushButton%20%5D%2C%0A%20%20preferredWidth%3A%20500%2C%0A%20%20%0A%7D%20)%20)%3B%0A%0A%2F%2Fconsole.log(%20roundPushButton.width%2C%20rectangularPushButton.width%20)%0A%2F%2Fconsole.log(%20roundPushButton.parent.width%20)%0Aconsole.log(%20roundPushButton.minimumWidth%20)%0A

[jonathanolson]

@jbphet I believe this is resolved. Do you mind checking briefly?

[jbphet]

Sun demo (where the problem was initially noticed) is lookin' good:

Also, the values shown by the Scenery helper seem more reasonable:

I also checked the case where text content is added to the buttons as a sort of a regression test, and it looked fine.

@jonathanolson - Hopefully this is what you meant by "checking briefly". Let me know if you need any additional review. I haven't review the code, just the behavior.

[jonathanolson]

Yup, thanks! Closing.