Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
samreid opened this issue Aug 27, 2022 · 4 comments
Closed

Comments

@samreid
Copy link
Member

samreid commented Aug 27, 2022

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

image

@jessegreenberg can you please take a look?

@jessegreenberg
Copy link
Contributor

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.

@jessegreenberg jessegreenberg changed the title Closing the preferences dialog in the state wrapper leads to a dimmed downstream sim Closing a dialog in the state wrapper leads to a dimmed downstream sim Aug 29, 2022
@marlitas
Copy link
Contributor

@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
Copy link
Member

zepumph commented Aug 30, 2022

This was a regression caused by phetsims/axon#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
Copy link
Contributor

marlitas commented Sep 2, 2022

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!

@marlitas marlitas closed this as completed Sep 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants