Components: ToolsPanel: fix deregister/register on type

[ellatrix]

What?

When logging all the callbacks in ToolsPanel, this is what happens on typing:

Panel items seem to be deregistered, then registered again.

Diving deeper, it seems to be caused by hasValueCallback triggering an effect. It seems a the dependency hasValue has been added in #45028, which is a new function on every render.

Why?

Note the re-rendering of the wrapping Panel:

Before	After

How?

Testing Instructions

Testing Instructions for Keyboard

Screenshots or screencast

[andrewserong]

This is very promising, thanks for putting up the PR!

I ran into one issue while manually testing out the failing unit test for should render optional panel item when value is updated externally and panel has an ID. The behaviour that's being tested was introduced in #47864, and it appears the expectation in that PR is that when copying and pasting styles from one block to another, that the ToolsPanel state will be updated to reflect the change that happened from outside of the ToolsPanel.

Here's a screengrab of how it looks with this PR applied (if testing locally, you'll need to run a dev environment on localhost for pasting styles to work):

2023-12-05.11.02.01.mp4

In the above:

• We start out with a Heading block with a few Typography rules set (font-size, appearance, letter case), and go to copy the styles
• We then paste the styles on another Heading block, and the paste works visually, however the ToolsPanel state does not update to reflect the new changes, i.e. font-size toggle isn't active for the right font size, and appearance and letter case controls are not visible, nor marked as having a value within the dropdown (you'd need to click out of the block and back into it to see the reflected state change)

So, it seems that perhaps #47864 was implicitly relying on the values in this PR being changed too frequently? I'm not too sure how, but I imagine we'll need to adjust some other logic to get that working again with the changes to the dependency array here.

I'll just ping @aaronrobertshaw, @glendaviesnz, and @ciampo for visibility since I see they were involved in #47864 🙂

[youknowriad]

Maybe add some comments to explain why we're disable the eslint rule.

This is the perfect use-case for the useEvent hook they proposed in React. not sure what happened to that proposal.

[ellatrix]

Wasn't aware of useEvent, but we have lots of cases in rich text and elsewhere too where we have to put stuff in a ref to access it in callbacks later.

[ellatrix]

@andrewserong I just tried copy pasting styles, and the typography/size button doesn't become active in trunk either. And in both trunk and this PR the letter case button seems to appear. 🤔

[ellatrix]

@andrewserong Your intuition was good here, #47864 seems relevant. The component was somehow relying on excessive re-renders to get a different value for menuItems?.[ menuGroup ]?.[ label ]. I remove the condition around flagItemCustomization and all tests seem to pass now, but I don't really understand what is happening much because I'm not familiar with these components.

[github-actions]

Flaky tests detected in c601a63.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7111030661
📝 Reported issues:

• [Flaky Test] should display the "Manage Fonts" icon #56776 in /test/e2e/specs/site-editor/font-library.spec.js

[andrewserong]

Nice work getting to the bottom of this @ellatrix! This is all testing well for me:

✅ Default and optional items are behaving correctly within the ToolsPanel and its menu
✅ Copy / pasting styles from one block to one or more other blocks behaves as on trunk
✅ Smoke tested many ToolsPanel instances (color, typography, dimensions, border) at the block level and within global styles, and all appears to be behaving as on trunk

The changes to when flagItemCustomization is called sound good to me, and appears to match the intended behaviour in #47864. The logic now means that whenever a new value is set, we update the item customization, which seems simpler, too 👍

I think this means we can also update the comment in the following line to remove the words when the panel ID is null:

gutenberg/packages/components/src/tools-panel/tools-panel-item/hook.ts

Line 131 in 0246a6b

// 2. For optional controls, when the panel ID is `null`, it allows the

But other than that, this LGTM! ✨