Conditional Duotone Toolbar Visibilty (With State)

[ajlende]

Description

A block supporting duotone needs to be able to control the visibility of the toolbar. Data needs to be passed from the supporting block (i.e. image block) to the block supports (i.e. duotone).

See #31373

First off, the feature should not be visible in the setup state

Secondly, the Duotone button should always be at the end. Right now it starts as the first button, then if you deselect the block and select it again, it moves to the end

This change allows the image block to prevent rendering of the duotone toolbar. This has the side-effect of solving the second issue since the point where the duotone toolbar is rendered comes after the rest of the block toolbar items, so the toolbar button is now always rendered at the end.

How has this been tested?

1. Create a cover block
2. See that there isn't a duotone button
3. Add a background image
4. See that there is a duotone button
5. Enable parallax/repeat backgrounds or clear media
6. See that duotone button is hidden again
7. Repeat 1-4 for image block instead

Screenshots

Types of changes

New feature / Bug fix

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• I've tested my changes with keyboard and screen readers.
• My code has proper inline documentation.
• I've included developer documentation if appropriate.
• I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

[talldan]

I don't think the block editor store is the right place, as it's more for general editor state. This is quite specific to an individual block feature.

I think this problem is bigger than just duotone as well. Looking at the cover block there's a bunch of features that probably shouldn't be active when the placeholder is visible, for example it's possible to put a border around the placeholder state.

So it might worth thinking more from a point of view of building a system that might work for various block features.

One option could be that a block has some way to indicate that it's in a placeholder/setup state. Block 'supports' and other features would have access to that state and can use it to determine whether their UI should be visible. It might be good tomake an issue to discuss what generic solution to the problem might look like.

[ajlende]

I don't think the block editor store is the right place, as it's more for general editor state. This is quite specific to an individual block feature.

@talldan But you like the idea of using a store as opposed to the block attributes, right?

One option could be that a block has some way to indicate that it's in a placeholder/setup state.

Just indicating the setup state isn't enough. In the case of the cover block, duotone should also be disabled when the background is repeated or fixed. And in the media & text case (#32984), it should be disabled for videos, but not images.

I can imagine a system where the block supports hooks have their own store for UI state. The margin/padding UI state could move there and fix #31839.

I'm not thrilled about using useEffect to sync the attributes with the store, but it's far less complicated for block supports consumers to use that instead of updating the store every time the attributes are updated.

I think another solution could be some function mapping attributes to block support hooks' options that doesn't rely on useEffect and ties more deeply into attribute updates, but I'm not sure a solution like that could solve the margin/padding UI problem in #31839. We need bidirectional communication between block support hooks and blocks that isn't just setting attributes.

It might be good tomake an issue to discuss what generic solution to the problem might look like.

I can also put it on the core-editor chat agenda for discussion. I'm actually in a reasonable timezone to attend that right now which is nice.

[talldan]

@talldan But you like the idea of using a store as opposed to the block attributes, right?

Yep, it's preferable to block attributes, as attributes should preferably used to drive the rendered output of a block rather than the editor experience. But I think anything in the block editor store should be generic, rather than referring to a concrete feature by name. So rather than getDuotoneVisibility as a selector, it'd be more like getFeatureVisibility( clientId, 'duotone' ).

There might also be other options when it comes to fixing this though. An alternative could be adding useState in the HOC here:

gutenberg/packages/block-editor/src/hooks/duotone.js

Lines 200 to 212 in ceaa2da

	( BlockEdit ) => ( props ) => {
	const hasDuotoneSupport = hasBlockSupport(
	props.name,
	'color.__experimentalDuotone'
	);
	return (
	<>
	<BlockEdit { ...props } />
	{ hasDuotoneSupport && <DuotonePanel { ...props } /> }
	</>
	);
	},

A setIsDuotoneEnabled function could be passed into the BlockEdit on this line so that the edit function can control enabling/disabling the controls. The state could be passed into the DuotonePanel.

This would become an API for duotone, and a question would be if it's possible to solve the problem for other features in the same kind of way. It's possible others working on these kind of features have had thoughts about the general problem, so definitely a good thing to gather a few more opinions. Flagging it at the meeting sounds like a good idea 👍

[ajlende]

There might also be other options when it comes to fixing this though. An alternative could be adding useState in the HOC

I was thinking too hard about the problem and overlooked the simple tools like useState 😅

a question would be if it's possible to solve the problem for other features in the same kind of way

I'm leaning towards not building a generic API considering how simple the useState already is. Trying to build one for all the block support hooks' UI seems like overkill especially when I'd want to take into consideration things like the margin and padding UI which isn't just a simple on/off toggle.

[gwwar]

Not sure if this is the best fit for it, but another option might be using context. https://reactjs.org/docs/context.html#updating-context-from-a-nested-component

[talldan]

Just to document it, #23198 is a report of the issue with the cover block I mentioned earlier where styles can be applied to the placeholder.

[Mamaduka]

Similar Styles/padding issue for the Image block - #23122.

[jasmussen]

Thanks for staying on this!

[oandregal]

While I like the idea of passing a setIsFeatureVisibleEnabled to the block edit function for the block to use as described at #33466 (comment) I have some braindump to share:

• This approach would work nicely for tools provided by core, but it won't for tools provided by third parties: because the block would be unaware they exist, it won't be able to disable them. Perhaps this is fine: so far, the block supports mechanism has remained something that core provides, but third parties don't extend. Alternatively, we could invert the control and make the block signal that its tools should be disabled. Having a general flag for all features could solve many of the use cases, including third-party tools. However, we'd still need particular conditions per feature in some cases (so we can disable duotone when the background is repeated or fixed, as Alex shared). Still, we can't expect blocks to know anything about third-party tools, so we land in the same spot.

• How would that work for things different than block supports? I'd think this mechanism should be the same as the block supports. So far, it seems it's only the styles panel. A quick look at the BlockInspector and BlockStyles components doesn't give me a clear way to hook into the block edit function to do the same thing block supports would do. They seem designed for pulling data from a store via the block's clientId.

It seems that making blocks to signal their "ready state" to the blocks store would cover more use cases, including third-party tools. Of course, we could also have specific "ready states" per feature to support cases like duotone's, although we should not encourage that. I've just seen that Nik is exploring this approach at #33823

[ajlende]

Thanks for the thoughts @nosolosw! I've been thinking long and hard about some of those things, so I added a comment on Nik's PR with some of the thoughts that I've had