Add support for dynamic locale to Keyboard Help

[pixelzoom]

Keyboard Help (scenery-phet/js/keyboard) currently does not support dynamic strings, and @jessegreenberg reports that dynamic layout is untested.

In phetsims/geometric-optics#441, I'm adding support for dynamic layout to Geometric Optics, and it has many translated strings that appear in Keyboard Help. I'm going to convert what is needed for Geometric Optics, then pass along to @jessegreenberg to finish the rest.

This is blocking for https://github.com/orgs/phetsims/projects/44.

[pixelzoom]

Ugh, this code is still in JavaScript. This would be so much easier if it was in TypeScript, because it's a type change fromstring to string | TReadOnlyProperty<string> type change.

[pixelzoom]

I made good progress for Geometric Optics, see screenshot below. Everything that's sim-specific is now supported by common code. I haven't seen any layout problems yet.

What I addressed in common code:

• All of the utility functions in KeyboardHelpSectionRow.
• All of the common-code help sections, except SliderControlsKeyboardHelpSection.
• Converted remaining files to TypeScript (wish I'd done this first)

TODO:

• Add support for dynamic strings in SliderControlsKeyboardHelpSection - see ComboBoxKeyboardHelpSection for an example, this blocks Geometric Optics
• Add support for dynamic strings in KeyboardHelpSection.getGrabReleaseHelpSection
• Address TODO comments that reference this issue
• PhET-iO instrumentation? There's no PhET-iO instrumentation anywhere in this code. So making StringProperties discoverable in Studio by instrumenting their associated Text/RichText nodes is not possible. I don't know if that's s requirement for https://github.com/orgs/phetsims/projects/44 - you should check with @kathy-phet.

Over to @jessegreenberg to complete this work.

[zepumph]

This is a big one for me over in phetsims/ratio-and-proportion#499, co-assigning.

[pixelzoom]

Reviewing scenery-phet issues for Q4 planning...

There has been no progress on the TODO list in #769 (comment). Assigning to @kathy-phet to prioritize.

[zepumph]

Subset of phetsims/scenery#1298

[zepumph]

I removed the two TODOs in KeyboardHelpSection, because we are supporting (from an API perspective) passing in a TReadOnlyProperty to the PDOM/Voicing code (readingBlockContent/innerContent).

[zepumph]

• With all the new PatternStringProperties being created, we need to implement dispose for KeyboardHelpSection to prevent memory leaks with phet-io state recreating the KeyboardHelpDialog.

[zepumph]

• Please note that the dispose issue is quite a bit more intense than I originally thought. Ratio and Proportion racked up 70MB of memory leak in a few minutes setting a new KeyboardHelpDialog every frame. I believe that each icon created with Text in KeyboardHelpIconFactory will need to support dispose. This is definitely a blocking issue for PhET-iO sims with a keyboard help dialog.
  UPDATE: I think this is a lie. Likely the leaks are elsewhere. We never dispose the KeyboardHelpDialog content. It lives forever, even though the dialog comes and goes. I created Eagerly create the KeyboardHelpDialog? joist#874 to help grok the confusion there.

If we need to support disposal of KeyboardHelpSections, then maybe we should start with this patch:

Index: scenery-phet/js/keyboard/help/SliderControlsKeyboardHelpSection.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/scenery-phet/js/keyboard/help/SliderControlsKeyboardHelpSection.ts b/scenery-phet/js/keyboard/help/SliderControlsKeyboardHelpSection.ts
--- a/scenery-phet/js/keyboard/help/SliderControlsKeyboardHelpSection.ts	(revision c7a36059fca7e5d4b24bb63e7d3ed8b8ef13aefc)
+++ b/scenery-phet/js/keyboard/help/SliderControlsKeyboardHelpSection.ts	(date 1666385944805)
@@ -52,6 +52,7 @@
 export default class SliderControlsKeyboardHelpSection extends KeyboardHelpSection {
 
   public static readonly ArrowKeyIconDisplay = ArrowKeyIconDisplay;
+  private readonly disposeSliderControlsKeyboardHelpSection: () => void;
 
   public constructor( providedOptions?: SliderControlsKeyboardHelpSectionOptions ) {
 
@@ -175,7 +176,32 @@
     // assemble final content for KeyboardHelpSection
     const content = [ adjustSliderRow, adjustSliderInSmallerStepsRow, adjustInLargerStepsRow, jumpToMinimumRow, jumpToMaximumRow ];
 
+
     super( options.headingString, content, options );
+
+    this.disposeSliderControlsKeyboardHelpSection = () => {
+      keyboardHelpDialogVerbSliderStringProperty.dispose();
+      keyboardHelpDialogVerbInSmallerStepsStringProperty.dispose();
+      keyboardHelpDialogVerbInLargerStepsStringProperty.dispose();
+      keyboardHelpDialogDefaultStepsStringProperty.dispose();
+      keyboardHelpDialogSmallerStepsStringProperty.dispose();
+      keyboardHelpDialogLargerStepsStringProperty.dispose();
+      jumpToMinimumStringProperty.dispose();
+      jumpToMaximumStringProperty.dispose();
+      jumpToMinimumDescriptionStringProperty.dispose();
+      jumpToMaximumDescriptionStringProperty.dispose();
+
+      [ shiftKeysString, keysStringProperty ].forEach( property => {
+
+        // We only own these for disposal if a PatternStringProperty
+        property instanceof PatternStringProperty && property.dispose();
+      } );
+    };
+  }
+
+  public override dispose(): void {
+    this.disposeSliderControlsKeyboardHelpSection();
+    super.dispose();
   }
 }
 
Index: scenery-phet/js/keyboard/help/KeyboardHelpSectionRow.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/scenery-phet/js/keyboard/help/KeyboardHelpSectionRow.ts b/scenery-phet/js/keyboard/help/KeyboardHelpSectionRow.ts
--- a/scenery-phet/js/keyboard/help/KeyboardHelpSectionRow.ts	(revision c7a36059fca7e5d4b24bb63e7d3ed8b8ef13aefc)
+++ b/scenery-phet/js/keyboard/help/KeyboardHelpSectionRow.ts	(date 1666385282172)
@@ -9,6 +9,7 @@
  * @author Jesse Greenberg (PhET Interactive Simulations)
  */
 
+import Emitter from '../../../../axon/js/Emitter.js';
 import TReadOnlyProperty from '../../../../axon/js/TReadOnlyProperty.js';
 import optionize, { combineOptions } from '../../../../phet-core/js/optionize.js';
 import StrictOmit from '../../../../phet-core/js/types/StrictOmit.js';
@@ -85,6 +86,8 @@
   // when the row is activated with a click.
   public readonly readingBlockContent: VoicingResponse | null;
 
+  public disposeEmitter: Emitter;
+
   public constructor( text: Text | RichText, label: Node, icon: Node, providedOptions?: KeyboardHelpSectionRowOptions ) {
     const options = optionize<KeyboardHelpSectionRowOptions>()( {
       readingBlockContent: null
@@ -94,6 +97,11 @@
     this.label = label;
     this.icon = icon;
     this.readingBlockContent = options.readingBlockContent;
+    this.disposeEmitter = new Emitter();
+  }
+
+  public dispose(): void {
+    this.disposeEmitter.emit();
   }
 
   /**
@@ -124,9 +132,14 @@
 
     iconBox.innerContent = options.labelInnerContent;
 
-    return new KeyboardHelpSectionRow( labelText, labelBox, iconBox, {
+    const keyboardHelpSectionRow = new KeyboardHelpSectionRow( labelText, labelBox, iconBox, {
       readingBlockContent: options.readingBlockContent || options.labelInnerContent
     } );
+    keyboardHelpSectionRow.disposeEmitter.addListener( () => {
+      labelText.dispose();
+      labelIconGroup.dispose();
+    } );
+    return keyboardHelpSectionRow;
   }
 
   /**
Index: scenery-phet/js/keyboard/help/KeyboardHelpSection.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/scenery-phet/js/keyboard/help/KeyboardHelpSection.ts b/scenery-phet/js/keyboard/help/KeyboardHelpSection.ts
--- a/scenery-phet/js/keyboard/help/KeyboardHelpSection.ts	(revision c7a36059fca7e5d4b24bb63e7d3ed8b8ef13aefc)
+++ b/scenery-phet/js/keyboard/help/KeyboardHelpSection.ts	(date 1666385775689)
@@ -79,6 +79,8 @@
   private readonly iconVBox: VBox;
   private readonly contentHBox: HBox;
 
+  private readonly disposeKeyboardHelpSection: () => void;
+
   public static readonly DEFAULT_VERTICAL_ICON_SPACING = DEFAULT_VERTICAL_ICON_SPACING;
 
   /**
@@ -164,6 +166,15 @@
     this.keyboardHelpSectionRows = content;
 
     this.setReadingBlockNameResponse( this.generateReadingBlockNameResponse() );
+
+    this.disposeKeyboardHelpSection = () => {
+      headingText.dispose();
+    };
+  }
+
+  public override dispose(): void {
+    this.disposeKeyboardHelpSection();
+    super.dispose();
   }
 
   /**
Index: ratio-and-proportion/js/common/view/RAPKeyboardHelpContent.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/ratio-and-proportion/js/common/view/RAPKeyboardHelpContent.ts b/ratio-and-proportion/js/common/view/RAPKeyboardHelpContent.ts
--- a/ratio-and-proportion/js/common/view/RAPKeyboardHelpContent.ts	(revision f8c92c9b2dcdd581b6551a55bdf302588abeca0d)
+++ b/ratio-and-proportion/js/common/view/RAPKeyboardHelpContent.ts	(date 1666385153425)
@@ -24,8 +24,10 @@
 
 class RAPKeyboardHelpContent extends TwoColumnKeyboardHelpContent {
 
+  private readonly disposeRAPKEyboardHelpContent: () => void;
+
   /**
-   * @param challengeHelpSection - keyboard help section for determining how to change the target ratio
+   * @param challengeHelpSection - keyboard help section for determining how to change the target ratio. We will dispose this!
    * @param [providedOptions]
    */
   public constructor( challengeHelpSection: KeyboardHelpSection, providedOptions?: RAPKeyboardHelpContentOptions ) {
@@ -43,10 +45,23 @@
       withCheckboxContent: true
     } );
 
-    const leftContent = [ moveLeftOrRightHandHelpSection, new BothHandsHelpSection() ];
+    const bothHandsHelpSection = new BothHandsHelpSection();
+    const leftContent = [ moveLeftOrRightHandHelpSection, bothHandsHelpSection ];
     const rightContent = [ challengeHelpSection, basicActionsHelpSection ];
 
     super( leftContent, rightContent, providedOptions );
+
+    this.disposeRAPKEyboardHelpContent = () => {
+      moveLeftOrRightHandHelpSection.dispose();
+      bothHandsHelpSection.dispose();
+      challengeHelpSection.dispose();
+      basicActionsHelpSection.dispose();
+    };
+  }
+
+  public override dispose(): void {
+    this.disposeRAPKEyboardHelpContent();
+    super.dispose();
   }
 }
 

[zepumph]

UPDATE: I will likely apply the above disposal patch after proceeding/finishing with phetsims/joist#874

[zepumph]

Also note much of this was covered by #780

[pixelzoom]

@zepumph said:

Instructions in #769 (comment) talk about how to make sure that a keyboard help dialog for your sim doesn't have a memory leak in PhET-iO.

I still don't understand. Am I still supposed to apply the patch, as the comment instructs? Why don't I see the patch code in KeyboardHelpButton.ts? What exactly am I supposed to be reviewing?

@zepumph Let's schedule a time to review this together on Zoom, rather than tossing the issue back and forth.

[pixelzoom]

Disposable.ts has a memory leak. The line added in this diff is missing, so disposeEmitter listeners are never freed.

  public dispose(): void {
    assert && assert( !this.isDisposed, 'Disposable can only be disposed once' );
    this._disposeEmitter.emit();
+   this._disposeEmitter.dispose();
    this.isDisposed = true;
  }

[pixelzoom]

@zepumph and I reviewed on Zoom. Summary:

• There's a memory leak in Disposable.ts, see Add support for dynamic locale to Keyboard Help #769 (comment)

• Let's not make KeyboardHelpSection dispose its content. It's confusing having one special case that's unlike anything else. Address TODO comments in GOKeyboardHelpContent.ts when this has been done.

• Investigate having a method like disposeDeep that calls dispose for a branch of the tree rooted as some Node. Nodes in that branch would still need to have proper dispose methods. But disposeDeep would be a way to say (for example) "yes, I want my keyboard-help content and everything it involves to be disposed".

Assigning to @zepumph to address those things.

Leaving this assigned to me also. I'm going to commit changes to GOSim.ts and GOKeyboardHelpContent.ts. My next step is to test using the patch in #769 (comment), which @zepumph clarified is a way of testing keyboard-help disposal without having to use the State Wrapper.

[pixelzoom]

Using the procedure and patch in #769 (comment), here are heap sizes for geometric-optics:

• 65.9MB with 0 KeyboardHelpDialog instances
• 67.6MB with 10 instances
• 68.5MB with 100 instances

In #769 (comment), @zepumph tells me what to expect when there's a leak, but doesn't tell me what to expect when there's not a leak. I'd expect the heap sizes to be the same, which they are not. I took another look through GOKeyboardHelpContent.ts, and I don't see anything that I'm failing to dispose.

So... I'm guessing that there's still a small leak somewhere in joist or scenery-phet.

@zepumph I'm going to unassign myself. Let me know if you need anything else.

[zepumph]

See phetsims/axon#433 for the issues discussed above with Disposable.

Investigate having a method like disposeDeep that calls dispose for a branch of the tree rooted as some Node. Nodes in that branch would still need to have proper dispose methods. But disposeDeep would be a way to say (for example) "yes, I want my keyboard-help content and everything it involves to be disposed".

Should be done in phetsims/scenery#1523.

As for the addition of 4MB or so when creating 100 dialogs, I do not believe that this is an issue. It is very possible that this is mostly/some in the browser/view logic weeds, and I have spent enough time diving through memory testing tools to know that it is likely not worth the effort to find them at this time.

I have also gotten rid of the automatic disposal of KeyboardHelpSectionRows in KeyboardHelpSection.

@pixelzoom will you please review this issue. I am not sure what else is left that won't be handled by side issues.

[pixelzoom]

It looks like we've covered everything here. Good work, and closing.

I'm going to do phetsims/ph-scale#253 and phetsims/fourier-making-waves#227 (which were linked above) while this is fresh in my mind.