DRAFT ToolsPanel: Trigger onDeselect/onSelect callbacks directly

[stokesman]

What?

Instead of using an effect hook to run some callbacks runs them from an event.

Why?

Simplification for maintainability. I noticed this while working on #60621.

How?

• Puts the onDeselect and onSelect callbacks into state accessible by toggleItem.
• Runs the callbacks from toggleItem which is the event handler for menu item clicks.
• Removes the effect hook that previously used for the above.

Investigation needed to be “ready for review”

Before this change there it seems there was some potential for triggering onSelect or onDeselect without actually interacting with the menu items. If that’s the case then this is a breaking change but there must not be tests covering such.

I’m walking myself through to see what needs verification. There’s two types of items—default and optional.

• Default items
  Their controls are always exposed so seemingly onSelect and onDeselect aren’t even applicable but do these callbacks run for when an item is "customized" and "reset"? If so then onSelect would be called when an item’s control changes its value from default. Also after Fix sticking “Reset” option in ToolsPanel #60621 onDeselect would be called when an item’s control changes its value back to default. Also controlled updates might do the same so those should be covered as well.
• Optional items
  Their controls aren’t exposed until either:
  • Selected via the menu item button
    In this case changing its value via its control isn’t possible so onSelect can’t be triggered other than through the menu item button. Once it is exposed, interaction with its control is not meant to cause the control to become hidden (and at least one e2e test catches this) so onDeselect also cannot be triggered except through the menu item button.
  • Item’s value is set via a controlled update
    In this case is onSelect being called? There’s a unit test for this but it doesn’t assert onSelect. Coverage should also exist for a controlled update restoring the default value I think.

It looks like I’ll need to review the unit tests and either verify the above behaviors are specified or revise/add tests to do so.

Testing Instructions

Testing Instructions for Keyboard

Screenshots or screencast