Long screen names push navigation bar icon to the right when sim is launched

[KatieWoe]

For phetsims/qa#662. Seen on MacOS 11 in FIrefox. Also went back to check recent GaO issue phetsims/qa#657 and saw it there, so assigning @samreid as well.

When renaming one of the screens, the behavior in studio itself looks fine. However, when the sim is launched the icon above the longer name has moved and can overlap the neighboring icons.

x

[pixelzoom]

This is not specific to Natural Selection. Transferring this issue to joist.

[pixelzoom]

I've lost count of how many times we've had problems with alignment of text + icons in the navigation bar.

Is this a duplicate of #718? Why isn't that issue blocking? Assigning @jonathanolson, since he's assigned #718.

[pixelzoom]

Tracking for Natural Selection 1.4 in phetsims/natural-selection#295. This issue is blocking because it affects usability of the navigation bar.

[pixelzoom]

This is blocking phetsims/qa#657 and phetsims/qa#662. Since the latter has top priority, this issue is elevated to top priority.

[samreid]

Wrapping in nested nodes seems safe, and seems to help a lot:

Index: js/NavigationBarScreenButton.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/NavigationBarScreenButton.js b/js/NavigationBarScreenButton.js
--- a/js/NavigationBarScreenButton.js	(revision 4cf017dc6096c19e12246d288d5fbe9362ad0ef0)
+++ b/js/NavigationBarScreenButton.js	(date 1625239974056)
@@ -91,7 +91,7 @@
 
     // spacing set by Property link below
     const iconAndText = new VBox( {
-      children: [ iconAndFrame, text ],
+      children: [ new Node( { children: [ iconAndFrame ] } ), new Node( { children: [ text ] } ) ],
       pickable: false,
       usesOpacity: true, // hint, since we change its opacity
       maxHeight: navBarHeight

However, now the highlight overlay is not centered:

There is also a similar problem on the home screen:

That is solved with a combination of resize: true and nested nodes, but I don't know if the resize:true is safe (the comment says it is not)

Index: js/HomeScreenButton.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/HomeScreenButton.js b/js/HomeScreenButton.js
--- a/js/HomeScreenButton.js	(revision 4cf017dc6096c19e12246d288d5fbe9362ad0ef0)
+++ b/js/HomeScreenButton.js	(date 1625240665678)
@@ -42,7 +42,7 @@
 
     options = merge( {
       cursor: 'pointer',
-      resize: false, // don't resize the VBox or it will shift down when the border becomes thicker
+      resize: true,
       showUnselectedHomeScreenIconFrame: false, // put a frame around unselected home screen icons
 
       // pdom
@@ -107,7 +107,7 @@
       textPropertyOptions: { phetioReadOnly: true } // text is updated via screen.nameProperty
     } );
 
-    super( merge( { children: [ nodeContainer, text ] }, options ) );
+    super( merge( { children: [ new Node( { children: [ nodeContainer ] } ), new Node( { children: [ text ] } ) ] }, options ) );
 
     // @public (read-only)
     this.screen = screen;

It seems the preferable long-term solution would be to completely rewrite the joist buttons, but that would be a lot of effort and require coordinate between designers, phet-io, a11y, etc.

[samreid]

I found we are no longer changing strokes or structure on highlight change, so it is safe to resize:true on the HomeScreenButton. I applied that change and the nested Node container for the dynamic text, and this is working much better. The navigation bar highlight is still a little off centered though.

[samreid]

I found that running updateLayout in the next frame fixes everything, but it is unclear why running updateLayout after changing the text is insufficient. I committed it as a workaround. Note that using the runOnNextTick makes it so the nested new Node is not necessary--but I left it in as a safety valve because I don't really understand the problem here.

If @jonathanolson has time to consult on this, it would be best, but if we need to publish before we get @jonathanolson assistance, maybe that is OK? I'll reach out to @jonathanolson.

[pixelzoom]

Highly depends on where branches are at,
...
@pixelzoom is there anything you need from me on this?

Yes. I need the exact set of cherry-picks needed to patch NS 1.4.

[samreid]

I'll work on that for gravity and orbits, it should be the same for NS. I'm just planning to cherry-pick all commits from this issue, I'll let you know how it goes. I'll cherry pick in this order:

321e788
58fd741
6299e69
eb2a5d9
720b149
33aab76

phetsims/scenery@f0adace
phetsims/scenery@7656bb0
phetsims/scenery@aa0952d

[samreid]

I cherry picked as described above and testing worked well. Over to @pixelzoom to cherry-pick for Natural Selection (I still have this problem which makes it difficult to help in this case phetsims/natural-selection#298 (comment) )

[pixelzoom]

Shas to cherry-pick are noted in phetsims/natural-selection#295 (comment). I'll continue patching NS 1.4 in that issue. Unassigning from this issue.

[zepumph]

I'd be inclined personally to not go back and write the test, because this is exceedingly unlikely to break in the same way in the future. @zepumph objections?

Sounds good to me!

I think this issue is ready to close, as cherry-pick issues are in their own repos.