-
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
Modules API: Fix Interactivity not working on Classic Themes. #57396
Conversation
This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress. If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged. If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack. Thank you! ❤️ View changed files❔ lib/experimental/modules/class-gutenberg-modules.php |
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 installed twentyseventeen and was able to reproduce the issue with Gutenberg 17.3. With this PR, the lightbox works again with twentyseventeen.
// Prints the preloaded modules in the head tag in block themes. | ||
add_action( 'wp_head', array( 'Gutenberg_Modules', 'print_module_preloads' ) ); |
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'm not well versed in the modules code now, but it seems like this may be an issue. Won't all the module loading rely on import maps? In that case, wouldn't we need to move all the module loading (including the "preload" modules) to the footer?
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.
Let's prevent it by moving all scripts to the footer.
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.
It seems safer to move all the modules to the footer 👍
I'm doing some testing locally to better understand the issue and the implications, but I think this is OK to merge now.
Does |
I was able to confirm that before moving preloads to the footer, this would cause an issue with preloaded modules. If I check out f13a53a and add module preloads, I get the following errors in my browser console and interactivity is broken:
|
That's a good question… I suspect it depends on a lot of different things and I don't have the expertise to explore them all. This is only a hint to help performance. I would be surprised if this were harmful, at worst I think we're providing a performance hint that's irrelevant. Maybe we could skip printing preload modules altogether if they're in the footer, but that can be revisited. @sgomes wrote the post on this, so maybe has more insight to share 🙂
Modules still have to be enqueued, so nothing changes there regarding preload. The preload is only a hint to the browser saying "we'll need this, so fetch it so we've got it cached." |
I added a conditional to make the preload only in block themes, where the modules are in the head. Classic themes will have worse performance, but is still a fix for an experimental feature that we can improve in future Gutenberg versions / WordPress Core implementation. |
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 confirmed that it works correctly with TwentyTwenty-One.
This theme injects a few scripts with the wp_footer hook, but the scripts related to the interactivity API are output in the expected order.
<!doctype html>
<html lang="en-US" >
<head><head>
<body>
<!-- ... -->
<script type="importmap">{"imports":{"@wordpress\/interactivity":"http:\/\/localhost:8888\/wp-content\/plugins\/gutenberg\/build\/interactivity\/index.min.js?ver=6.4.2"}}</script>
<script type="module" src="http://localhost:8888/wp-content/plugins/gutenberg/build/interactivity/image.min.js?ver=6.4.2" id="@wordpress/block-library/image"></script>
<script>document.body.classList.remove("no-js");</script>
<script>
if ( -1 !== navigator.userAgent.indexOf( 'MSIE' ) || -1 !== navigator.appVersion.indexOf( 'Trident/' ) ) {
document.body.classList.add( 'is-IE' );
}
</script>
<script src="http://localhost:8888/wp-content/plugins/gutenberg/build/modules/importmap-polyfill.min.js" defer></script>
<!-- ... -->
</body>
</html>
I think we can ship this approach for now and explore better approaches in the future.
This reverts commit 3edb69f.
Sorry for all the changes, but, let's the browser fetch the module earlier, it doesn't need to parse the other script to find the dependency, so should give some perf improvement at best, and not really hurt at worst. Again, it fixes the issue on the next Gutenberg release, and can be iterated in next releases. |
Flaky tests detected in f77a1e9. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7338977851
|
I see. I confirmed that it continues to work with the classic theme 👍 |
What?
Fixes #57370 and Automattic/wp-calypso#85638
Why?
As @t-hamano spotted in #56143 (comment). In classic themes, blocks are rendered on
the_content
.If we move the import map and the enqueued modules to the footer. Lightbox is working again.
This can be a temporary workaround before moving to WordPress Core.
Testing Instructions