Unable to make "Environmental Factors" string longer in studio

[arouinfar]

While looking into a broader issue related to long strings in studio, I think I stumbled across a different bug in Natural Selection on master. I tested both Win10/Chrome and macOS 10.15.5/Chrome and the issue appears in both platforms.

I tried editing naturalSelection.introScreen.view.environmentalFactorsPanel.titleNode.textProperty in studio. If the string has fewer characters than Environmental Factors, everything looks fine:

However, as soon as the new string is longer than the original, it will revert back to Environmental Factors.

Steps to reproduce

1. Navigate to naturalSelection.introScreen.view.environmentalFactorsPanel.titleNode.textProperty in studio.
2. Change the current value of the string to something longer than Environmental Factors. (Adding an extra letter to the original string or deleting it an typing in something new will both exhibit the problem.)
3. String will revert back to Environmental Factors.

Note that this appears to be isolated to the Environmental Factors string (on both screens). Editing the title of the Alleles panel, naturalSelection.*Screen.view.graphs.pedigreeNode.allelesPanel.titleNode.textProperty, behaves as expected.

[pixelzoom]

Wow, that's really odd. With longer strings, it should just scale down to fix.

There might be something else going on here. Just a hunch, I haven't investigated yet. For both the "Add Mutations" and "Environmental Factors" panels, the title is adjusted to be singular vs plural, depending on how many mutations or factors there are. So something may be triggering the code that sets the title.

[pixelzoom]

Hmmm... A conversation in the Slack phet-io channel (below) indicates that this is a general issue, and can be reproduced in several sims. But I don't see a general GitHub issue referenced anywhere here. @arouinfar can you please clarify?

Slack phet-io

Amy Rouinfar 3:14 PM I've found a general phet-io issue related to long strings, and I'm not sure which repo would be most appropriate. Basically, editing a string in studio via a nameProperty or textProperty doesn't respect the maxWidth and the layout can get broken. I've found a few examples across different sims, and it's not isolated to a single UI component. Sam Reid 4:16 PM Do some other sites respect maxWidth? It could be that some sim components don’t specify maxWidth yet. Amy Rouinfar 4:18 PM Sometimes maxWidth is respected and things behave as you would expect. Sometimes the string scales down before reaching the maxWidth. Sometimes the maxWidth is violated. 4:18 An example of strings scaling down before reaching the maxWidth is editing a screen name, such as gravityAndOrbits.modelScreen.nameProperty. 4:19 An example of maxWidth being violated is molarity.molarityScreen.view.beakerNode.labelNode.textNode.textProperty. 4:21 Other examples of layout issues include molarity.molarityScreen.view.concentrationDisplay.maxNode.qualitativeNode.textProperty and statesOfMatter.interactionScreen.view.atomsControlPanel.title.textProperty. 4:23 I think it probably boils down to changing strings after the sim is initialized can have weird consequences sometimes, but I thought it should probably be documented in an issue somewhere. Kathryn Woessner:house_with_garden: 4:23 PM I think there might be issues from SoM that you might be able to add to or generalize. It sounds familiar.

[arouinfar]

@pixelzoom Natural Selection was the only sim I saw with a string that would revert back to its original. Other sims I tested included Energy Forms and Changes, Gravity Force Lab, Gravity and Orbits, Molarity, pH Scale, Projectile Motion, and States of Matter.

As for the general issue, there's isn't one. Editing strings in studio can exhibit all sorts of bad behavior, but based on a conversation with @samreid the issues are all sim-specific. There is an agenda item in phet-io meeting to discuss next steps.

[pixelzoom]

The titleNode for the "Add Mutations" panel behaves similarly. (I instrumented it in the above commit, which seemed like an oversight.)

This is not related to the singular/plural feature of panel titles in this sim.

[pixelzoom]

There's something weird going on here with maxWidth. I temporarily instrumented the label on the "Add a Mate" button, using this patch:

patch

```diff Index: js/common/view/AddAMateButton.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- js/common/view/AddAMateButton.js (revision 8fbf70506235965069d8a36f1fcc2deafce08f1d) +++ js/common/view/AddAMateButton.js (date 1599582113953) @@ -35,7 +35,8 @@ assert && assert( !options.content, 'AddAMateButton sets content' ); options.content = new Text( naturalSelectionStrings.addAMate, { font: NaturalSelectionConstants.PUSH_BUTTON_FONT, - maxWidth: 150 // determined empirically + maxWidth: 150, // determined empirically + tandem: options.tandem.createTandem( 'textNode' ) } ); super( options ); ```

When I edit addAMateButton.textNode.textProperty in Studio, maxWidth appears to be totally ignored. See below:

[pixelzoom]

Sam Reid:house_with_garden: 10:37 AM That is so crazy! I’ll take a look

Here’s the trouble

    checkboxes.boundsProperty.link( () => {
      const visibleCount = _.filter( checkboxes.children, child => child.visible ).length;
      titleNode.text = ( visibleCount === 1 ) ?
                       naturalSelectionStrings.environmentalFactor :
                       naturalSelectionStrings.environmentalFactors;
    } );

Chris Malley 10:41 AM Thanks! So checkboxes.boundsProperty is changing when the panel's title gets wider? That's odd, but I guess I can observe checkboxes.localBoundsProperty instead.

[pixelzoom]

Fixed in the above commit, for both "Add Mutations" and "Environmental Factors" panels.

I also implemented something that @samreid suggested. If the panel title has been changed (via Studio) to something other than "Environmental Factors" or "Environmental Factor", then the singular/plural code is ignored.

@arouinfar do you want to have a look?

[samreid]

The commit uses localBoundsProperty to decide when to update the strings. But the code it uses to determine the visible count is

const visibleCount = _.filter( rows.children, child => child.visible ).length;

Therefore it seems more straightforward to multilink to the rows.children's visibleProperty instances to decide when to update.

[pixelzoom]

checkboxes is a VBox whose only children are the checkboxes. If the visibility of any child changes, the VBox gets bigger or smaller due to the excludeInvisibleChildrenFromBounds feature that was recently added to layoutBox. Why do you think it's "more straightforward" to observe every child?

[samreid]

Do you want this callback to run if the bounds of a child changes too?

[pixelzoom]

What I'd really like is something like:

checkboxes.numberOfVisibleChildrenProperty.link( ... )

But that doesn't exist.

So I did what I think is the easiest, most straightforward way to implement a feature that has a 1:10000 chance of ever being used in the wild. And if the bounds change for some reason other than the visibility of a child changing, the behavior is still correct.

[pixelzoom]

Implemented in the above commit. Big changes were required. This doesn't feel like a big win to me, especially this late in the game.

[arouinfar]

@pixelzoom looks good in master. I am unable to reproduce the original issue, and I even tried changing the strings to have the incorrect singular/plural.