Using customClassName: false produces a weird bug

[icurdinj]

Describe the bug
In WP 5.4-RC2, if a block uses

supports: {
	customClassName: false,
},

a weird bug occurs in relationship with another block that displays Advanced panel: if you click on a block without Advanced and immediately on a block with that panel, the panel will be missing from the block where it should be, and shown on the block where it shouldn't be (and corrupted - without controls inside).
This was not happening on WP 5.3.

To reproduce
Steps to reproduce the behavior:

1. Insert 2 blocks in Editor - 1 with customClassName: false and 1 with customClassName: true
2. Click on both (you probably won't see the problem on the first Inspector render, but will on next clicks)
3. Advanced panel in the Inspector will be missing on the block where it should display, and will show up on the block where it shouldn't display (and there, it will be missing controls inside).

Expected behavior
Advanced panel shows by default, does not show on a block with customClassName: false, and works properly. This worked properly on WP 5.3.

Desktop (please complete the following information):

• OS: [linux]
• Browser [chrome]
• Version [80]

Additional context

• Gutenberg 7.3, I guess?

[icurdinj]

Note: I just checked with latest Gutenberg plugin version, 7.7.1, and this issue seems to be fixed there. So, this should be just about backporting the fix to WP 5.4 before it's too late.

[youknowriad]

@jorgefilipecosta It would be good to identify if there's a fix that needs backporting here. Maybe the SlotFill refactor fixed this issue?

[jorgefilipecosta]

I verified the issue is present in WordPress 5.4 RC and happens even with the default blocks (e.g: paragraph and shortcode block). The issue is not present in the Gutenberg plugin because it was fixed by #19242.

[aduth]

Debugging via git bisect points to 6447a09 (#16807) as the point at which the issue began.

[aduth]

Looking specifically at the changes of 6447a09 and comparing to how it was fixed via #19242, I believe the issue stems from this remark in the pull request:

For example, the context value never changes because it's a class property whose value is never updated during the SlotFillProvider lifecycle:

You can see in 6447a09 that <InspectorAdvancedControls.Slot bubblesVirtually /> is rendered on the basis of the condition of hasFills from __experimentalSlotFillConsumer. Depending on the order that these components are rendered, if a block provides a fill for the advanced controls slot after the block inspector has rendered, it may not be shown, because there is nothing which would cause the inspector (specifically __experimentalSlotFillConsumer) to re-render and evaluate that fills now exist.

[aduth]

I was able to confirm that it's the lack of an updating context value which prevents the rendering from occurring as expected in the changes introduced with 6447a09.

I did so by isolating a small subset of the effective fix from #19242, applied to the previous implementation of SlotFillContext:

diff --git a/packages/components/src/slot-fill/context.js b/packages/components/src/slot-fill/context.js
index b4229c71f..094d47821 100644
--- a/packages/components/src/slot-fill/context.js
+++ b/packages/components/src/slot-fill/context.js
@@ -31,11 +31,12 @@ class SlotFillProvider extends Component {
 		this.getFills = this.getFills.bind( this );
 		this.hasFills = this.hasFills.bind( this );
 		this.subscribe = this.subscribe.bind( this );
+		this.updateContext = this.updateContext.bind( this );
 
 		this.slots = {};
 		this.fills = {};
 		this.listeners = [];
-		this.contextValue = {
+		const contextValue = {
 			registerSlot: this.registerSlot,
 			unregisterSlot: this.unregisterSlot,
 			registerFill: this.registerFill,
@@ -45,6 +46,11 @@ class SlotFillProvider extends Component {
 			hasFills: this.hasFills,
 			subscribe: this.subscribe,
 		};
+		this.state = { contextValue };
+	}
+
+	updateContext() {
+		this.setState( { contextValue: { ...this.state.contextValue } } );
 	}
 
 	registerSlot( name, slot ) {
@@ -63,6 +69,7 @@ class SlotFillProvider extends Component {
 		if ( previousSlot ) {
 			previousSlot.forceUpdate();
 		}
+		this.updateContext();
 	}
 
 	registerFill( name, instance ) {
@@ -71,6 +78,7 @@ class SlotFillProvider extends Component {
 			instance,
 		];
 		this.forceUpdateSlot( name );
+		this.updateContext();
 	}
 
 	unregisterSlot( name, instance ) {
@@ -83,6 +91,7 @@ class SlotFillProvider extends Component {
 
 		delete this.slots[ name ];
 		this.triggerListeners();
+		this.updateContext();
 	}
 
 	unregisterFill( name, instance ) {
@@ -92,6 +101,7 @@ class SlotFillProvider extends Component {
 		);
 		this.resetFillOccurrence( name );
 		this.forceUpdateSlot( name );
+		this.updateContext();
 	}
 
 	getSlot( name ) {
@@ -139,7 +149,7 @@ class SlotFillProvider extends Component {
 
 	render() {
 		return (
-			<Provider value={ this.contextValue }>
+			<Provider value={ this.state.contextValue }>
 				{ this.props.children }
 			</Provider>
 		);

This mimics the memoization behavior implemented in #19242 via the new useSlotRegistry hook:

https://github.com/WordPress/gutenberg/pull/19242/files#diff-f472b8665ea56405a705f3ff1bc5a6d2R12-R80

---

As I see it, options for WordPress 5.4 could be one of the following:

• Cherry-pick Components: Refactor SlotFill #19242 to backport
• Revert Update the Inspector slots to use the bubblesVirtually slots #16807 ("un-closes" RichText don't work properly in InspectorControls because BlockEditContextProvider is not providing the right context. #9203)
• Implement an isolated fix to SlotFillContext, like in the diff above
• Allow the bug to ship, assuming it will be fixed in the next version via Components: Refactor SlotFill #19242

(cc @diegohaz)

[diegohaz]

Implement an isolated fix to SlotFillContext, like in the diff above

This is the only option I would go against as I'm not sure how this would impact the current code that relies on forced updates. If all tests pass though, maybe it's not a problem.

I also think that bubblesVirtually (or using React Portal on SlotFill) should be re-considered in the future. After working on that, I see it leading to more bugs that can't be easily (or possibly) solved using React Portal.

It's not supported by React Native. So, even if we migrate every Slot to bubblesVirtually, they'll still differ from the RN versions.

[aduth]

The patch in #20874 (comment) seemed to work okay when applied to the v7.1.0 tag, but trying to incorporate it into wp/trunk for cherry-picking to WordPress 5.4, I encounter errors when interacting with the editor:

Invariant Violation: Maximum update depth exceeded. This can happen when a component repeatedly calls setState inside componentWillUpdate or componentDidUpdate. React limits the number of nested updates to prevent infinite loops.

I suppose it could be an issue that somewhere, as a result of the context updating, a component triggers to register or unregister a slot or fill (thus causing an infinite loop).

In any case, it's alarming enough to me to want to avoid it.

@diegohaz For this issue specifically, could you imagine any subset of changes from #19242 we could use for a fix? Or you think it should be cherry-picked in its entirety? Is your commentary on bubblesVirtually meant to advocate a revert of #16807? @youknowriad had mentioned to me that, in addition to addressing #9203, it was apparently fixing some other issues with RichText as well.

[diegohaz]

I think the safest option for now is to cherry-pick #19242 entirely.

Regarding bubblesVirtually, I'm just afraid of React Portal not being the right solution for the problems we have with the regular SlotFill implementation. I don't mean any immediate changes, but I wanted to share this so that we keep in mind if new issues arise. 😊

[jorgefilipecosta]

The refactor was cherry-picked so we can close this issue.