phetsims / joist

Joist is the main framework for PhET Interactive Simulations. Joist creates and displays the simulation content, home screen, navigation bar, About dialog, enables switching between tabs, and other framework-related features.
MIT License
8 stars 6 forks source link

Assistive Description Prototype #941

Open jonathanolson opened 10 months ago

pixelzoom commented 9 months ago

For this issue, density-main.ts has:

  console.log( DensityDescriptionStrings_en.toString() );
  console.log( DensityDescriptionLogic.toString() );

Recommended to change to:

  phet.log && phet.log( DensityDescriptionStrings_en.toString() );
  phet.log && phet.log( DensityDescriptionLogic.toString() );
zepumph commented 8 months ago

Agreed! I just hit this as work begins on bringing buoyancy to the finish line. phet.log is used in the above commit.

zepumph commented 7 months ago

Hello! A couple of items about this issue that I wanted to post here:

Yesterday @AgustinVallejo and I were trying to reproduce https://github.com/phetsims/density-buoyancy-common/issues/97 and we found that the description plugin failed when running density with ?screens=2,3.

I am unsure about the current progress here, but I wanted to note that we may want to try to republish density over in https://github.com/phetsims/density/issues/190. In my opinion.

samreid commented 4 months ago

This density prototype is also preventing setting the designed focus order in the screen view as part of https://github.com/phetsims/density-buoyancy-common/issues/121. How should we proceed?

samreid commented 4 months ago

I commented it out for now, see the commit above.

zepumph commented 4 months ago

I believe the best way forward for the high priority part of this issue is to comment out all the DensityDescription... calls in density-main.ts. Or at least only run them with the presence of a query parameter.

jessegreenberg commented 2 weeks ago

I have scratch notes for potential next steps from previous meetings. Putting them here for better review:

terracoda commented 2 weeks ago

@terracoda 's thoughts:

Identify how this may work for Voicing

  • Yes, agreed, where can designers identify what descriptions might need to change for Voicing (e.g., screen summary), or what descriptions might not be relevant to Voicing (e.g., headings and verbose help text).

When creating a heading level, wrap it and its children content in a

. (Note, this does not seem necessary according to TS)

  • To clarify, I don't think we need the <section> element for the end design in the sim, but the section element might be very helpful for the tool. @jessegreenberg, I would like to know more about what you want to do with the <section> element.

Consider changing the tool to work from an editor instead of from within the web. (Needs designer feedback).

  • @terracoda thinks this could be very helpful. The current web editor does not have much in terms of functionality.

Should we use methods or data fields for strings that are static?

  • not sure what we are using now, but if data fields are more straight forward to code, it might be useful to use a data field to enter all static state descriptions, and then that would separate the static from the more dynamic elements in the code, BUT/AND a designer can still see the descriptions all together in the A11y View on the right.

It wasn’t clear that pdomCreateContextResponseAlert was only available for sliders/AccessibleValueHandler. How will users of the tool understand what is available on a particular Node?

  • Agreed. This is problem. It would be better if we could find a a way for a designer to a PhET Type, then give it an accessible name, some help text, and then as step 2 make a map for it if it is a slider.

Assess how we can teach team members JavaScript? Resources on fundamentals?

  • Some of us already know some basics to coding, can use an editor, are even familiar with the command line and git commands. However, if I am not doing it everyday, it will be like re-learning the tool some for each new sim. We need to have learnable flow that sticks. If the flow is disjointed, it will be more difficult to make it stick.
pixelzoom commented 1 week ago

After discussion in Slack#description-tooling, a "Description Tooling & pH Scale Design Check-in" meeting has been scheduled for Wed 10/2/24 @ 11AM MT. The feedback in this issue would be good for attendees to skim before that meeting.

During Slack#galileopards team standup meetings, @arouinfar, @jbphet, and @pixelzoom all had questions/concerns related to the description prototype. We realize that it's a work in progress, but we thought this might be a good time to communicate those questions/concerns. Since this is a prototype (we think?) we don't expect there to be answers now to all of these questions/concerns.

@arouinfar has her own list which she'll add here.

In Slack#DM to @arouinfar and @pixelzoom, @jbphet offered this feedback:

Hey there, you mentioned during standup that I should consider taking a look at the "logic" file and assessing it based on my experiences with creating description for Greenhouse Effect. I took a few minutes to examine it, and here are my initial thoughts. You're welcome to share them with the design team if you believe it would help.

  • There does seem to be some potentially duplicated information in here, like constants, enums, and PDOM order stuff. Maybe it's in a temporary state as details are refined, but if not, steps should be taken to fix this up.
  • As Amy mentioned, this seems to be pretty sophisticated code, so I agree that it would be a big ask to have designers write this.
  • The file is currently 333 lines long, and it looks like a very small fraction of the functionality that was implemented for Greenhouse Effect. If all the description functionality is intended to go here, it's going to be huge, at least for some sims.
  • I don't understand how this relates to the "description tool". Perhaps that understanding would come through training and/or documentation, but having that information readily accessible - perhaps linked from the header comment in this file - would be important for maintenance and for people who want to use this file as an example in the future.

Here's my (numbered) list of questions/concerns. This is based on my assigned task of reviewing for scalability and impact on the sim code base. I have mainly looked at the files that the description plugin creates/uses, not the implementation of the plugin (DescriptionRegistry.ts, etc.)

  1. Is this approach more scalable than what was done previously for sims like Greenhouse Effect? How much more scalable? What are the scalability pain points? It's nice that very few changes have been needed to sim-specific code. But looking at description-logic.js, it’s getting big for ph-scale-basics. And as JB said above “it looks like a very small fraction of the functionality that was implemented for Greenhouse Effect. If all the description functionality is intended to go here, it's going to be huge, at least for some sims.”

  2. description-logic.js contains sophisticated code, which seems like a big ask of description designers.

  3. description-logic.js uses hard-coded phetioIDs. Is this creating the same pain point as we encountered with overrides.js for PhET-iO metadata? For overrides.js, we learned that hard-coding phetioIDs, in a file not related to those ids, creates an expensive development burden. We eventually decided to move overrides.js into the code, and that has been a big improvement (where it's been done), but a big task that has not been completed.

  4. If we're using phetioIDs... How will this work for a suite of sims? Elements that are common to sims in the suite will have different phetioID because they are in different sims. Or for different instances of the same class? Elements that are created via the same class constructor will have different phetioIDs.

  5. Will description-logic.js be moved into sim code after description is designed?

  6. How will description-logic.js and description-strings.js be shared across a suite of sims? For example... ph-scale cannot have ph-scale-basics as a depenendency. So how will what is being done for ph-scale-basics-description-logic.js and ph-scale-basics-description-strings.js reused for ph-scale? Presumably the contents of those files should eventually be moved to in ph-scale. In the case of strings, that needs to happen before ph-scale-basics is published and made translatable, because relocating strings after they've been translated is problematic.

  7. Is description-logic.js responsible for setting pdomOrder, or is the sim? All pdomOrder, or just ScreenView level? This problem was encountered for ph-scale and is described in https://github.com/phetsims/ph-scale/issues/291#issuecomment-2371782114.

  8. description-logic.js uses nameSpace, which PhET has considered removing. Is it wise to use it here? See https://github.com/phetsims/phet-core/issues/100

  9. description-logic.js contains duplication of stuff that’s in the sim. For example see const solutes in ph-scale-basics-description-logic.js. If sim code changes, this would also need to change. And this creates a maintenance pain point.

  10. There is logic in ph-scale-basics-description-strings_en.js. This seems like it should live in description-logic.js, and strings should be in a JSON file. How are strings translated if the strings file also contains logic?

  11. When changes are made to ph-scale-basics-description-strings_en.js, how does that impact description-strings for other locales? E.g. ph-scale-basics-description-strings_es.js. And how is logic in those strings files handled, is it duplicated in each locale file?

  12. Why is description specified in .js files instead of .ts? Should they live under {repo}/js/? Will they be linted?

  13. Where should the strings for accessibleName and helpText attributes live? It looks like they can be specified via the description plugin, but then they end up in description-strings.js, where they can’t (currently) be translated with Rosetta. And in some sims, these strings are coming from strings.json, where they can be translated with Rosetta. Is it a problem that accessibleName and helpText might be in 2 different string files?

  14. What additional tooling will be needed with this approach? Something to prevent phetioIDs from becoming out of sync? Other code from becoming out of sync? How do description-logic.js and description-strings_en.js get bundled into a published sim? Etc.

terracoda commented 6 days ago

Some things mentioned during PhET-iO meeting:

@pixelzoom and @zepumph, please feel free to edit. If I didn't capture the concerns appropriately.

terracoda commented 6 days ago

I share many of the same concerns in 1, 2, 3, 5, 10, 13, 14.

arouinfar commented 6 days ago

@arouinfar has her own list which she'll add here.

I agree with all of the concerns listed in this issue thus far, so I will try not to repeat things already said above.

  1. One motivation for the description tool was to simplify the process of creating, implementing, and iterating on description. Right now, the process does not feel any more efficient than the “old way”. Personally, I find working with the tool more enjoyable, but it is by no means a cost-saving tool in the way it is currently being used.
  2. Designers are not developers, but we have done nearly all of the description implementation ourselves (with lots of help with JG when we get stuck). We have been pairing in multiple ~2-hour sessions per week to implement the description using the tool. Writing the code ourselves doesn’t make sense long term – 3 designers pairing for 2 hours to do something a single developer could do in 30 minutes is not economical. That said, I think designers are learning important skills that will better equip us to iterate on description, assuming there are no major changes to how the description tool functions. (But can we trust that assumption?)
  3. Code written by designers may function, but is unlikely to conform to best practices, and will need developers to review and clean up.
    • For example, the dynamic-state-of-sim portion of the Scene Summary describes the current state of the simulation, and relies on several conditions in the sim (beaker is empty, beaker contains only water, pH probe is not in beaker, to name a few). Designers implemented the LOGIC for these statements in the STRINGS file. This is probably not the way it should be done.
  4. Designers still have a maximalist mindset, which adds a lot of complexity to the description design, resulting in higher costs on many fronts – implementation, QA, maintainability. PhET is the best at what we do, both in terms of visual and inclusive design. It’s hard for us to create something that is “good enough”, but perfection is the enemy of progress.
    • For example, we have plans to describe the diluted color of the solution in the beaker. I foresee this being very complicated to implement for a few reasons: we have no access to the current color of the solution, the color interpolation algorithm is complicated, and color perception is even more complicated. My intuition is that describing the color of the stock solution in the dropper plus general “lighter/darker” responsive descriptions are sufficient to serve the learning goals regarding dilution and color change.
pixelzoom commented 6 days ago

@arouinfar said:

... For example, we have plans to describe the diluted color of the solution in the beaker. I foresee this being very complicated to implement for a few reasons:

This would be an even more intractable problem if the colors were PhET-iO instrumented, and therefore could be changed. (The solution colors are currently not customizable, but could easily be customizable in the future, if a client asks for it.)

There is a general tension here between description and things that can be changed via PhET-iO. We'll need to be careful about describing things that may be customized in ways that makes the description incorrect.

jessegreenberg commented 5 days ago

I created some branches in ph-scale and ph-scale-basics to try a different way that doesn't use PhET-iO or a 'plug-in' type system, see https://github.com/phetsims/ph-scale-basics/issues/59.