phetsims / friction

"Friction" is an educational simulation in HTML5, by PhET Interactive Simulations.
http://phet.colorado.edu/en/simulation/friction
GNU General Public License v3.0
4 stars 6 forks source link

Dialog state behavior #324

Closed KatieWoe closed 1 year ago

KatieWoe commented 1 year ago

Device Dell OS Win 11 Browser Chrome Problem Description For https://github.com/phetsims/qa/issues/886. From Slack:

Is the preference dialog or the keyboard nav dialog being open meant to be stateful? I thought it wasn't, but it seems to be in friction.

In addition to this, when the state is set, the open dialog sound plays, which seems wrong.

Steps to Reproduce

  1. Go to the state wrapper
  2. Have the state set rate set to every second or so (default works)
  3. Open the preferences dialog
  4. Observe the downstream sim and note the sound

Visuals

https://user-images.githubusercontent.com/41024075/214445596-cc91f1b9-a16b-4914-8b33-877fa745efea.MOV

zepumph commented 1 year ago

I'll take a look before tossing this one to our resident sound expert.

zepumph commented 1 year ago

@matthew-blackman and I were able to figure out the problem, but we aren't exactly sure what the best solution is. Basically the sound engine is doing a good job at silencing sound while the state engine is setting state, but in this case, the dialog is being cleared in the "before state set" hook, and that deletion closes dialog, and starts the close sound.

I would like to implicate @samreid to the next part of this conversation, and will likely discuss in about 5 minutes with him and @matthew-blackman.

zepumph commented 1 year ago

Fixed by commits over in https://github.com/phetsims/phet-io/issues/1906. Thanks @samreid and @matthew-blackman for helping to get this solved. Commits over in that repo for cherry pick.

zepumph commented 1 year ago

Wait. I also still hear another problem with the state engine where I hear the very first open sound. I'll keep investigating.

UPDATE: I heard it while testing an older version of the changes, I cannot reproduce with the new fixes in place. Nevermind!!

jessegreenberg commented 1 year ago

OK, 4 commits were cherry-picked from https://github.com/phetsims/phet-io/issues/1906.

image

I don't know how to test this one, but opened a dialog in studio in the 1.6 SHAs and pressed "test" and nothing was obviously broken.

Ready to verify.

Nancy-Salpepi commented 1 year ago

I no longer hear sounds in the downstream sim. However, @jessegreenberg I don't think Katie's original question was ever answered. Is having a dialog open supposed to be stateful? I just compared the behavior in Friction to Beer's Law 1.7.0-rc.2. and it is the same. If that is the intended behavior, then this issue can be closed.

jessegreenberg commented 1 year ago

I tried to find the answer in https://phet-dev.colorado.edu/html/friction/1.6.0-rc.1/phet-io/doc/guides/phet-io-guide.html#preferences-customization but couldn't find anything specific about the dialog state (just individual preferences). So I believe Dialog's isShowingProperty is meant to be stateful and this is correct.

@arouinfar can you please confirm?

arouinfar commented 1 year ago

That's correct @jessegreenberg. Every dialog's isShowingProperty is stateful, regardless of what type of dialog it is. For Preferences, specifically, selectedTabProperty is also stateful. I reviewed the Friction RC and it is behaving as intended, closing.