WIP: Blocks: Reimplement shared block as embedded editor

[aduth]

Related: #7409, #7130
Closes #7119

Note: Work-In-Progress. Most basic behaviors work as expected, though lack some of the UX polish which had existed previously around intermediate states (saving, etc), unit tests, documentation.

This pull request seeks to explore a reimplementation of the shared block to render its editable form as a nested instance of <EditorProvider><BlockList /></EditorProvider>.

In doing so, it explores:

• Inheriting context in EditorProvider to allow pass-through of slot fill handling
  • Shared block inspector controls surface to the main editor sidebar
• Unique editor stores by core/data namespace (e.g. core/editor-shared-8174)
  • Workaround to the fact that all stores registered to @wordpress/data are shared in a common global namespace
• Umbrella override of select calls within an element tree via withCustomReducerKey to avoid each descendent needing to be updated to account for the custom store namespace

  • gutenberg/editor/components/provider/index.js

    Lines 130 to 136 in 58c5103

    	withCustomReducerKey( ( reducerKey, ownProps ) => {
    	if ( reducerKey === 'core/editor' && ownProps.reducerKey ) {
    	return ownProps.reducerKey;
    	}
    	return reducerKey;
    	} ),

• Using existing Editor effects to leverage updates to the shared block (both its title and content), rather than a separate set of duplicated effects (saving a shared block uses the same REQUEST_POST_UPDATE since it is in-fact a post, rather than a custom SAVE_SHARED_BLOCK action)
  • In doing so, it removes the custom block endpoint so as to be able to treat the shared blocks as no different than any other post
• Eliminating shared blocks as part of the top-level editor's own blocks state
  • Important for proper solution to what was initially explored as part of State: Trigger autosave as standard save for draft by current user #7130 for autosave and dirty detection
• Shared block components no longer need to pull data from the top-level data store tracking of state.sharedBlocks, since as being part of its own provider context, it can simply call the same selectors for edited post (e.g. getEditedPostAttribute( 'title' ) to retrieve the shared block title)

  • gutenberg/core-blocks/block/edit-panel/index.js

    Lines 128 to 134 in 58c5103

    	withSelect( ( select ) => {
    	const { getEditedPostAttribute } = select( 'core/editor' );
    	return {
    	title: getEditedPostAttribute( 'title' ),
    	};
    	} ),

There's some future work here that may enable removing all shared block knowledge from the top-level editor, not addressed here:

• Fully treating shared blocks as simply post entities to be fetched from the core data namespace
  • Fetching, deleting, creating

[noisysocks]

Very clever! I haven't looked at this closely yet but I like the idea.

Just dropping a note to be careful when dropping the shared blocks API controller. It has a few workarounds in it which ensure that only certain roles can read and delete shared blocks, see #4725.

[noisysocks]

I don't think we should remove these tests. They help us prevent #4725 from regressing.

[aduth]

Just dropping a note to be careful when dropping the shared blocks API controller. It has a few workarounds in it which ensure that only certain roles can read and delete shared blocks, see #4725.

Yeah, fair to say its removal is tentative, though I'd like to see it gone, merely for the inconsistencies it has with the posts responses, and the fact that it doesn't seem like it should be any more special than a standard post. Shouldn't the posts endpoint already be accounting for these capabilities?

[noisysocks]

Shouldn't the posts endpoint already be accounting for these capabilities?

In theory, yes. I vaguely remember running into some issues with the WP API posts controller. So long as we confirm that the capability checking hasn't regressed (e.g. by keeping test_capabilities) then I'm happy 🙂

[aduth]

Reference to original implementation as reference in preparation for pending rebase: 58c5103

[aduth]

Rebased to account for the new data registry implemented by @youknowriad in #7527 .

This is still very much work-in-progress, but it's back to a workable state, and simpler too now with the new registry.

I'm curious to get feedback on the idea with cloning registries / creating editor stores. I added a clone function to the store object. This also requires tracking original "register" values, because we assign into the registry internal namespaces object using overridden behavior (e.g. selectors with pre-bound state).

[noisysocks]

I really like the overall approach!

I'm fine with how clone() is implemented especially since namespaces and _cloneConfig isn't accessible by callers of createRegistry(). This means we can iterate on how clone() works without breaking backwards compatibility. For example, in the future, it might be worth exploring using prototype inheritance to save some memory.

Are you OK for me to pick this PR up and sand off the remaining rough edges? I'm not sure how your schedule looks 🙂

From my very quick testing, I spot these problems:

• Progress indicator no longer shows after clicking Save
• Inserter isn't updated when shared block title is changed
• Exception when selecting Convert to Regular Block
• Menu shows Convert to Shared Block instead of Convert to Regular Block after refreshing the page
• Shared blocks aren't shown in the inserter after refreshing page
• More Options and Move up / Move down shouldn't be shown within a shared block
• Deleting a shared block does not remove the wp:block from the post
• Needs tests and JSDoc comments
• Need to confirm (e.g. using unit tests) that Add capabilities for reusable blocks #4725 hasn't regressed

The E2E tests that I added in 5eb5fdd should act nicely as a set of acceptance tests for this PR.

[aduth]

Are you OK for me to pick this PR up and sand off the remaining rough edges? I'm not sure how your schedule looks 🙂

Yes! Please feel free.

[aduth]

The E2E tests that I added in 5eb5fdd should act nicely as a set of acceptance tests for this PR.

❤️ ❤️ ❤️

[noisysocks]

I rebased and fixed some of the regressions identified above. Will continue on this tomorrow.

One regression that I'm struggling with is that changes to a shared block aren't picked up by other blocks on the page. For example:

1. Insert the same shared block twice
2. Make some changes to the first wp:block (e.g. change its title)
3. Note that the second wp:block isn't updated with these changes

(The same issue manifests itself in a few different ways. For example, if you change a shared block's title, the inserter doesn't immediately update to have the new title.)

I suspect that this happens because changes to a wp_block entity record are isolated to the registry that the change occurred in. That is, when editPost() is dispatched from one shared block editor, other editors aren't notified because they are subscribed to a different Redux store. Having seperate registries mean that we no longer have a single source of truth for wp_block entity records.

Does that make sense, @aduth? Do you have any ideas on how to address this?

[youknowriad]

I suspect that this happens because changes to a wp_block entity record are isolated to the registry that the change occurred in. That is, when editPost() is dispatched from one shared block editor, other editors aren't notified because they are subscribed to a different Redux store. Having seperate registries mean that we no longer have a single source of truth for wp_block entity records.

While editing the shared blocks, I'd say it's acceptable, but we should probably rely on the serialized value of the original store when we're not in edit mode. What I'm trying to say is that the separate registry should only be used when we click "edit" to edit the content of the shared block and not wrap the entire BlockEdit of shared blocks with a RegistryProvider.

[noisysocks]

but we should probably rely on the serialized value of the original store when we're not in edit mode. What I'm trying to say is that the separate registry should only be used when we click "edit" to edit the content of the shared block and not wrap the entire BlockEdit of shared blocks with a RegistryProvider.

Unfortunately this doesn't work when previewing e.g. a shared columns block because <InnerBlocks> assumes that we have the nested blocks available in editor state.

[youknowriad]

Unfortunately this doesn't work when previewing e.g. a shared columns block because assumes that we have the nested blocks available in editor state.

I guess I assumed that there's a two-way sync between both states:

<EmbedRegistry>
<EditorProvider onChange={updateSharedBlock} post={sharedBlock}>
</EditorProvider>
</EmbedRegistry>

Having this kind of API would be ace, not only for this shared block issue but also to reuse Gutenberg as a standalone editor (passing a value, and having changes persisted with onChange calls like any React component)

[slimmilkduds]

Any updates on this, could this be expected to get merged before 5.0?

[noisysocks]

Yes, it's something we want to get in.

I've been working on other tasks but hopefully can focus on this again soon. At the very least it would be good to deprecate the REST API endpoint which I think is the only breaking API change involved.

[aduth]

Splitting off the refactoring to remove WP_REST_Blocks_Controller in #10751

[youknowriad]

While I'd love for this to happen sooner rather than later. It doesn't seem ready for 4.2 (or close to be ready). Thoughs @noisysocks

[slimmilkduds]

Nooo, another month or something without converting reusable block templates with nested blocks (crying like a baby)

[noisysocks]

While I'd love for this to happen sooner rather than later. It doesn't seem ready for 4.2 (or close to be ready). Thoughs @noisysocks

Agreed.

I'll go further and say that I'd be OK with punting this work to Phase 2 because:

• There's quite a lot left to do here.
• There are other higher priority tasks and fixes we need to do for WordPress 5.0.
• The remaining work here only affects the client and can be done in a backwards compatible way. (The only part of this PR that I think was essential to get into WordPress 5.0 was the changes to WP_REST_Blocks_Controller which @aduth made in REST API: Use posts endpoint for reusable blocks #10751.)
• It aligns with Phase 2's focus which is to expand Gutenberg to enable broader site customisation using e.g. templates and reusable blocks.

[aduth]

My bigger issue at this point is the inclusion of related actions and selectors as part of the public API, and whether that will be something we can deprecate after a phase one.

Maybe need a pass at prefixing __experimental to APIs we don't want to hang around, but can't address for the initial launch.

[noisysocks]

That's a good point. I should have bandwidth this week to replace the reusable block selectors and actions with @wordpress/core-data methods. It should be straightforward now that #10089 is in. Failing that, __experimental is a good fallback plan.

edit: 🤦‍♂️ Wasn't thinking straight—we can't use the data methods unless we also swap out ReusableBlockEdit to use a nested editor because using the data methods requires that we don't store parsed reusable blocks in blocksByClientId.

[noisysocks]

Closing this PR for now—will split it out into smaller PRs going forward.

[ezkitcabs]

So how has this been resolved? Every page of my site has reusable blocks of images that I cannot access now. I do not know how this happened in my case, I was editing and working with blocks. I think that one of my pages has a section with the old editor and this caused it? IDK, the blocks are still "there" I'm not a tech person, I just barely know how to do this. Help.

[aduth]

@ezkitcabs I'm not familiar with the issue you describe, and the effort in this pull request was not included in any WordPress release, nor do I think it would be related to what you're describing.

For general troubleshooting and support, your best option will be to search / post to the WordPress support forum. If after troubleshooting it can be determined there's a bug in the block editor, a new issue should be created in this repository detailing the steps to recreate the issue.