Skip to content

[Site Editor]: Set html block as freeform fallback block #48129

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ntsekouras merged 1 commit into from
Feb 23, 2023
Merged

[Site Editor]: Set html block as freeform fallback block #48129

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 6 additions & 21 deletions packages/edit-site/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import { store as blocksStore } from '@wordpress/blocks';
import {
registerCoreBlocks,
__experimentalGetCoreBlocks,
__experimentalRegisterExperimentalCoreBlocks,
} from '@wordpress/block-library';
import { dispatch } from '@wordpress/data';
Expand All @@ -16,7 +17,6 @@ import {
import { store as editorStore } from '@wordpress/editor';
import { store as interfaceStore } from '@wordpress/interface';
import { store as preferencesStore } from '@wordpress/preferences';
import { addFilter } from '@wordpress/hooks';
import { registerLegacyWidgetBlock } from '@wordpress/widgets';

/**
Expand All @@ -41,32 +41,17 @@ export function initializeEditor( id, settings ) {
settings.__experimentalFetchRichUrlData = fetchUrlData;

dispatch( blocksStore ).__experimentalReapplyBlockTypeFilters();
registerCoreBlocks();
const coreBlocks = __experimentalGetCoreBlocks().filter(
( { name } ) => name !== 'core/freeform'
);
registerCoreBlocks( coreBlocks );
dispatch( blocksStore ).setFreeformFallbackBlockName( 'core/html' );
Copy link
Contributor

@mcsf mcsf Feb 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens in the Widget editor and other spinoffs? 😉

This feels hacky and exposes other underlying fragilities:

  • registerCoreBlocks implicitly calls setFreeformFallbackBlockName and then we override that here by calling setFreeformFallbackBlockName again.
  • registerCoreBlocks accepts an optional argument to override the default selection of block types to register. However, it doesn't take the (IMO necessary) precaution to check that core/freeform is actually on the list of registered blocks before setting it as the fallback.
  • This solution, as I mentioned, requires all editors instances to do this kind of failure checking.
  • Supposedly, registerCoreBlocks already does some checking (if ( window.wp && window.wp.oldEditor ) { ), but it is clearly insufficient.

So I would start from that condition:

  • Ensure it's actually testing for the presence of TinyMCE
  • Ensure that it doesn't set Freeform as the fallback if it's not included in the blocks array
  • ... and then we'll see what is left to do :)

Copy link
Contributor Author

@ntsekouras ntsekouras Feb 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. I didn't think about this check window.wp?.oldEditor too much and had the impression that it was not defined in site editor, but what we actually checked was only if it was a WP environment.

In general, registerCoreBlocks probably does more than it should IMO and we should better handle this separately. At first I thought about adding an option object that the grouping, default, etc.. blocks could be set. The problem is bigger though, as we should not only have guards here about these blocks, but also in other actions like unregisterBlock, etc.. because we rely in many places about these blocks to be set.

Copy link
Member

@dmsnell dmsnell Feb 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm probably more comfortable at the outset with calling setFreeformFallbackBlockName. @mcsf if not in a place like this, where would you expect people to call it?

it would make me just as happy to remove that entirely from state and instead have this statically set at editor boot.

Copy link
Contributor

@mcsf mcsf Feb 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'm not really liking the tight coupling in registerCoreBlocks, despite what I said. It's also arguably too big a change for something implicit; that could break stuff in third-party editors, etc.

if not in a place like this, where would you expect people to call it? it would make me just as happy to remove that entirely from state and instead have this statically set at editor boot.

Thinking aloud:

  • The closest thing we have to a "standard init interface" is each editor's initializeEditor function. As was Nik's intuition, this is the most natural place to do this sort of imperative configuration.
  • We also have editor settings (a two-part pipeline starting in the server as $editor_settings and ending in the client in initializeEditor). There's always the possibility of introducing optional settings for this kind of stuff instead. It's more declarative, but I'm also wary of introducing a setting when other APIs already exist (even if they are imperative).
  • I think my initial reflex to fix this directly in registerCoreBlocks came because that function was already making implicit choices (and doing them poorly, since it wasn't properly checking for window.tinymce). Well, what if we separate these two problems. That is:
    • Don't try to be smart in registerCoreBlocks. Instead of automatically falling back to other block types, just throw a loggable error ("Cannot set xyz as fallback block type: block type not found.").
    • Thus, go back to Nik's original approach and update all of core's non-post editors so that they explicitly set their freeform handler to HTML in initializeEditor.
    • Fix the Classic block so that it checks for the presence of TinyMCE instead of just blindly failing.

Thoughts?

Copy link
Member

@dmsnell dmsnell Feb 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it all seems fine to me. settings seems like the same general idea I had concerning "statically set at editor boot."

making implicit choices (and doing them poorly,

this is more or less why I'm fine with a raggy solution now, because I doubt it would very much change the existing situation or impede improvements.

if the site editor needs to replace the fallback block then it only needs to ensure that happens before a post is loaded.

getting this into a more declarative spot might take a little more thought, whereas the imperative approach already exists and announces itself basically as the way to do it right now.

hopefully it's clear I have almost no concern about how this change is implemented at this stage.

Copy link
Contributor Author

@ntsekouras ntsekouras Feb 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kept only my initial commit and it seems the other screens already did that(1, 2).

Maybe we could fix this as is part of 6.2 and handle in a follow up the better handling in registerBlocks? What do you think @mcsf ?

registerLegacyWidgetBlock( { inserter: false } );
if ( process.env.IS_GUTENBERG_PLUGIN ) {
__experimentalRegisterExperimentalCoreBlocks( {
enableFSEBlocks: true,
} );
}
/*
* Prevent adding the Clasic block in the site editor.
* Only add the filter when the site editor is initialized, not imported.
* Also only add the filter(s) after registerCoreBlocks()
* so that common filters in the block library are not overwritten.
*
* This usage here is inspired by previous usage of the filter in the post editor:
* https://github.com/WordPress/gutenberg/pull/37157
*/
addFilter(
'blockEditor.__unstableCanInsertBlockType',
'removeClassicBlockFromInserter',
( canInsert, blockType ) => {
if ( blockType.name === 'core/freeform' ) {
return false;
}
return canInsert;
}
);

// We dispatch actions and update the store synchronously before rendering
// so that we won't trigger unnecessary re-renders with useEffect.
Expand Down