[Site Editor]: Set html block as freeform fallback block
#48129
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ntsekouras
added
[Type] Bug
ntsekouras
requested review from
mcsf,
youknowriad,
jsnajdr,
michalczaplinski and
dmsnell
|
Size Change: -1 B (0%) Total Size: 1.33 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in fc4654f. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4240969123
|
mcsf
reviewed
Feb 16, 2023
| ( { name } ) => name !== 'core/freeform' | ||
| ); | ||
| registerCoreBlocks( coreBlocks ); | ||
| dispatch( blocksStore ).setFreeformFallbackBlockName( 'core/html' ); |
There was a problem hiding this comment.
What happens in the Widget editor and other spinoffs? 😉
This feels hacky and exposes other underlying fragilities:
registerCoreBlocksimplicitly callssetFreeformFallbackBlockNameand then we override that here by callingsetFreeformFallbackBlockNameagain.registerCoreBlocksaccepts an optional argument to override the default selection of block types to register. However, it doesn't take the (IMO necessary) precaution to check thatcore/freeformis 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,
registerCoreBlocksalready 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
blocksarray - ... and then we'll see what is left to do :)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
initializeEditorfunction. 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_settingsand ending in the client ininitializeEditor). 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
registerCoreBlockscame because that function was already making implicit choices (and doing them poorly, since it wasn't properly checking forwindow.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.
- Don't try to be smart in
Thoughts?
There was a problem hiding this comment.
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.
ntsekouras
commented
Feb 16, 2023
ntsekouras
force-pushed
the
update/freeform-fallback-to-html-block-in-site-editor
branch
from
de6afd1
to
4daf31c
Compare
ntsekouras
force-pushed
the
update/freeform-fallback-to-html-block-in-site-editor
branch
from
4daf31c
to
fc4654f
Compare
ntsekouras
added
the
Backport to WP 6.7 Beta/RC
|
An additional test scenario that I tried:
Expected:
|
ntsekouras
deleted the
update/freeform-fallback-to-html-block-in-site-editor
branch
|
@ntsekouras @mcsf @dmsnell I just realized this PR existed and went to test the following use case: add an empty HTML comment (meaning no content as a child). My expectation was that the comment would be ignored. Instead, it was converted to a block. Steps:
<!--
title: Title for the template or part
area: related-area
-->
Would you think there's a way to follow up this PR and ignore HTML comments with no content? While the issue reported caused a bug, the original intent was that the comments would be ignored, so we can attach metadata to the template via HTML comments, as in #38984 |
|
@oandregal: yeah, we fixed the worst part, which was the crash, but the rendering of comments is still something to address. Could you turn your comment into an issue and link it to #47212? 🙏 IMO, this part will be the hardest to fix if we want to fix it properly. |
|
Issue at #48408 |
ntsekouras
added a commit
that referenced
this pull request
Feb 27, 2023
|
I just cherry-picked this PR to the wp/6.2 branch to get it included in the next release: abf45f5 |
ntsekouras
removed
the
Backport to WP 6.7 Beta/RC
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Resolves: #47212
This PR does two things:
core/htmlblock as a fallback freeform fallback block in site editor(this is the resolution of the issue)__unstableCanInsertBlockTypeforclassicblock and achieves the same behavior by not registering it.Testing Instructions for
htmlblock fallback<p>Hello World</p>in a block themehtmlblocks are used for the above.Testing Instructions for
classicblock not available in site editorclassicblock