search

phetsims / scenery-phet

Reusable components based on Scenery that are specific to PhET simulations.
MIT License
8 stars 6 forks source link

GroupSortInteraction: Should isKeyboardFocusedProperty consider isGroupItemKeyboardGrabbedProperty #866

Open marlitas opened 1 month ago

marlitas commented 1 month ago

While working on https://github.com/phetsims/mean-share-and-balance/issues/343 @zepumph and I discovered that there is an order dependency issue between isKeyboardFocusedProperty and isGroupItemKeyboardGrabbedProperty.

It seems odd to me that we can have intermediary states where the interaction can have something grabbed via keyboard, but not be focused and vice-versa. It might not be worth too much investigation as I believe it does create some catch-22 scenarios, but I do believe it can create unexpected results down the line.

This should not block MSaB.

marlitas commented 1 month ago

FYI, that the assertion is now turned on again and working because of a fix for https://github.com/phetsims/mean-share-and-balance/issues/343. However, it doesn't address the weird relationship between isKeyboardFocusedProperty and isGroupItemKeyboardGrabbedProperty. It just happened to be that the commit addressed a dependency collision through a different unrelated scenario.

zepumph commented 1 month ago

This is hard for me to make progress on because I don't know how to reproduce the order dependency.

I was looking back at the git history of how we set the isKeyboardFocusedProperty and found https://github.com/phetsims/scenery-phet/commit/c4adcfc9b8a0ac2248f870c3ce7b8fa73c52a4bb. This isn't too helpful, but it shows that there is precedent for recognizing that the focus may change during the interaction, for better or worse, so it seems like depending on the more "modelly" isGroupItemKeyboardGrabbedProperty is best for logic determining other model entities, like the current selection.

Can you provide steps to reproduce the issue, or do you want to pair to continue on this issue?