Block Editor: Use default layout type when block specifies unknown value #39232
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
8 tasks
|
Size Change: +21 B (0%) Total Size: 1.32 MB
ℹ️ View Unchanged
|
dmsnell
force-pushed
the
fix/fallback-on-default-layout
branch
from
62f666c to
e282980
Compare
scruffian
approved these changes
Apr 22, 2022
|
Any thoughts there @scruffian on if this is a proper fix vs. a coverup that will lead to corruption? I don't have a good enough feel for what could happen to layouts if we auto-revert to |
|
If the layout type isn't recognised then things are definitely going to be broken in the editor. The way I see it this will do its best to fix that, but since the change will only be in the editor, the user will have the opportunity to preview it before saving, so if it does cause any issues they should see them. It seems safe to me! |
|
That's a great point about this only existing inside the editor @scruffian, and I think it gives me enough confidence to push this in. It'll either crash inside the editor (what it does now) and you can see none of it, or it'll display wrong in the editor in which case you can try to fix it. The one sticking point is that the existing crashing behavior does preserve that attribute while picking the default layout I think will end up changing the attribute on save. If a block isn't in view when loading a post that could lead to the common situation of "I opened my post in the editor and now the post is broken." |
dmsnell
force-pushed
the
fix/fallback-on-default-layout
branch
from
e282980 to
f190b64
Compare
…lue. While working on #38923 with invalid blocks it happened that a JavaScript error surfaced when working with a block that specified an incorrect layout type. In the case at hand the block asked for the `flerx` alignment instead of the `flex` alignment. It's reasonable to expect blocks to call for unrecognized layouts, especially if a plugin introduces new layouts and then blocks are loaded without that plugin's support. When `getLayoutType` is unable to find a recognized layout it returns `undefined` which causes numerous issues with calling code that expects a valid layout. In this patch we're guarding for those cases and if no known layout can be found for the specified layout type then we return the default layout instead. This introduces data corruption so it could be that this solution is more of a coverup than a fix. There is no way however to ignore that layout and preserve the behavior while still editing the block. On the other hand a block should not fail validation due to an implementation bug in the core editor and if this fix truly resolves the issue we should find invalidated blocks and prevent editing anyway (unfortunately this is not the case in this patch).
dmsnell
force-pushed
the
fix/fallback-on-default-layout
branch
from
f190b64 to
c594237
Compare
Mamaduka
added
[Type] Bug
|
It seems we can rebase and merge this proposal. |
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
@Mamaduka please feel free to commandeer this. it’s obviously been a while since I touched it, but I think I never merged it because I was still uncomfortable with the fact that it would create a new situation in which opening a post (and doing nothing other than opening it) in the editor could break a post. I generally like to avoid that at all costs. |
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.
Description
While working on #38923 with invalid blocks it happened that a JavaScript
error surfaced when working with a block that specified an incorrect layout
type. In the case at hand the block asked for the
flerxalignment insteadof the
flexalignment.It's reasonable to expect blocks to call for unrecognized layouts,
especially if a plugin introduces new layouts and then blocks are loaded
without that plugin's support.
When
getLayoutTypeis unable to find a recognized layout it returnsundefinedwhich causes numerous issues with calling code that expectsa valid layout.
In this patch we're guarding for those cases and if no known layout can
be found for the specified layout type then we return the default layout
instead. This introduces data corruption so it could be that this solution
is more of a coverup than a fix. There is no way however to ignore that
layout and preserve the behavior while still editing the block. On the
other hand a block should not fail validation due to an implementation bug
in the core editor and if this fix truly resolves the issue we should find
invalidated blocks and prevent editing anyway (unfortunately this is not
the case in this patch).
Can you provide some insight into how this kind of problem should be resolved?
If this is a "right" approach I can finish the PR and make sure it's ready
to merge. If I'm handling this improperly I'm fine taking a different approach.
Testing Instructions
Edit a post and paste the following contents into the text editor to produce invalid markup.
Notice in
trunkthat the block has been replaced with an error message while in this branch it loads fine, though the rendering should be using the default layout type/alignemnt instead of the non-existentflerxas incorrectly specified in the block comment.Screenshots
The JS error produces the "This block has encountered an error and cannot be previewed" message. It's not a failure to validate the block; instead it's a JS exception thrown when rendering the block and that's the exception isolation boundary replacing the contents with that message.
Thanks to @scruffian for calling this error out
Types of changes
Checklist:
*.native.jsfiles for terms that need renaming or removal).