Slotfill no longer contextual?

[albanyacademy]

Description

Not sure if I'm saying this right, but in 6.3.2 this custom taxonomy component for a custom block looked like this:

and in 6.4 it now looks like this:

(I confirmed this by changing nothing in our plugin's code, and only rolled back / forwards the wordpress version.)

This is being looped inside a Repeater field we've made, and the SlotFill implementation is standard as per the docs.

Inside our Taxonomy Settings Panel component:

// ... other stuff
<Fill name={ 'repeaterControls' }>
{ checked && (
	<Button
		isSmall={ true }
		text={ 'Settings' }
		variant={ open ? 'primary' : 'secondary' }
		onClick={ () => toggleOpen( ! open ) }
	/>
) }
</Fill>

Block Edit.js:

<RepeaterFieldV2
  fields={ taxonomies }
  >
  <TaxonomySettingsPanel
	  availableTaxes={ availableTaxes }
  />
</RepeaterFieldV2>

Inside Repeater.js:

<SortableContainer>
{ !! fields && fields.map( ( field, index ) => {
	return (
		<SortableItem
			key={ `item-${ index }` }>
			{ props.children }
		</SortableItem>
	);
},
) }
</SortableContainer>

And inside our Sortable Item:

<SlotFillProvider>			
  { render }
  <div className={ cls.elem( 'controls' ).toString() }>
	  <Slot name={ 'repeaterControls' } />
  </div>
</SlotFillProvider>

Is this a bug? Did something change in the Slotfill package or somewhere else that would explain this change in behaviour? Again, we haven't changed anything at all in our codebase regarding these items between 6.3.2 and 6.4.1, so any help would be appreciated.

Step-by-step reproduction instructions

Make a slotfill inside a looped item?

Screenshots, screen recording, code snippet

No response

Environment info

No response

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

No

[albanyacademy]

lmk if more details are needed

[albanyacademy]

Ok simpler example.

This is the example code I used:

import { createSlotFill, SlotFillProvider } from '@wordpress/components';
import { registerPlugin } from '@wordpress/plugins';

const { Fill, Slot } = createSlotFill( 'CustomizeFillStructure' );

const SomeComponent = ( { children } ) => (
	<SlotFillProvider>
		<div style={ { border: '1px solid red' } }>
			<div>
				<Slot name={ 'custom' } />
			</div>
			<hr />
			{ children }
		</div>
	</SlotFillProvider>
);

import { PluginSidebar } from '@wordpress/edit-post';
/// Register the plugin.
registerPlugin( 'example-edit-post-plugin-pre-publish-panel', {
	render: () => (
		<PluginSidebar
			className="custom-panel-class"
			initialOpen
			title="My Custom Panel"
		>
			{ [ 'steve', 'dave' ].map( ( el ) => (
				<SomeComponent key={ el }>
					<Fill name={ 'custom' }>
						My name is { el }
					</Fill>
					<h1>
						Hello { el }
					</h1>
				</SomeComponent>
			) ) }
		</PluginSidebar>
	),
} );

In 6.3.2:

In 6.4.2:

[talldan]

My understanding is that using a slot in an array like that isn't a supported use case. I think generally there's supposed to be only a single instance of a 'slot'. I remember in the past considering whether slots might possible as a way to extend the blocks in List View, but it wasn't something that slots support. Instead the block editor tends to use it for things where there's only one instance of a slot (Inspector controls, toolbar etc.).

I'm not sure if it's possible to solve it by giving each slot a unique name, but may be worth trying. 🤔

Another observation, usually a fill isn't a child of the slot it's connected to, it'd be in an adjacent part of the component tree (for example, inspector controls is a slot that's filled by blocks rendered in a completely different part of the UI, the block canvas). It might also be possible to refactor your code to not use slot/fills, and it may also end up being much more performant.

[albanyacademy]

Interesting, but why would it have been working fine within an array prior to 6.4.x?

[talldan]

Interesting, but why would it have been working fine within an array prior to 6.4.x?

Not sure. There were a bunch of code changes to the component, but mostly refactors, here's the commit history:
https://github.com/WordPress/gutenberg/commits/trunk/packages/components/src/slot-fill

If you have the time, it could be worth trying to find the last good working version, so that it's possible to identify where the change was made that caused this (if it was caused by a change in the gutenberg code base, there's a chance it was also caused by a third-party library, even React).

I would help with this but personally have quite a lot on at the moment.

[ciampo]

I'd agree with the observations and the advice that @talldan shared above. I also don't have capacity to take a look right now, but I'd suggest first finding the point in time where the change in behaviour was introduced. We'll then be able to understand if it's going to easy to ever to the previous behaviour or not.

The safer change would on your side to have multiple slots instead of reusing the same one.

[jsnajdr]

This behavior change was caused by #53940. If there are multiple nested <SlotFillProvider> instances, the inner instance is now treated as noop -- it won't create a new context, but will pass through the parent context. Here are the critical lines of code:

gutenberg/packages/components/src/slot-fill/index.tsx

Lines 60 to 62 in 00ccb8b

	if ( ! parent.isDefault ) {
	return <>{ children }</>;
	}

In the example above, each <SomeComponent> instance is creating its own slot/fill context, is rendering its own slot, and is rendering one fill to that slot. The two rendered slots are separate.

But now the separate slot/fill context is not created, and both instances share the parent one. Registering two slots with the same name leads only to the second one being registered, the first one is overwritten. That's why both "My name is..." strings are rendered in the second instance of <SomeComponent>.

FYI @youknowriad who is the author of that change.

[youknowriad]

Interesting, I guess yeah, it was not a real intent to support contextual slot fills like that but since it was something that was supported, maybe we should just bring it back but I wonder what that means for the duplicated SlotFillProvider components being rendered.

[jsnajdr]

yeah, it was not a real intent to support contextual slot fills like that

Was the real intent to prevent having multiple slot-fill registries when rendering multiple nested BlockEditorProvider instances? Then SlotFillProvider could have some idempotent={ true } or passthrough={ true } prop that enables the special behavior to pass through an already existing registry. This would be enabled only for BlockEditorProvider, but other usages would get the old, and the more intuitive behavior: when rendering a SlotFillProvider, you simply get a new registry for the tree that's under the provider.

[albanyacademy]

+1. Just adding a flag to pass through would be nice - but if the performance difference between that and just rendering unique slotfills for every array item is negligible, then w/e, because I've converted our plugins code to do that and it works 🤷

[youknowriad]

@jsnajdr yep, that solutions looks good for me (could also be a private one to start with).