Fix FM module loader in Gutenberg context

[kjbenk]

Problem

Most FM initializes happens when the DOM is ready and only if that element is considered visible via the jQuery .is(':visible') function. In a recent Gutenberg update, metabox contents are not rendered until after the DOM is ready. (related to - #793 (comment))

This can lead to issues with initialization since some metabox elements will not be rendered properly on DOM ready and therefore will return false for .is(':visible').

Potentially closes - #812

Steps to Reproduce

1. Add a sortable repeater field

add_action( 
	'fm_post_post',
	function () {
		$fm = new Fieldmanager_Textfield(
			[
				'name'  => 'test_field',
				'limit' => 0,
				'sortable' => true,
			]
		);
		$fm->add_meta_box( 'Sortable Test', 'post', 'normal' );
	}
); 

2. Notice that you are unable to sort the fields when in the Gutenberg editor

Expected Behavior

• Sortable fields are properly initialized within the Gutenberg editor

Solution

When loading FM in a Gutenberg context, hook into the areMetaBoxesInitialized (wait a short period of time) and perform all FM initializations. This ensures that all metabox elements are rendered and in the DOM before any .is(':visible') checks are executed.

[dlh01]

@kjbenk So that folks can test more easily, could you post (or link to) example fields that are subject to the bug, and what the expected behavior is before and after the patch is applied?

[kjbenk]

@dlh01 Thanks for the input :) I added some steps to reproduce the issue and the expected behavior.

[mboynes]

There must be a way to do this without the setTimeout call, let's keep at it until we identify what that is. setTimeout is too fragile for this.

[kjbenk]

Totally agree :) I figured starting here is better than nothing for the time being.

[kjbenk]

I just did some quick testing and something like this works. The only thing I do not like about this approach is that we are not checking any state within the subscribe function, but instead checking a DOM element. Ideally, the code we run within wp.data.subscribe would validate against the store, however, this might be a good option for now.

	const wrappedCallback = () => {
		if (document.querySelector('.block-editor-page')) {
			const metaboxesInitializedUnsubscribe = wp.data.subscribe(() => {
				// Metaboxes are rendered and in the DOM.
				if ( document.querySelector('.edit-post-meta-boxes-area__container') ) {
					callback();

					// Clean up.
					metaboxesInitializedUnsubscribe();
				}
			});
		} else{
			callback();
		}
	};

[joemcgill]

I think subscribing to the areMetaBoxesInitialized state is the right approach, but rather than using a setTimeout, I think we could wrap the callback in a polling function that checks for the existence of the initialized markup before firing our callback. The polling function would need to have an upper limit of the amount of times it would run, but should work great.

[joemcgill]

I took a stab at updating @kjbenk's previous approach with a polling function in 08926e6. I think this a good enhancement on the original setTimeout approach that is a bit less fragile but works similarly in that it sets a polling function once the wp.data store has an updated state that the meta boxes have been initialized, but waits to make sure the markup is in place before firing the callback.

[kevinfodness]

Left a suggestion, but I like this a lot. 🍣

[kevinfodness]

Rather than setting this to 0 here and hardcoding the number of attempts it can't be greater than below (5) why not set this to 5 (and maybe rename it to something like pollAttemptsRemaining) and decrement it instead? Then the check could just be whether there are any poll attempts remaining. Currently, the threshold for polling attempts is pretty far down into the logic.

[joemcgill]

Good suggestion. Updated to decrement the counter in 1f4697f.

[mboynes]

Do we feel this is always going to be sufficient? It's only as little as a half a second -- but, if loading the meta boxes takes longer than that, I expect it would be blocking the event loop, so that's probably moot?

Using timeouts or intervals still feels like a flawed pattern to me. For what it's worth, setInterval doesn't appear once in the GB codebase. And as far as I could tell with a quick pass, setTimeout was only used in cases like animations, flashes, etc.

Further, is an interval even necessary? Once subscribed, won't this repeatedly get executed during ~any activity in the editor? And to that point, should this get properly unsubscribed once successfully executed? Does GB subscribe work like redux subscribe in that you get an unsubscriber and can unsubscribe? I actually don't know.

[mboynes]

yes, it looks like subscribing a listener returns a function to unsubscribe it, as with redux

[kjbenk]

FWIW @joemcgill here is how I implemented the unsubscribe logic - c25c3ca#diff-92affb4a9da5be89b7b8cf47a3822c1eba297ee97c9f7487feaea5f214726c45R33

[joemcgill]

@mboynes If we want to be in explicit control of when the callback gets fired, I think we'll need some way of handling the timing within our listener. That said, you're correct in the pragmatic sense in that...

Further, is an interval even necessary? Once subscribed, won't this repeatedly get executed during ~any activity in the editor? And to that point, should this get properly unsubscribed once successfully executed?

We can probably just rely on state changes here instead of an interval and properly unsubscribe the listener after the callback is called. I've updated to that approach in 6f00450 and refined in 621cfa3. This is much simpler and works well.