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.
http://scenerystack.org/
MIT License
9 stars 6 forks source link

State doesn't capture which preferences tab you're on #877

Closed KatieWoe closed 2 years ago

KatieWoe commented 2 years ago

Test device Dell Operating System Win 11 Browser Firefox Problem description For https://github.com/phetsims/qa/issues/844 and https://github.com/phetsims/qa/issues/845 Opening the preferences dialog is captured in state, but which tab you're on in that dialog isn't.

Visuals tabnotstate

arouinfar commented 2 years ago

In general, it seems like dialogs are stateful. I verified this in fourier-making-waves which has many types of dialogs: preferences, keyboard help, about, NumberKeypad, three different info dialogs, and the game reward dialog.

In general, Preferences are not stateful, and we tell clients this in the PhET-iO Guide. I think I would extend that to the Preference dialog too. Since we don't restore the selected tab, an open dialog doesn't seem particularly informative, anyway.

I wouldn't change the statefulness of other dialogs, though. This includes the Keyboard Help dialog, which is preferences adjacent. I don't think having this dialog open reveals anything about a user's needs or personal settings, so I don't think it shares the same privacy concerns that lead us to keep most preferences out of state.

Reassigning to @samreid @zepumph.

arouinfar commented 2 years ago

While perhaps odd, I don't think the current behavior is problematic enough to block publication.

kathy-phet commented 2 years ago

I agree, not blocking, but if it is easy to remove the Preferences dialog open/close from state, let's do it. @zepumph is this easy?

samreid commented 2 years ago

I'm not sure how difficult it is to unstateful the preferences dialog. But here is a patch of one line of code that correctly transmits the selected tab:

Index: js/preferences/PreferencesDialog.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/preferences/PreferencesDialog.ts b/js/preferences/PreferencesDialog.ts
--- a/js/preferences/PreferencesDialog.ts   (revision 87af221810b8074cbd8cb1e0d753902189bdaae6)
+++ b/js/preferences/PreferencesDialog.ts   (date 1667005492884)
@@ -93,7 +93,7 @@
     const selectedTabProperty = new EnumerationProperty( PreferencesType.OVERVIEW, {
       validValues: supportedTabs,
       tandem: options.tandem.createTandem( 'selectedTabProperty' ),
-      phetioState: false,
+      phetioState: true,
       phetioReadOnly: true
     } );

UPDATE: I tried putting phetioState: false in the PreferencesDialogCapsule and that didn't seem to do anything.

arouinfar commented 2 years ago

It sounds like there isn't a straightforward way to make the Preferences dialog unstateful. From a UX perspective, restoring the selected tab seems slightly preferable to defaulting to the Overview tab. However, it's a bit at odds with the privacy concerns we cite when explaining why many Preferences are unstateful.

Regardless, I think this is a fairly minor issue. We inform clients that Preferences are generally unstateful, with limited exceptions. We can amend the documentation to specifically mention the dialog itself.

Here's a snippet from the PhET-iO Guide (Lines 263-269):

Some preference selections are saved into the simulation state, and persist in the save and launch process of the
Standard PhET-iO Wrapper. These include `audioEnabledProperty`, `colorProfileProperty`, and all settings on the
Simulation tab.

Many preference selections - generally those that are learner-specific modality preferences - are not saved in the
simulation state. In this way, if the simulation state is saved and used in a general context, the learner-specific
modality preferences are not inadvertently exposed in a classroom setting. This protects learner privacy.

We could add a sentence to the 2nd paragraph (emphasis added):

Many preference selections - generally those that are learner-specific modality preferences - are not saved in the simulation state. Additionally, the state of the Preferences dialog (e.g. open to a particular tab) is not saved in state. In this way, if the simulation state is saved and used in a general context, the learner-specific modality preferences are not inadvertently exposed in a classroom setting. This protects learner privacy.

zepumph commented 2 years ago

@samreid, @arouinfar and I spoke about this and decided that stateful Preferences dialog components are acceptable, and orthogonal to stateful preferences themselves. Closing