phetsims / center-and-variability

"Center and Variability" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
1 stars 2 forks source link

Assertion error: "The valueProperty of the focusedSoccerBall should not be null." #574

Closed Nancy-Salpepi closed 1 year ago

Nancy-Salpepi commented 1 year ago

Test device MacBook Air M1 chip

Operating System 14.0

Browser Safari 17

Problem description While verifying an issue on main, the sim froze when I pressed the Reset All button with the mouse while a soccer ball had focus. I don't see this issue in https://github.com/phetsims/qa/issues/985.

Steps to reproduce

  1. On the Median Screen, Press the Kick 5 button
  2. Tab to and grab a soccer ball
  3. While the ball has focus, press Reset All with the mouse

Visuals

Screenshot 2023-10-03 at 9 55 36 AM Screenshot 2023-10-03 at 9 55 54 AM
Nancy-Salpepi commented 1 year ago

This error also appears on main if I press Reset All with the mouse while a card is grabbed with the keyboard.

marlitas commented 1 year ago

Oooh good catch! I'll take a look.

marlitas commented 1 year ago

This was caused by a commit fixing a different bug in: https://github.com/phetsims/center-and-variability/issues/571.

This situation should now be addressed. We have to reset the focusedSoccerBallProperty before resetting the sceneModels. I made sure to move other keyboard focus related Properties before the sceneModel reset as well since it makes sense that we want to reset our general Properties before triggering reset logic in the sceneModels that rely on those Properties.

Over to @samreid and @matthew-blackman for review.

@Nancy-Salpepi this should also be ready to check on main.

matthew-blackman commented 1 year ago

Commit looks good - nice change @marlitas. Moving the scene model reset to the end makes thing very clear.

Up to you if you want to close this issue or keep it open while #571 is still open.

marlitas commented 1 year ago

Thanks for the review @matthew-blackman! I think this is ready to close.