Components: Avoid wrapping div for Slot

[aduth]

Closes #6839, #6632.
Supersedes #6920

This pull request seeks to update <Slot /> to avoid the need for a permanent div element wrapper. While a DOM node is needed for virtual-event-bubbling slots, based on the behavior of createPortal, it is acceptable to assign the node as the parent in which the Slot is rendered. This requires a div, albeit temporarily, until the parent can be determined.

Testing instructions:

Verify that there are no regressions in the behavior of Slot/Fill, both with and without bubblesVirtually.

Examples:

• Plugin sidebar (non-virtual)
• Popovers (virtual)

cc @ryanwelcher

[youknowriad]

I'm a little bit concerned about this. What happens if we do:

<div>
    <Slot/>
     <somethingElse />
</div>

What happens when the somethingElse rerenders but not the portal (or the opposite)? I wonder how React handles this and if it's safer to just keep the wrapper for the bubblesVirtually case, do we need to remove it for this use-case as well, it seems the main issue is for Plugin APIs which don't use bubblesVirtually?

[gziolo]

It needs to be rebased if we want to land it.

I don't have any feedback to share so the question is if we want to address what @youknowriad raised and merge it?

In my opinion, we should decide whether we land it in 3.6 as proposed or leave it as is forever 😄

[gziolo]

We probably should use react-test-renderer instead.

[aduth]

And here I thought we stopped using Enzyme altogether 😬

[aduth]

@gziolo Guessing you stopped by on an audit of API Freeze. FWIW I don't know that this really has any impact on public-facing API, except for how one might expect to interact with styling (and the presence of intermediary div elements).

[gziolo]

Guessing you stopped by on an audit of API Freeze. FWIW I don't know that this really has any impact on public-facing API, except for how one might expect to interact with styling (and the presence of intermediary div elements).

Yes, this exactly what happened. I wanted to clean up this project to see where we are. I agree it can be considered as non-breaking from plugin developer's standpoint. However, it might cause subtle unexpected visual discrepancies for theme developers. It's really your call. I'm fine with giving it a lower priority and removing it from the API Freeze project.

[youknowriad]

Do you think we can move this forward @aduth? I need this in one of the UI refactorings of the block toolbars.

Also, do you think an acceptable compromise here would be to only drop the wrapper when bubblesVirtually is false to avoid this #7231 (comment)
?

[aduth]

I think I share your concern at #7231 (comment) . I don't like fragmenting the behavior based on bubblesVirtually, but it might be the only / best option in this case.

I'll plan to do some short experiments on whether the React reconciler can deal with slot siblings, but otherwise will move forward with the Fragment for non-bubblesVirtually.

[aduth]

I've spent the better part of my afternoon on this one. Aside from porting the tests to using react-test-renderer, which was successful, I'm running into a few new frustrations, not entirely related to what's going on here:

There's a bug with virtual portal rendering, even with just a basic test case (fails on master as well):

	it( 'should render virtually by portal', () => {
		const tree = renderIntoDocument(
			<Provider>
				<Slot name="chicken" bubblesVirtually />
				<Fill name="chicken">
					content
				</Fill>
			</Provider>
		);

		const slot = findRenderedComponentWithType( tree, Slot );
		const slotDiv = findRenderedDOMComponentWithTag( slot, 'div' );
		expect( slotDiv.outerHTML ).toBe( '<div role="presentation">content</div>' );
	} );

It's related to some other issues we've had with race conditions between the rendering and registration of a fill/slot.

Generally, I'm finding this interchange between slots and fills, and the registration and rendering between them to be quite fragile. This is compounded by the distinct behaviors of bubblesVirtually and not. There's a lot in SlotFillProvider where we're just calling forceUpdate often unnecessarily, but in a (failed) effort to try to remedy these race conditions.

I considered more whether the bubblesVirtually is something we still want to offer as an option, or if we should default to one or the other. Based on some usage I found, I expect we are certainly still in need of the bubblesVirtually behavior (see e.g. Autocomplete capturing Popover input via onInput).

Given the prior concern about React's inability to reconcile changes if we'd render into a Slot by its parentNode (which I confirmed to be a valid concern), we still need a wrapper element for bubblesVirtually which would defeat the purpose of the pull request if we were to assign as the only option. There are also some issues I recall with bubblesVirtually not being the desired intended behavior (like onMouseDown for block multi-selection triggering when interacting with a block's inspector controls like a Range).

So I'm left feeling we need bubblesVirtually as an option. Considering how we could make this less fragile, I contemplated as part of this a refactor of Slot / Fill using the newer context API. Perhaps having the SlotFillProvider manage fills and slots via proper state (trickling down to consumers) could provide a more solid basis for keeping the slots and fills better in sync.

The downside is that in the course of trying various refactors, I've come to the conclusion that our tests do not cover the edge cases of the race conditions very well, so I have little confidence in our ability to refactor without potential for breakage.

Finally, there's some "fun" side-effects of removing the Slot wrapper, including a regression in the block inspector which strips intended padding from the block title:

...because we've styled elements in the sidebar to rely on the wrapper being an indicator of things like :first-child pseudo-class:

gutenberg/edit-post/components/sidebar/block-sidebar/style.scss

Lines 17 to 19 in 6ec181d

	&:first-child {
	margin-top: 16px;
	}

I've pushed a rebase of the branch which passes all tests to my knowledge, and "works", but I find to be very fragile and unreliable.

[youknowriad]

I agree that slot/fill is still fragile in general and us trying to support two different implementations in the same components makes it even more fragile.

• I wonder if we could make them separate components, it would make them easier to reason about
• I wonder if we can avoid using Slot/Fill in Popover and Autocomplete in favor of Portals but without necessarily having to rely on Slot and Fill (right in the components).

[gziolo]

Can we rebase and get it in as is in 3.8?

[youknowriad]

Actually, I forgot to mention but I removed the container div in #9572 it's not perfect and we probably need more robust tests.

[aduth]

Most of the changes in this pull request are an improvement on what exists in master. My previous comment applies to issues which were discovered in the course of refactoring, but that also exist in master as well. So there's an option that we just merge the first imperfect pass and open a separate pull request to address the remaining issues. However, with #9572 making the pivotal change already, this pull request is already mostly "refactoring" anyways (with a few small improvements), so it could be repurposed to attempting to address the remaining concerns.

@youknowriad I don't see the issue on master, so I'm assuming the styling issues noted in #7231 (comment) were accounted for ?

[gziolo]

I removed it from API freeze project as this PR won't have any breaking changes. #9572 might produce some styling issues if some theme used those div wrappers to style things.

[youknowriad]

@youknowriad I don't see the issue on master, so I'm assuming the styling issues noted in #7231 (comment) were accounted for ?

Yes, I did fix this issue

[aduth]

This may still be something worth exploring, but the pull request has languished too far and may be too difficult to salvage in its current form.