phetsims / scenery-phet

Reusable components based on Scenery that are specific to PhET simulations.
MIT License
8 stars 6 forks source link

Buggy readingBlockNameResponse in KeyboardHelpSection #772

Open pixelzoom opened 2 years ago

pixelzoom commented 2 years ago

Re this lint rule: https://github.com/phetsims/chipper/issues/1338

It exposed a problem in KeyboardHelpSection.ts. Relevant code:

  private generateReadingBlockNameResponse(): string {

    // Include the section heading. Headings typically don't have punctuation, but don't use a period because
    // it may appear to the synth as an abbreviation and change the pronunciation.
    let readingBlockNameResponse = '';

    //TODO https://github.com/phetsims/scenery-phet/issues/769 is it OK to use a dynamic translated string here?
    readingBlockNameResponse += `${this.headingStringProperty.value}, `;

    // Append the readingBlockNameResponse assigned to each row.
    this.keyboardHelpSectionRows.forEach( row => {
      if ( row.readingBlockContent ) {
189     readingBlockNameResponse += `${row.readingBlockContent} `;
      }
    } );

Line 189 in the above block of code is suspect:

189     readingBlockNameResponse += `${row.readingBlockContent} `;

row.readingBlockContent is of type VoicingResponse | null, and VoicingResponse does not implement toString. So will currently added either "[object Object]" or "null" to readingBlockNameResponse, both of which would seem to be a serious problem.

Assigning to the author @jessegreenberg. High priority please, so that we can wrap up https://github.com/phetsims/chipper/issues/1338.

pixelzoom commented 2 years ago

I was surprised that the lint error at line 189 (above) went away when https://github.com/phetsims/chipper/issues/1338 was closed by @zepumph.

VoicingResponse is defined as:

export type VoicingResponse = ResponseCreator | ResolvedResponse;

which evaluates to:

TReadOnlyProperty<string> | (() => ResolvedResponse) | string | number | null

It would seem that () => ResolvedResponse does not have a useful toString, so I'm not sure how line 189 above works when row.readingBlockContent is a function.

189     readingBlockNameResponse += `${row.readingBlockContent} `;
zepumph commented 2 years ago

Subset of https://github.com/phetsims/scenery/issues/1298