PhET-iO instrumentation of Preference dialog seems "off"

[pixelzoom]

Here's the current structure of the Preferences dialog's tandem hierarchy, as shown in Calculus Grapher:

Why are preferencesPanels and preferencesTabs siblings? Shouldn't each panel be a child of it's associated tab?

Assigning to @samreid and @arouinfar to discuss. Blocks publication because changing this structure will require migration rules.

[pixelzoom]

Looking at the implementation of PreferencesDialog by @jessegreenberg , it looks like the tabs are considered to be just the labels that you click on, and a tab has no content. That's contrary to every UI "tab" component that I've every encountered. But maybe it's OK.

Adding @jessegreenberg to discuss with @samreid and @arouinfar. I don't have a strong feeling about whether you decide to do anything here, just pointing out that the structure of the current Studio tree doesn't match my conceptual understanding of a tabbed dialog.

[jessegreenberg]

Right, I remember pondering this. I wanted a class for the "tab" and a class for the "contents" and ended up with PreferencesTab and PreferencesPanel. I may have been influenced by the ARIA roles which call these tab and tabpanel.

[pixelzoom]

The relationship between aria roles tab and tabpanel is not reflected in the structure of the Studio tree, and this is exactly the problem I'm reporting.

According to the links you provided above, each tab has an associated tabpanel. From https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/tabpanel_role:

A tabpanel is part of a tab interface, a common user experience pattern in which a group of visual tabs allow for quickly switching between multiple layered views. Each tab is defined as such with the tab role, and these tabs are contained within an element with the tablist role. The tablist is often visually positioned above or to the side of a content area, containing the associated tabpanels. The tabpanel is the role of the container for each pane of content that is associated with a corresponding tab in the tab interface's tablist.

Therefore I expected each tab in the Studio tree to have a child tabpanel. Instead we have parallel tab and tabpanel subtrees, with no relationship between them.

[samreid]

The existing instrumentation mirrors the implementation detail:

    // the set of tabs you can click to activate a tab panel
    const preferencesTabs = new PreferencesTabs( supportedTabs, selectedTabProperty, {
      tandem: options.tandem.createTandem( 'preferencesTabs' )
    } );

    // the panels of content with UI components to select preferences, only one is displayed at a time
    const preferencesPanels = new PreferencesPanels( preferencesModel, supportedTabs, selectedTabProperty, preferencesTabs, {
      tandem: options.tandem.createTandem( 'preferencesPanels' )
    } );

Perhaps refactoring the main code should go hand-in-hand with changing the PhET-iO API? Should @jessegreenberg and I collaborate on that?

[samreid]

I believe we are free to decide and implement as we judge appropriate, but commenting on the argument from https://github.com/phetsims/joist/issues/907#issuecomment-1430365988, particularly:

Therefore I expected each tab in the Studio tree to have a child tabpanel. Instead we have parallel tab and tabpanel subtrees, with no relationship between them.

The example linked from https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/tabpanel_role is https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/tab_role#example, which shows this structure:

```js

First Tab Second Tab Third Tab

Content for the first panel

```

In the example, the tab content is not a child of the tab button--they are separate lists. But how difficult would it be for us to rewrite our data structures so each tab has a tab panel? If that is too expensive, should they be linked via LinkedElement?

[jessegreenberg]

Changing the scene graph to put a PreferencesPanel under a Preferences tab could make layout more difficult. We could try to change the implementation without changing the scene graph. I don't interpret https://github.com/phetsims/joist/issues/907#issuecomment-1430365988 as a description of tab and tabpanel hierarchy, its describing tab and tablist and that tabpanel is a container for its content. So I don't think the implementation needs to change unless we hear from design team or clients that it is confusing.

[pixelzoom]

As I said above:

I don't have a strong feeling about whether you decide to do anything here, just pointing out that the structure of the current Studio tree doesn't match my conceptual understanding of a tabbed dialog.

So unassigning myself.

[arouinfar]

Why are preferencesPanels and preferencesTabs siblings? Shouldn't each panel be a child of it's associated tab?

I remember asking this same question during the review of Friction, and I am satisfied with @jessegreenberg's explanation. I don't see any real benefit to changing the PhET-iO API, especially if it has the potential to disrupt Voicing or Description.

Generally, preferencesPanels should be empty except for simulationPreferencesPanel. We decided that clients should not be able to hide bits and pieces of inclusive features (such as hiding the Rate slider for Voicing). We provide access to the content in the Simulation Tab because it has instructional implications and relates to the specific learning goals of a simulation. It's easy to find the Simulation Tab contents using the autoselect feature, so I am less worried about the exact tree structure.

However, I notice that preferencesPanels.localizationPreferencesPanel.visibleProperty appears in the tree. Why did we instrument this? The fact that it's phetioReadOnly: false is also problematic because you can create situations where the selected tab is something other than Localization, but the locale selections are visible:

[samreid]

This patch uninstruments the localizationPreferencesPanel.visibleProperty:

Subject: [PATCH] Factor out includeExtremeElements, see https://github.com/phetsims/circuit-construction-kit-common/issues/900
---
Index: js/preferences/LocalizationPreferencesPanel.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/preferences/LocalizationPreferencesPanel.ts b/js/preferences/LocalizationPreferencesPanel.ts
--- a/js/preferences/LocalizationPreferencesPanel.ts    (revision 2eb0988aea1ac0250bd6bcf2ef9ed76f05a52a60)
+++ b/js/preferences/LocalizationPreferencesPanel.ts    (date 1676829309412)
@@ -40,7 +40,8 @@
                       providedOptions: LocalizationPreferencesPanelOptions ) {

     const options = optionize<LocalizationPreferencesPanelOptions, SelfOptions, PreferencesPanelOptions>()( {
-      labelContent: localizationTitleStringProperty
+      labelContent: localizationTitleStringProperty,
+      phetioVisiblePropertyInstrumented: false
     }, providedOptions );

     super( PreferencesType.LOCALIZATION, selectedTabProperty, tabVisibleProperty, options );

However, regenerating the PhET-iO APIs generates no changes, so I'm confused and could use help. Also, after uninstrumenting that, localizationPreferencesPanel has no important details in studio, and therefore we should discuss if it could be uninstrumented altogether.

Also, keep in mind this instrumentation would require migration rules, since some sims have been published with that visibleProperty.

[zepumph]

Looks like we should be adding locales=* to our api generation script. Anything else important for our general api snapshot?

[samreid]

Thanks, I added locales=* to API generation, and uninstrumented localizationPreferencesPanel.visibleProperty using the patch above. Remaining for this issue:

• [x] localizationPreferencesPanel has no important details in studio, and therefore we should discuss if it could be uninstrumented altogether.
• [x] Anything else important for our general api snapshot?

[zepumph]

• [x] This morning I committed https://github.com/phetsims/phet-io-sim-specific/commit/cca2be9170126cedf7d7ecf139813476ac951ca7 because CT was failing with designed API changes. It looks like when a new translation language is added by a trusted translator, that breaks our the API. I'm not sure how to handle this, because I think it makes sense for localeProperty.validValues to be in the API (basically as "supported languages"). What do you think @samreid?

[samreid]

Today, CT says BLL has a problem:

beersLawLab.general.model.localeProperty._data.initialState differs.
Expected:
{"units":null,"validValues":["af","am","ar","ar_MA","ar_SA","ay","az","be","bg","bi","bn","bo","bs","ca","cs","cy","da","de","el","en","en_GB","es","es_ES","es_MX","es_PE","et","eu","fa","fi","fo","fr","ga","gl","gu","ha","hi","hr","ht","hu","hy","ig","in","is","it","iw","ja","ka","ki","kk","kn","ko","ku","ku_TR","lg","lo","lt","lv","mi","mk","ml","mn","mr","ms","mt","my","nb","ne","nl","nn","ny","om","or","pl","pt","pt_BR","ro","ru","rw","sd","sh","si","sk","sl","sq","sr","st","sv","sw","ta","te","tg","th","tk","tl","tn","tr","tw","uk","ur","uz","vi","wo","xh","yo","zh_CN","zh_HK","zh_TW"],"value":"en"}
actual:
{"value":"en","validValues":["en"],"units":null}

Increasing priority.

[samreid]

I added locales=* to the phetioCompareAPI test so it should match the generated API. This will probably get us over the next hurdle, but I suspect the next problem is that a translator adding a language will change the API and cause a CT error for the phetioDesigned test (because there are new locales not listed in the API file). How will we deal with that?

UPDATE: Brainstorming... Can we put phetioDesigned: false just on that subtree? I don't know how sophisticated our phetioDesigned toggling on and off is.

[zepumph]

I was SOO excited about the idea of "remarking" a sub directory as phetioDesigned: false, but the comparison logic is not set up to do this. It is because phetioDesigned is a 2 value boolean, it isn't like pickable where you can specifically opt out. Dang! Want to have a brainstorming session about this soon?

[samreid]

Would something like propagateDynamicFlagsToDescendants be able to handle it?

[zepumph]

I think it is more like updating phetioDesigned as a 3 value flag:

• true: opt in to everything underneath me
• false: opt out of everything underneath me
• null: not designed but I can take my value according to any ancestors with the values above.

That said, I'm not convinced this is the way to solve the problem. It is just a workaround perhaps. Should we focus on our particular case instead? I don't know. Let's discuss it tomorrow.

[zepumph]

RE: https://github.com/phetsims/joist/issues/907#issuecomment-1441791110, YES, let's uninstrument localizationPreferencesPanel

[zepumph]

• [x] We also want to "uninstrument" preferencesPanels so that it is just a name in the tandem instead of making it an actual phetioID.

We are discussing this as part of https://github.com/phetsims/phet-io/issues/1913 BTW

[zepumph]

RE https://github.com/phetsims/joist/issues/907#issuecomment-1444297263: We updated the api comparison algorithm to ignore a few specific cases that were giving us this trouble. @samreid and I feel good about this change because we wrote it to support allowing extra locales, but still erroring if you took away support for a locale.

All that is left here is https://github.com/phetsims/joist/issues/907#issuecomment-1452582501 and localizationPreferencesPanel uninstrumentation. I'll take care of it.

[zepumph]

I uninstrumented the preferencesPanels and individual panels above. This means that you only get preferencesPanels if you have simulation-specific content.

Calculus Grapher:

Friction:

@pixelzoom, I believe we are ready to close. Have all items of this issue been addressed?

[pixelzoom]

Looks like an improvement to me. But as I said in https://github.com/phetsims/joist/issues/907#issuecomment-1430267748:

I don't have a strong feeling about whether you decide to do anything here, just pointing out that the structure of the current Studio tree doesn't match my conceptual understanding of a tabbed dialog.

So you'll need to find someone else to sign off and close.

[zepumph]

Makes sense thanks. @arouinfar ready for a review.

[arouinfar]

Looks good to me, thanks @zepumph. And thanks to @pixelzoom for starting the conversation. I think we made a lot of great changes here, closing.