Closing a dialog in the state wrapper leads to a dimmed downstream sim

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

Closing a dialog in the state wrapper leads to a dimmed downstream sim #849

Closed samreid closed 2 years ago

samreid commented 2 years ago

Discovered in https://github.com/phetsims/circuit-construction-kit-common/issues/867. After closing the preferences dialog in the state wrapper, the downstream Preferences Dialog closes, but the semi-transparent overlay is still there.

@jessegreenberg can you please take a look?

jessegreenberg commented 2 years ago

I just confirmed this is a problem for all Dialogs. I spent a few minutes investigating but I am not really sure how to fix this. Maybe the modalNodeStack needs to be stateful? Assigning to the PhET-iO team.

marlitas commented 2 years ago

@samreid and I did some exploration.

We learned:

dispose is called in the downstream sim, but not in the upstream
hidePopup is called in the upstream, but not in the downstream

Questions:

In the upstream sim, when the preferences dialog is clicked out of/ exited, do we want to dispose or keep in memory?

zepumph commented 2 years ago

This was a regression caused by https://github.com/phetsims/axon/issues/409. @samreid and I were able to work it out. Basically, on dispose of a Dialog, we are calling isShowingProperty.value = false, which the calles hidePopup via a listener. This was no longer happening because of this code:

https://github.com/phetsims/axon/blob/b5c62d90476a1e9c2684bbb71ffa6183b5c775a5/js/ReadOnlyProperty.ts#L230-L236

Because this Property is instrumented. The solution is to not consider dynamic element clearing (occurring on onBeforeSetState as part of this.isSettingPhetioStateProperty: true territory. Commits above. @marlitas would you like to spot check? Feel free to close.

marlitas commented 2 years ago

Spot checked the commits, and all looks good to me. I'm not sure I'm fully following the order of things that caused the bug, but the explanation and fix seems reasonable. Closing!