useToolsPanel: calculate derived state in reducer to prevent too many renders

[jsnajdr]

Second attempt at implementing #65494, this time with a very different approach. Instead of calculating the derived state (menuItems, areAllOptionalControlsHidden) in effects, layout- or normal, let's calculate all the updated state at once, in a reducer. If you register or unregister a panel, the new menuItems value is calculated instantly, within one render, and is immediately available.

This should prevent painting an unfinished intermediate state, because there is no longer an intermediate state. And also avoid untractable infinite effect loops that plagued #65494, because the effects are eliminated.

How to test:
Use test steps from #65494 to verify that the unwanted visual glitches in Global Styles editor are gone.

Test also all other usages of ToolsPanel especially in the post editor when switching blocks.

[github-actions]

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: jsnajdr <[email protected]>
Co-authored-by: youknowriad <[email protected]>
Co-authored-by: ciampo <[email protected]>
Co-authored-by: aaronrobertshaw <[email protected]>
Co-authored-by: andrewserong <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[youknowriad]

Looks good in my tests. Would appreciate a second review.

[jsnajdr]

I'm worried whether the isResetting state and ref still works. The code introduced in #34157 and similar PRs.

[ciampo]

Also looking good on my machine.

I'm worried whether the isResetting state and ref still works. The code introduced in #34157 and similar PRs.

I followed testing instructions in #34157 and things seem to work as expected.

If we wanted to be more conservative, we could wait for @aaronrobertshaw and/or @andrewserong to have confirmation that the resetting logic still works as expected. But I'm tempted to merge it and watch out for any reports in the upcoming days.

[jsnajdr]

But I'm tempted to merge it and watch out for any reports in the upcoming days.

Well that's what we did on Friday with #65494 and it ended badly 🙂 Let's wait for at least one "independent" review, from someone who actually worked on the code.

[aaronrobertshaw]

to have confirmation that the resetting logic still works as expected

Thanks for the ping 👍

I've given this some testing focused on the reset behaviour. It's looking good to me.

I did find a couple of bugs for different panels in the editor but they also exist on trunk (e.g. Image block's resolution control). I'll create separate issues for those as they are unrelated to the ToolsPanel itself.

✅ Component unit tests pass
✅ Global styles panels in Site Editor behave as expected
✅ Block inspector panels function as per trunk in both editors
✅ Individual control resets and reset all functionality looks to be working fine
✅ Ad hoc controls injected to ToolsPanels e.g. Cover overlay work as before
✅ Storybook examples that conditionally render controls or make them default, work too

🚢

[andrewserong]

Ah, Aaron beat me to it! 🙇 This is testing well for me, too, across the range of block supports used by blocks like Group, Cover, Heading, Paragraph etc, tested in the post and site editor and in global styles. I also really like how this reads now using a reducer, instead 👍

LGTM! 🚀

[jsnajdr]

Thanks everyone for your testing and reviews 🚀

[ciampo]

I guess this should feels like a nice fix to backport to the next release, I'll add the label

[youknowriad]

I guess you anticipated this but the performance regression is back here.

[github-actions]

I just cherry-picked this PR to the wp/6.7 branch to get it included in the next release: b26e274

[jsnajdr]

Yes, the focus performance regression is also present here, just like in #65494:

It's because the ToolsPanel is rendered right away, during the focus event handler, while previously it was fully rendered only after a cascade of effects and state updates.

I believe this is a deficiency of the measuring method, i.e., measuring the execution of the focus handler. There shouldn't be any user-visible change. Previously the work was postponed more or less after the next animation frame, outside the focus handler, and now it's done all at once.

But yes, maybe we could update the tools panel in some kind of "low priority" update, the kind that useDeferredValue offers. That way the rendered blocks and their selection are updated immediately, high priority, and sidebars are updated later. Maybe @ellatrix has some thoughs on this.