Add flag to identify a11y strings in strings_*.json

[jessegreenberg]

Hopefully this is the right place for this issue.

We have translatable strings in sim-strings_*.json, and all a11y strings in some file like SimA11yStrings.js. Separation is so that a11y strings are not translatable. We hope to have them translatable someday, but PhET can't do that work now.

In the meantime, it was recommended that we add some flag to string entries like
accessible: true or visible: false or accessibleVisible: false to signify that these strings are invisible and only available with an assistive device. With this flag, they won't show up at all in the translation utility.

This way, all strings will be in one place, we can delete the SimA11yStrings.js files, and it will be easier to enable i18n for these strings when the time comes.

@jbphet does this still seem reasonable? This would hold us over until a11y is fully ready for i18n, so probably not worthwhile if you think it would take much time.

[zepumph]

Coassigning, @jbphet maybe we could pair on this? Or if you don't have time for it, I bet this could go into my a11y time, so I will be able to get to this sooner =).

[jbphet]

Yes, adding a flag to identify strings that are only for accessibility seems like the way to go. I think that the semantics of that flag will need to be that the string is used only for accessibility. In other words, if there is a string that serves a dual purpose in that it is visually displayed to the user and is also used for accessibility (and feel free to speak up if we never expect this situation to arise), the flag is not set on that string.

Also, @jessegreenberg mentioned possibly using something like visible: false. I'd rather that we be more specific, and also would like to support letting the user decide if they want to translate accessibility strings, so I'd suggest things like accessibility: true, a11yOnly: true, forA11y: true.

[jbphet]

I'd like to be involved in the initial planning on this, and then I think @zepumph can take it from there. Since I'm not too clear on the priority of this, can either @jessegreenberg or @zepumph set up a meeting when it becomes time to get going on this and include me?

[jessegreenberg]

I expect that strings will serve dual purposes. A visible type flag that can be removed might still be useful if we want to make a11y strings available for i18n on a sim by sim basis, rather than enabling all at once somewhere in rosetta.

This issue isn't critical, but the sooner it can be added the better so we don't keep creating SimA11yStrings.js files.

[zepumph]

I think I am beginning to understand what @jessegreenberg is speaking about. @jbphet mentioned two situations:

1: not a11y string for visual text (I think we all agree on this)
2: anything used for a11y (this includes common code a11y strings)

I believe what @jessegreenberg is speaking to may be best described with an example. Sim 1 is published with a11y features that want to be translated. Sim 2 is published without any a11y support. Both simulations were published after instrumenting a Common Code Node 1 (CCN1) with a11y. To be clear this means that both sims have an a11y specific string inside of them meant for CCN1. So when we flip a switch and want to translate a11y features, both technically "have" the a11y strings needed for CCN1, but Sim 2 has a11y hidden behind a query parameter, never mean for use publically/in production. We probably don't want those a11y strings populating the translation utility for Sim 2.

We need some sort of way to decipher whether or not to populate Translation X with common code string Y based on if the sim being translated for Translation X is implemented in a11y to a point where that string is needed.

[jbphet]

Notes from Zoom discussion with @jessegreenberg, @zepumph, and myself on 6/27/2018:

• Rosetta currently extracts the list of strings that need to be translated from the published version of the simulation. The strings in the published sims do not include meta-data, they are a simple key-value mapping. We kicked around the idea of changing this so that the metadata is included, but this would increase the sim size by a bit, and would likely increase the size of the _all.html file substantially. @zepumph did a fast prototype of this and found that it increased the size of the build-an-atom_all.html file by 15-20%. An alternative way for rosetta to determine whether a string is for a11y is to look at the English version of the string file on GitHub and examine the meta-data for each string, and then decide whether to present it to the user.
• @jessegreenberg said that we probably will want the ability, at least for a while, to control which simulations allow translation of a11y strings through rosetta. One way to do this would be to have a GitHub file with a list of sims for which a11y string translation is enabled, and rosetta would re-read it each time the translation page was presented. In that case, an edit and push of this file would be enough to enable or disable a11y string translation for a given sim.
• In the past, we have discussed allowing users to decide whether or not they want to translate a11y strings. We should probably have a design meeting to decide how we want to do this.
• Some of the current strings, such as sim title, screen title, and so forth, are used by accessibility, so the flag that we are thinking would need to be added to the metadata would need to distinguish strings that are only used for a11y. We are thinking something like a11yOnly: true.

Marking for developer meeting to get input on each of the above items from the dev team.

[samreid]

@zepumph did a fast prototype of this and found that it increased the size of the build-an-atom_all.html file by 15-20%.

A phet-brand build of Build an Atom is 1.8MB. 15% of that is 270KB. For comparison, Node.js is 5160 LOC and 202KB.

We hope to have them translatable someday, but PhET can't do that work now.

Why can't PhET do that work now? It will take us N hours to implement a workaround to suppress a11y strings vs M hours to provide a way to see and test a11y strings (along with all the strings). How many hours do we have to work on this?

In the long run, we want all of the a11y text translated--let's focus on providing translators ways to see and test the strings rather than a way to suppress the strings.

[jonathanolson]

A phet-brand build of Build an Atom is 1.8MB. 15% of that is 270KB. For comparison, Node.js is 5160 LOC and 202KB.

It minifies to ~36 kB, which is significantly less. I'm not sure this comparison applies.

Why can't PhET do that work now?

I'm also interested in this. Presumably we'll need other string metadata in the future.

We kicked around the idea of changing this so that the metadata is included

Presumably we could include the metadata once per string (instead of once per each string+locale combination)?

One way to do this would be to have a GitHub file with a list of sims for which a11y string translation is enabled

Sounds workable.

[jbphet]

This issue was reviewed in the 7/5/2018 developer meeting and we made decisions on three of the four items above. Summarizing:

Rosetta currently extracts the list of strings that need to be translated from the published version of the simulation...

We should go with the approach of leaving the strings as they are in the sims and using the data from GitHub to determine whether a string is only for a11y. This minimizes impact to the size and structure of sims, and the metadata isn't needed during runtime. If there ever comes a time when the metadata is needed at runtime, we will revisit.

[W]e probably will want the ability, at least for a while, to control which simulations allow translation of a11y strings through rosetta...

We should go with the suggested approach, which is to have a file in the rosetta repo that tracks the list of sims for which the a11y string should be presented for translation. At some point in the future, all sims will support this if they have a11y strings, but that time is probably at least one year out.

We are thinking something like a11yOnly [for the flag name]

No objections to this.

The remaining open issue concerns how to modify the rosetta UI to give the user the option of supporting a11y strings. I'd like some input from @ariel-phet and @emily-phet on this. Specific questions are:

@emily-phet - what is the desired time from for supporting translation of a11y strings?

@ariel-phet - How much effort should we put into the UI changes and who should work on the design? It could be something as simple as a checkbox that says "Show accessibility strings" at the top of the page, but if we'd like anything more than that we should have a design meeting and potentially do some mockups.

[samreid]

It would be nice to show something like the a11y-view, so that translators can see the a11y strings better.

[jbphet]

@zepumph and @jessegreenberg - how would you like to proceed on actually moving the strings into the string files? I was thinking that we'd log an issue per repo that currently has a11y strings in another file, but you guys are the experts so you should chime in.

[jessegreenberg]

It would be nice to show something like the a11y-view, so that translators can see the a11y strings better.

That would be very nice!

I was thinking that we'd log an issue per repo that currently has a11y strings in another file

Sounds great, plus common code repos. There currently 14 of these files.

[zepumph]

At some point in the future, all sims will support this if they have a11y strings, but that time is probably at least one year out.

I'm not totally clear on this, because all sims have joist, and so will have joist's a11y strings in them, even if they don't have and accessibility content. As a result I think we need to design this solution in a way that recognizes that it will need to be around for a very long time. This manual file approach seems like it will be a pain once we are managing 20 a11y sims in a year, but all 100 other HTML sims (yay optimism) still have will end up requiring a11y strings that we don't want translated.

What if we had a two pronged solution.

1. We have metadata from the website in which we know the list of all a11y sims.
2. In rosetta we can use this file solution as an override, to opt in/out specific sims as needed.

That allows flexibility for a less manual approach in the future.

how would you like to proceed on actually moving the strings into the string files? I was thinking that we'd log an issue per repo that currently has a11y strings in another file, but you guys are the experts so you should chime in.

Since this is not top priority for the a11y project right now, I think an issue per repo is a good idea, with a "chip-away" mindset, but I would suspect that many of these may sit around for a while. I will be in friction over the coming months, and @mbarlow12 is in faradays-law right now, so maybe we could use those as test cases. Also JT and OL with sonification are (hopefully) on the docket for publishing in September, so they could be good to try to implement this with too.

It would be nice to show something like the a11y-view, so that translators can see the a11y strings better.

right now the a11y view is built with every build, but it is filtered out and not copied into production builds. One potential, minimal solution would be to publish the a11y view, and then load that (in an iframe or something) on the translation page, just so translators can see the english version. Stage 2 of that would be some way to manually feed temp strings into a new instance of the a11y view as a "testing mode" (I think we do that right now for normal sims), although URL length may become a problem.

[samreid]

I merged master into the branches, but still didn't see the expected console.log output.

[samreid]

@zepumph and I discussed this and it is looking good. Some ideas going forward:

(1) maybe we can generalize the code in ChipperStringUtils.getStringFromMap to recursively check string suffix | child structure . Ideally, this would get rid of the explicit checks for "a11y" key prefix and could be used from requirejs side and built side. The signature could be something like:

// Search suffixes or children equally
harvestPathsAll(string,country,locale,english){
	if (harvestPaths(string,country)){

	}else(harvestPaths(string,locale){}
        ...
}

const results = harvestPathsAll(...);

// prevent collisions like {my.name} vs {my{name}}
assert && assert(results.length===1, 'should have only one hit');

[zepumph]

We may also be able to use this function in replacement of phet.chipper.strings.get, and some rosetta TranslationUtils.js functions.

[zepumph]

After more discussion with @samreid, here are some notes:

goals:

• move a11y strings to string json files
• support nesting in a11y strings
• make sure rosetta ignores a11y strings
• make good and reusable code, only have one way of doing things.
• support nesting for all string
• (not yet) support a11y string translation

cases to support:

• Rosetta populating view
• rosetta saving to babel
• rosetta previewing translation
• rosetta loading strings
• requirejs en
• requirejs other language
• built mode en
• built mode other language

maybe we create a TranslatableStringUtil.js that has all the string code used in all the above cases.

• Run in rosetta, requirejs plugin, build sim, building
• getter for strings AND
• creating of a map
  where are fallback strings integrated with preferred locale strings
• (even if a bit duplicated to begin with)

---

Next Steps:

• go through checkboxes in above issue and discuss each element
• figure out how to load rosetta and test some string parsing changes to it (see Make sure rosetta can read both flat and structured string keys. #215)
• move code to TranslatableStringUtil.js (see Strings in requirejs and build process should support nested objects chipper#786)
• discuss and understand where things should be flat vs nested
• make sure that we error out if "hello.hi": 'string' and "hello": {hi: 'string'} were in the same file MK UPDATE: this was not possible because of the implementation of _.at(). Instead we just prefer the flat key to the nested. This was discussed with @samreid
• create issues to audit dangling white space in a11y strings. (see strip leading/trailing space from string values chipper#619)

where are fallback strings integrated with preferred locale strings?

[samreid]

Some review comments from the branch:

• Can ChipperStringUtils.formatStringValues be implemented with ChipperStringUtils.forEachString?
• Can ChipperStringUtils.getStringFromMap be implemented with ChipperStringUtils.forEachString?
  MK UPDATE: no need not that it uses _.at()
• ChipperStringUtils.formatStringValues has mismatched JSDoc variable names
• Add unit tests for the ChipperStringUtils functions.
  ChipperStringUtils.getStringFromMap should be marked public
• @param {Object.<string, intermediate:Object|{value: string}>} map
  should use a typedef for easier readability

[zepumph]

Much of the above was completed at the end of phetsims/chipper#786.

[zepumph]

Lots of work has been done on this issue today. In general this was just cleanup for the implementation over in phetsims/chipper#786. It was decided there to focus on requirejs/build mode in sims. We added nested string objects only in English JSON files, all other locales are still flat. Furthermore the build string map is flat. Keeping this implementation meant that no rosetta code had to be touched to support this. We were able to factor out most logic shared by requirejs (string plugin) and build mode (getStringMap) into ChipperStringUtils.js, and I'm quite happy with that implementation.

Here is a list of further work that will be done in separate issues:

• Delete *.A11yStrings.js files Delete *.A11yStrings.js files chipper#795
• string plugin getWithCache doesn't really cache that much string plugin getWithCache doesn't really cache that much chipper#797
• Make sure rosetta can read both flat and structured string keys. Make sure rosetta can read both flat and structured string keys. #215
• strip leading/trailing space from string values strip leading/trailing space from string values chipper#619
• Support comments in string files Support comments in string files chipper#781
• Rosetta should ignore the "a11y" key in strings files Rosetta should ignore the "a11y" key in strings files #214
• merge to master branch: a11ystring-plugin chipper#777
• Can phet.chipper.stings.get and string!getStringFromFileContents be simplified? Can phet.chipper.stings.get and string!getStringFromFileContents be simplified? chipper#796

I'm going to assign this over to @samreid to review. In #193 (comment) it looked like you started a review, and even though we talked this morning. I wanted to make sure that you had a final time to look through what you wanted to.

[samreid]

It would be nice to inline forEachStringImplementation, could be done like so:

Index: js/common/ChipperStringUtils.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/common/ChipperStringUtils.js	(revision 163a9b419b1960719c0b64417fd5c977e6d557d5)
+++ js/common/ChipperStringUtils.js	(date 1569338689828)
@@ -168,35 +168,26 @@
      * Call a function on each object with a value attribute in an object tree.
      * @param {Object.<string, Object|{value:string}>} map - string map, like a loaded JSON strings file
      * @param {function(key:string, {value:string})} func
+     * @param {string} [keySoFar] - as we recurse down, build up a string of the key separated with dots.
      * @public
      */
-    forEachString( map, func ) {
-      forEachStringImplementation( '', map, func );
-    }
-  };
-
-  /**
-   * This implementation function helps keep a better api for `forEachString`.
-   * @param {string} keySoFar - as we recurse down, build up a string of the key separated with dots.
-   * @param {StringMapNode} map
-   * @param {function(key:string, StringObject)} func
-   */
-  const forEachStringImplementation = ( keySoFar, map, func ) => {
-    for ( const key in map ) {
-      if ( map.hasOwnProperty( key ) ) {
-        const nextKey = keySoFar ? `${keySoFar}.${key}` : key; // don't start with period, assumes '' is falsey
-        const stringObject = map[ key ];
+    forEachString( map, func, keySoFar = '' ) {
+      for ( const key in map ) {
+        if ( map.hasOwnProperty( key ) ) {
+          const nextKey = keySoFar ? `${keySoFar}.${key}` : key; // don't start with period, assumes '' is falsey
+          const stringObject = map[ key ];
 
-        // no need to support arrays in the string map, for example stringObject.history in locale specific files.
-        if ( Array.isArray( stringObject ) ) {
-          return;
-        }
-        if ( stringObject.value ) {
-          func( nextKey, stringObject );
-        }
+          // no need to support arrays in the string map, for example stringObject.history in locale specific files.
+          if ( Array.isArray( stringObject ) ) {
+            return;
+          }
+          if ( stringObject.value ) {
+            func( nextKey, stringObject );
+          }
 
-        // recurse to the next level since if it wasn't the `value` key
-        key !== 'value' && forEachStringImplementation( nextKey, stringObject, func );
+          // recurse to the next level since if it wasn't the `value` key
+          key !== 'value' && ChipperStringUtils.forEachString( stringObject, func, nextKey );
+        }
       }
     }
   };

[zepumph]

Agreed! Implemented above, and I updated the documentation too to use the typeDef we want to.

[samreid]

Everything else seems good, thanks for your work on this! We will continue in the related issues mentioned above. Closing.