Slot/Fill: Add a constrained version

[youknowriad]

Required for #14367

In #14367 we're building an editor inside another editor which means we'll have two "inspector controls" slots, two "block controls" slots...

Based on the current implementation of Slot/Fill, this means the last rendered Slot will get all the fills and the other one is useless. That's not exactly what we want, we want to be able to assign the Fills in the embedded editor to the embedded slot and the one outside the embedded editor should go into the global slot. If we use another SlotFillProvider at the BlockEditorLevel, this will work but the problem is that it will duplicate all the popovers rendered in the embedded block editor won't be shown as there's no slot for them in that provider and the Popover Slot is something global.

This led to the decision to create a version of SlotFill that allows only a single Slot inside its assigned provider. The API looks like this:

const { Provider, Slot, Fill } = createContrainedSlotFill();


const ParentComponent = () => (
   <Provider>
        <Slot />


       <ChildrenCanGoHere />
   </Provider>
)

const ChildComponent = () => (
   <Fill />
)

This means the PopoverSlot will have its own PopoverSlotProvider, the InspectorControlsSlot will have its own InspectorControlsProvider...

The implementation here is made very simple by leveraging React hooks.

To do

• Tests
• Refactor all the existing Slots

[youknowriad]

Ignore the React upgrade changes as these were cherry-picked, the PR is not merged yet.

[nerrad]

I like this (good example of using the new React hooks api as well). What's not immediately clear to me is:

• Is this intended to eventually replace the existing createSlotFill?
• If not intended to eventually replace, what criteria determines when the new constrained slot fill is used vs the original createSlotFill? I realize the use-case currently is allowing for multiple editor instances, but that appears to me to indicate this might eventually deprecate the use of createSlotFill?

[nerrad]

While I realize it's not needed (because it's "guaranteed" to be static), convention seems to be to include dispatch as a dependency here. Should we included it along with [ instanceId, children ]?

[youknowriad]

right 👍 Seems like a good thing to add.

[youknowriad]

I think this new version is a bit superior in terms of API (and even simplicity of the implementation). The problem is that we can't remove the old one (BC) so we might have to support both even if we update all of our current slots with the new API.

[aduth]

we want to be able to assign the Fills in the embedded editor to the embedded slot and the one outside the embedded editor should go into the global slot.

My reading of this must be wrong, since it seems we want certain fills within an embedded editor (i.e. inspector controls) to reach outside into the one slot rendered by the parent editor. Or, at least, there should be at most one slot of this given name. Which, based on your further remarks and specifics notes about inspector controls, sounds like it's what you've implemented here?

I'm not in a well-adjusted mindstate to delve into this now, but wanted to at least remark on it. I could have sworn something like this had been considered with previous efforts for the embedded reusable blocks editor (#7453), but I can't find but a few mentions of it, no implementations.

• Inheriting context in EditorProvider to allow pass-through of slot fill handling
  • Shared block inspector controls surface to the main editor sidebar

[youknowriad]

My reading of this must be wrong, since it seems we want certain fills within an embedded editor (i.e. inspector controls) to reach outside into the one slot rendered by the parent editor

So, yes but we don't want them to reach it directly. With this PR, we'll be using a "Slot" for the inner inspector inside a "Fill" for the parent block inspector "Slot". Maybe it's confusing at the moment, but I can go ahead, cherry-pick and implement it in the other PR to clarify.

[youknowriad]

After testing this in #14367 I'm having some issues:

• It seems that we can't use a "Slot" inside a "Fill" with the current implementation and I'm not certain if it's possible to "fix" this.
• I was able to fix it by refactoring this code and using the "bubblesVirtually" technique, the problem is we can't use "bubblesVirtually" because it breaks the keyboard navigation in the toolbars because we can't catch the "keypress" events anymore from a parent div.

[youknowriad]

This also makes me wonder if we shouldn't always use "bubblesVirtually". Doing so means when we do things like onFocus, onClick... anytime we want these events to take into account nested elements we need to rely on dom and not React handlers.

[youknowriad]

Anyway, I'd appreciate some thoughts because I feel like I'm going on a rabbit hole and not sure what's the path forward.

[aduth]

This also makes me wonder if we shouldn't always use "bubblesVirtually". Doing so means when we do things like onFocus, onClick... anytime we want these events to take into account nested elements we need to rely on dom and not React handlers.

I think generally yes, bubblesVirtually is the more ideal behavior for us to be promoting. At the time that it became an option (when portals became a thing), we had more than a few conflicts where the virtual bubbling produced unexpected behaviors (e.g. #3082). See also #3132.

Edit: I may have read your comment as proposing the opposite of what I thought (i.e. are you suggesting we don't use the virtual bubbling?). I still think that bubblesVirtually will give us less trouble in the long-term, but understand that its many conflicts may not be worth the trouble.

[aduth]

In #14367 we're building an editor inside another editor which means we'll have two "inspector controls" slots, two "block controls" slots...

I need to look closer at this, but I wonder: Why are there two of each slot? If we'd just render the one (in the top editor), wouldn't those fills rendered in the embedded editor find their way correctly to the sidebar inspector controls? That's what we want, isn't it?

So, yes but we don't want them to reach it directly.

Can you expand on this? I guess as I see it, the fills would be coordinated by whichever closest <SlotFillProvider /> exists. This could be one rendered by the embedded editor, but especially for how we want this to behave, it seems like we wouldn't want a separate SlotFillProvider for the reusable blocks, and instead let those be handled by the top-level editor.

[youknowriad]

wouldn't those fills rendered in the embedded editor find their way correctly to the sidebar inspector controls? That's what we want, isn't it?

You read right! I was proposing we only use bubblesVirtually. but I'm not certain how much work this involves as we have a lot of DOM tree related behaviors.

Can you expand on this? I guess as I see it, the fills would be coordinated by whichever closest exists. This could be one rendered by the embedded editor, but especially for how we want this to behave, it seems like we wouldn't want a separate SlotFillProvider for the reusable blocks, and instead let those be handled by the top-level editor.

That's not the direction I was going, I was trying to use two provides and two slots and render the inner Slot inside the parent fill to cascade. but rendering a Slot inside a Fill don't work unless we use "bubblesVirtually" in my testing.

[youknowriad]

I need to look closer at this, but I wonder: Why are there two of each slot? If we'd just render the one (in the top editor), wouldn't those fills rendered in the embedded editor find their way correctly to the sidebar inspector controls? That's what we want, isn't it?

This would be a solution (alternative in the precedent comment) but the alternative I was trying to implement is more "controlled" for example, it would allow us to say: the inner inspector could go in a modal while the outer one stays at its position, we could have had more "control"

[aduth]

it would allow us to say: the inner inspector could go in a modal while the outer one stays at its position, we could have had more "control"

A question I might have then is: Do we need it now?

And even for hypothetical other usages of a Block Editor, the current behavior could still support placing inspector controls in a modal, just not a combination of differing placements for nested editors. To this end, I might wonder if we need it ever (is there some proposal for a varying placement slot?).

[youknowriad]

is there some proposal for a varying placement slot?

There are discussions here and there. Customizer inspector, widgets... but not specifically reusable blocks. I agree that if we can solve the reusable blocks issue without this, we can move forward and rethink this when needed.

[aduth]

In my work related to #14367, I've been running into similar issues to what's considered here. While I'm still inclined toward the idea of a single InspectorControls.Slot, there's a problem of dealing with toolbar controls when a block is selected within a reusable block.

In this scenario, either we:

• Let both the reusable block and the inner block each be considered "selected" within their own editing context. The challenge here then is that there will be two BlockControls.Slot present, one for the reusable block, and one for the inner block. Trying to edit the inner block may wrongly render the block controls into its parent reusable block's toolbar instead.
• Only allow one block to be selected at a time, regardless of editing context. The challenges here are that (a) it's cumbersome to coordinate the selection states across two editor stores (this is what ReusableBlockSelection was doing in the earlier WIP: Blocks: Reimplement shared block as embedded editor #7453) and (b) the top-level editor only renders the inspector controls if there's a selected block, so there needs to be some way to either forcefully override or otherwise communicate that there is a selected block, but it exists within an embedded editor.

The first option would lead down a path toward something more like what's explored in this pull request. In my mind though, I'm wondering if it could be implemented like something of a whitelisted proxy: a limited SlotFillProvider which handles some slots (block controls) but then passes others up to its own context provider (inspector controls).

It could even be an overloaded form of SlotFillProvider which accepts the exclusive names of slots it should handle, e.g.

<SlotFillProvider slots=[ 'BlockControls' ]>{ children }</SlotFillProvider

Edit:

Only allow one block to be selected at a time, regardless of editing context.

This line of thinking also has me considering whether, if selection is meant to be considered more "global", that it be considered as part of state separate from an individual registry's block editor store.

[youknowriad]

This line of thinking also has me considering whether, if selection is meant to be considered more "global", that it be considered as part of state separate from an individual registry's block editor store.

I don't think we should be shared between from the embedded editor and the parent editor. (I think a lot of reusable blocks open issues are due to this). I feel the first option the most logical for me.

<SlotFillProvider slots=[ 'BlockControls' ]>{ children }

This is not that different from the current PR, what I like about the current PR is that it removes the need for these strings, thus simplifying the implementation and solving this use-case.

The only downside I see for the current approach compared to this proposal, is that it will force us to use a Provider for each slot but ultimately I think it's a good thing because these providers in a lot of cases can be embedded in reusable components:

• BlockControlsProvider is something unique for each BlockEditorProvider so it can be embedded in it
• BlockInspectorProvider is very similar
• The SidebarSlotsProvider, MoreMenuSlotProvider are something that are specific to the EditPost screen and should be used in EditPost root component in that case.
• The PopoverSlotProvider is something generic to the whole screen and can be used once in the root of the application
  ...

I think it clarifies where we should include these providers instead of blindly adding them to the root level like we do today and making our components less reusable.

[aduth]

it removes the need for these strings

Yeah, it occurred to me after I left my comment that the strings are considered an internal implementation detail. At the very least, it should be a reference to the slot component, e.g. <SlotFillProvider slots=[ BlockControls.Slot ]>{ children }</SlotFillProvider>.

That said, I'll plan to revisit next week in mind of your feedback.

[youknowriad]

I'm going to close this PR at the moment.

I still believe this is a better Slot/Fill implementation but the fact that it doesn't solve the first goal I intended here (the reusable blocks refactoring) makes me thing it's not urgent to provide an alternative implementation if there's no added value for us directly.