Don't force iframe editor when gutenberg plugin and block theme are enabled #65372
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
|
Size Change: -36 B (0%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
t-hamano
added
Gutenberg Plugin
t-hamano
requested review from
Mamaduka,
youknowriad,
ellatrix,
stokesman and
fabiankaegy
|
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. |
fabiankaegy
requested changes
Sep 16, 2024
There was a problem hiding this comment.
I do think we want to keep the isBlockBasedTheme check to ensure that any block based theme always gets the iframe in all cases.
My rationale for this is that block based themes already force the iframe at all times in the site editor. So it would be odd to go back and not force it for content.
In my mind we should only remove the isGutenbergPlugin part from the check
|
@fabiankaegy Thanks for the feedback!
Do you mean the following conditional statement? I think such a change would have a very big impact when this PR ships to core. For example, plugin developers may intentionally pin their blocks' API version to I think we need to ensure that the editor is not iframed if all blocks are not v3 for now. See the documentation here. This PR aims to make the editor behavior consistent whether the Gutenberg plugin is enabled or not. It does not extend the coverage of the iframe editor. Could such a suggestion be considered in a follow-up? |
|
@t-hamano I'll defer to @ellatrix and @youknowriad on this :) My understanding was always that block themes just always get the iframe. Regardless of the API version. Because the block are already broken in the site editor which currently only exists in an iframe. And we as core want to push for the iframe because it is the better solution. I agree however that the messaging around V3 of the block API made this a lot more confusing. Jetpack for example still uses API version 1 in a lot of their stuff because they never got around to update their blocks for the impactful V2 update. But all of their blocks support the iframed site editor just fine. You can see some of that convo here: Automattic/jetpack#34120 |
stokesman
approved these changes
Sep 16, 2024
There was a problem hiding this comment.
Right now, in the Post editor, whether the theme is block or not has no impact on iframing (unless the Gutenberg plugin is active). This just makes the plugin congruent. As I see it, it removes a source of confusion/trouble in testing and otherwise has no impacts.
|
I've removed my request for changes as I agree what I'm suggesting is more of a change where as what is in this PR today is a fix :) So we can get this in and then argue about what it should be like going forward separately 👍 |
fabiankaegy
approved these changes
Sep 16, 2024
|
Thank you all for your reviews. In WP6.7, the editor will work as an iframe even if meta boxes are present, which will have some impact on developers. Based on their feedback, we can reconsider whether always to enable iframes in block-based themes. |
|
Hey folks, I'm not sure I understand or agree with this change. This condition was there as a way to let users know that at some point in Core, all block themes will be iframed by default regardless of the other conditions. So this change feels backwards to me. Instead of moving towards more iframing, we're going back to less iframing. |
|
@youknowriad Thanks for the feedback. I agree that in the future block themes should iframe the editor by default. However, having the editor behave differently depending on whether the Gutenberg plugin is enabled seems confusing and hard to test. Even taking this into account, should we revert this PR? |
|
I understand, it's true that the plugin behaves differently than Core but it's kind of the same like all the features built on the plugin and that at some point make its way into Core. (Alpha). So I consider this check as "alpha" basically. Personally, I would be in favor of reverting |
|
I see, I'll submit a PR to revert this change. In the future, we might be able to consider just removing the |
t-hamano
added a commit
that referenced
this pull request
Sep 17, 2024
…me are enabled (#65372)" (#65402) This reverts commit 99fefd7. Co-authored-by: t-hamano <[email protected]> Co-authored-by: fabiankaegy <[email protected]> Co-authored-by: youknowriad <[email protected]>
|
Just to note it seems like the reason to do this was to make testing more obvious and avoid shipping bugs #64247 (comment) and that’s not a bad thing. I can see the reason to keep it as is though. Perhaps there’s another condition we could add so that for Gutenberg development it acts like it’s not the plugin 🤔. Or we can just strive to test scrupulously. |
|
I would like to raise a little bit of a discussion about the future direction. In the future, will the editor always default to an iframe, i.e. will the In that case, I am concerned that Block API version 3 and version 2 will essentially be the same. |
Yes, I think we should strive for that.
Yes, I'm not sure that's an issue though, the version 3 was created for that specific reason, to give block authors some time to migrate and test the iframe. What do you have in mind here? |
I just felt a little strange that the API versions were different but worked exactly the same 😅 Other than that, I have no concerns. |
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.
Related to #48286
What?
This PR removes the special condition for the Gutenberg plugin and block themes from the
useShouldIframehook that determines whether the editor should work as an iframe.Why?
Whether the editor works as an iframe depends on whether the Gutenberg plugin is enabled.
For example, if all registered blocks are not v3 and a block theme is enabled, the results will be different as follows:
This difference is especially problematic when testing WordPress beta/RC versions.
As seen in #64351, in order to move forward with a fully iframed editor, we need to unify the editor behavior whether Gutenberg is enabled or not.
How?
Remove
( isGutenbergPlugin && isBlockBasedTheme ) ||from the conditional statement (Take a look at this Diff view). This change does not affect the behavior of the editor in core.When this PR is merged and a new version of the Gutenberg plugin is shipped, the only time it will be affected is if all of the following conditions are met:
In this case, the editor that previously worked as an iframe will no longer work as an iframe.
Testing Instructions