-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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.
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
Fix the styles compatibility hook for the editor iframes #40842
Conversation
Size Change: +2 B (0%) Total Size: 1.23 MB
ℹ️ View Unchanged
|
I can confirm that the fix resolves the issue for me when using |
Test Report: Fix comment query Loop's duplicate inspector controlsEnv:
Steps to reproduce:
Before applying the PR on the default page:
After applying the PR: Findings
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested this using the following within a test Plugin I made:
function myplugin_enqueue_block_editor_assets() {
wp_enqueue_style(
'davesmith-editor',
plugins_url( 'test-editor.css', __FILE__ ),
array( 'wp-edit-blocks' )
);
wp_enqueue_style(
'davesmith-block',
plugins_url( 'test-block.css', __FILE__ ),
array( 'wp-edit-blocks' )
);
}
add_action( 'enqueue_block_editor_assets', 'myplugin_enqueue_block_editor_assets' );
The test-block.css
file contains the following:
.wp-block-paragraph {
color: red !important;
}
On trunk it doesn't work in the Site Editor (i.e. the paragraph block isn't red).
On this branch it works perfectly and the CSS is applied within the Site Editor iframe. See the screenshot and video.
Screen.Capture.on.2022-05-05.at.12-39-07.mp4
Great work 🙇 👏
I have just tested thoroughly with this fix + 5.9.3 and on 6.0 RC1. I have also tested on Chrome and Safari. Based on my testing, #40562 and #40469 are both resolved with this PR. Amazing work, thanks @youknowriad 🙌 Here are some examples using the Aino theme and Aino Blocks. Before: Styles are not applied to the custom blocks in the patterns. After: Styles are applied and patterns appear correctly. Here is another example using the Block Visibility plugin, which uses Before: Contextual indicator is missing, it should appear on the Post Title. After: Contextual indicator is now visible. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on my thorough testing, this PR fixes both issues that it was designed to fix. Based on that, I am approving. I will leave the code review to others as that's outside my area of expertise 😅
✅ Fixes issue with Do we need to add a note somewhere that only files that contain the block prefix ( @ndiego, what version of Aino Blocks are you using? I'm getting an error when trying to test with it. Cc @ellenbauer |
I have exactly the same issue when opening the site editor with Aino theme installed and activated from the Theme Directory using the WP Admin interface. |
const BODY_CLASS_NAME = 'editor-styles-wrapper';
const BLOCK_PREFIX = 'wp-block'; It's also styles that contain |
@Mamaduka this was also the case before. I would love to see this removed in the future because it hampers extenders, but that is likely a separate PR. |
I am not seeing that error. I am seeing some block validation errors, but that is not related to this PR. I'm using the latest version of Aino and Aino Blocks plus the 6.0 RC1 and this PR. |
I haven't been able to check with the Aino plugin either, it breaks the editor as soon as I open the inserter but that seems separate. I'm pretty confident that this brings back the old behavior. |
Thanks, everybody, for the quick turnaround ❤️ Can we backport this PR to Gutenberg 13.2 and release another RC? cc @georgeh |
13.2-rc2 has been published with this fix |
Thank you for such great work with fixing this. First Class! |
#40603 is confirmed fixed with this PR, awesome! |
I cherry-picked this PR for WordPress 6.0 RC2 release with b4fa92c. |
Based on feedback (see above) received in https://wordpress.slack.com/archives/C02QB2JS7/p1653450177962219 it might be that we need to find a way to amend this so that the styles end up in the |
@getdave @youknowriad Created a PR so these styles can end up in the head once again: #46732. |
closes #40562
closes #40469
What?
There was a regression related to the styles loading in the iframes (previews, site editor...) in Gutenberg 13.0.
I believe the regression happened as a result of this PR #38855 but the reality is that the bug pre-existed and was hidden.
How?
We had a function that was copying styles from the parent document to the iframe and using
appendChild
to inject the cloned styles, the problem is that the parent element (the head element) was being controlled by React. Meaning its content can get wiped out at any moment due to re-rendering/remounting... (which I think has been triggered by #38855)This PR refactors how we apply the "cloning of styles" from the parent document by injecting these styles into a "div" at the top of the body and that div is "uncontrolled", meaning React doesn't care about its content. Also, I'm using
useRefEffect
to retrigger the compatibility hook anytime the reference of that div changes. (It doesn't happen now but it's a security for the future).Testing Instructions
1- Add some stylesheet at the root of gutenberg call it
test.css
and put the following content.wp-block-group { background: red !important; }
.2- Add the following code to the gutenberg.php file
3- Load the site editor and notice that group blocks have a red background.