inspector-section: Track isOpen to make a better isContextuallyActive() function

[mreishus]

Description

Inside the WP customizer, with certain themes, you can edit block widgets using the gutenberg editor. One of the features available is to choose "Show more settings", which replaces the entire editor with the settings sidebar. Typically, gutenberg has its own sidebar display area, but in the customizer, we use this neat trick because we're already rendered inside a sidebar.

Peek.2021-09-22.10-30-advanced-settings-incustomizer.mp4

The way this is done is by adding a customizer "section", the "inspector section". This is unconventional, because sections are typically shown at the root level of the customizer:

The customizer checks two active-related values about each section, deciding if it should be drawn. A month ago, the values for this inspector section were as follows:

• active(): Always true.
• isContextuallyActive(): Always false.
  This allowed the section to be used, but to not be rendered at the root level.

In #34543, it was discovered that isContextuallyActive() always returning false lead to some bugs while using the section. Notably, sometimes you would be kicked out of the section while using it. A much more detailed writeup is available in that issue. That changes the values for the inspector section to be:

• active(): Always true.
• isContextuallyActive(): Returns active(), which is the same as always true.
  This allows the section to be used, and stops the bugs discovered in Fix Block Settings sidebar unexpectedly collapsing #34543 from kicking the user out of the section while using it, but it introduced a new bug: "Block Settings" menu entries are now rendered at the root level (Unexpected "Block Settings" sections added to Customizer #35041).

This PR fixes the issue in #35041 while keeping the issue in #34543 fixed as well. That is, it changes the values for the inspector section to be:

• active(): Always true.
• isContextuallyActive(): Only true when the block settings section is actively open.

I had to get a bit tricky with this. First, I had to add a new this.isOpen variable and track it in the custom open function (link1, link2), and create an overridden collapse function that sets isOpen to be false. My first approach was to use onChangeExpanded to track this state, but it doesn't work properly since the block settings sidebar is a layer "on top of" the G editor sidebar. For example, when closing the block settings sidebar, the G editor sidebar would display, and onChangeExpanded in inspector-section would run with expanded=true.. but I want isOpen to be false when the block settings is closed.

Finally, I had to make the customizer "ask about" the isContextuallyActive() value more often. It simply doesn't expect the value to change when it does, and doesn't query the isContextuallyActive() value when it needs to. The relevant part of WP is here:

https://github.com/WordPress/wordpress-develop/blob/b83b01e1b01429dc4a6da49ec23e1f7dfc4ae446/src/js/_enqueues/wp/customize/controls.js#L996-L1001

			container.active.bind( function ( active ) {
				var args = container.activeArgumentsQueue.shift();
				args = $.extend( {}, container.defaultActiveArguments, args );
				active = ( active && container.isContextuallyActive() );
				container.onChangeActive( active, args );
			});

Here, it's setting up this function with container.active.bind, which uses a custom binding system to define callbacks that run when container.active changes. This callback takes into consideration isContextuallyActive(). The problem is, container.active isn't something that changes for block settings; it's always something that we want to be able to render when the user presses the "Show More Settings" menu item. So I manually called a part of the customizer's binding code to fire the function above, which considers isContextuallyActive. Link: https://github.com/mreishus/gutenberg/blob/f6833587e3fa14b0e9ab43c01596d83733cfc671/packages/customize-widgets/src/controls/inspector-section.js#L70-L72

this.active.callbacks.fireWith( this.active, [ false, true ] );
^ This says "Fire the callbacks associated with this.active moving from false to true".

I did first try this.active.set(true), but the callbacks do not fire when moving from true to true.

Ultimately, these are complex changes driven by the settings sidebar of gutenberg not fitting cleanly with the customizer's notion of a "Section". However, this change works well in my testing and fixes both issues.

Additionally, I removed the overrided added to sidebar-section.js. It seems to not be needed to fix #34543.

How has this been tested?

• Used wp-env
• Follow steps to reproduce in Fix Block Settings sidebar unexpectedly collapsing #34543
• Follow steps to reproduce in Unexpected "Block Settings" sections added to Customizer #35041
• BOTH issues should be fixed after these changes

Screenshots

• See screenshots and videos in Fix Block Settings sidebar unexpectedly collapsing #34543 and Unexpected "Block Settings" sections added to Customizer #35041. Here, we're keeping both issues fixed at the same time.

Types of changes

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).

[chad1008]

I'll defer to more experienced reviewers on the coding and implementation, but I can confirm this tests well. The Block Settings no longer appear in the Customizer. Adding and editing widgets, including the individual block settings for each worked well for me. I tested both block and legacy widgets without any issue 👍

[noisysocks]

Thanks so much for digging deeper into this! ❤️

I think your fix makes sense, though I'll cc. @kevin940726 as he's smarter than me.

I'm worried about the maintainability of this new code as it's quite nuanced. For example, I can foresee a developer in the future not understanding why we override collapse() and what this.active.callbacks is. Would you mind copying some of what you wrote in this PR description into doc comments?

Additionally, a regression E2E test could be really useful here to ensure this doesn't regress again in the future.

Lastly, from #34543:

So, my solution is to override isContextuallyActive() for the sections that are defined in Gutenberg, and have them only return if the section itself is active, while ignoring the .controls() check. Note that this is not without precendent, as themes in the customizer had to do the same thing.

Does this mean themes also has this bug?

[noisysocks]

Could use a setter here if you want to be fancy and reduce the likelihood of a future developer accidentally doing this.isOpen = x instead of this.setIsOpen( x ).

get isOpen() {
	return this._isOpen;
}
set isOpen( value ) {
	this._isOpen = value;
	this.triggerActiveCallbacks();
}

[noisysocks]

Let's rename opt to option or something more descriptive. We try our best to avoid abbreviations in Gutenberg.

[kevin940726]

Agree that we should have regressions test for this as this is really getting complicated.

The code looks good to me as long as it works :). This whole part is already a giant workaround on top of a workaround anyway 😅.

[mreishus]

I've updated it with some comments and made a simple e2e test to make sure that the "Block Settings" header is not being rendered. I want to make a test for #34543 eventually, but I'm being moved to a high priority project and at least wanted to share what I have so far.

[kevin940726]

This will wait for 3 seconds, best to provide the timeout option. Or just use the $$ selector.

[mreishus]

I was able to move the timeout down to 0 and verify that it still fails with the fix missing, and passes with the fix present

[mreishus]

I was able to work on this again and now we have e2e tests that cover #34543 and #35041 .

[noisysocks]

Fantastic work. Thanks!