Try: Aggressive TinyMCE deprecation #50387
Conversation
tyxla
added
[Status] In Progress
tyxla
force-pushed
the
try/aggressive-tinymce-deprecation
branch
from
5541250 to
e040dc2
Compare
|
Size Change: +172 B (0%) Total Size: 1.42 MB
ℹ️ View Unchanged
|
tyxla
force-pushed
the
try/aggressive-tinymce-deprecation
branch
2 times, most recently
from
6eaa1cb to
0687bc7
Compare
|
Flaky tests detected in e5ce4e9. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5442538841
|
tyxla
added
[Block] Classic
lib/client-assets.php
Outdated
| $gutenberg_experiments = get_option( 'gutenberg-experiments' ); | ||
| if ( | ||
| ! $gutenberg_experiments || | ||
| ! array_key_exists( 'gutenberg-no-tinymce', $gutenberg_experiments ) || |
There was a problem hiding this comment.
We're now loading editor as a dependency of wp-block-library only if the experiment is disabled and the "require tinymce" cookie query argument isn't present.
| async function markAsRequiringTinymce() { | ||
| const expiration = new Date( 'Dec 31, 2099 23:59:59' ); | ||
| document.cookie = `requiresTinymce=1; expires=${ expiration }; path=/`; | ||
| window.location.reload(); |
There was a problem hiding this comment.
@mtias we talked about displaying a popup to the user, but I reconsidered this as being too aggressive and invasive. With this approach, we're simply detecting usage of TinyMCE, and if there is any, we'll save a cookie and redirect, and on the next page load, the user will have the classic block and TinyMCE.
There was a problem hiding this comment.
We should be able to detect usage server side on the parse_blocks loop before hitting the client. Isn't that more straightforward?
There was a problem hiding this comment.
Well, I'm not sure about that - last time I checked, parse_blocks() would return an empty blockName for a classic block instance, and it returns the same if we have an empty space between blocks, too. Also, even if we were able to detect usage of a Classic block that way, we wouldn't be able to detect plugin usage that relies on the wp.tinymce global. Thoughts?
There was a problem hiding this comment.
@tyxla I don't know if that would be a problem on the server for two reasons:
- there is no "classic block" - it's implicitly "a block without a name" as a way of supporting raw HTML in a post that isn't block material. to that end we can be confident that a block without a name is a classic block because that's exactly the specification we made which determines if something is a classic block.
- if a block's inner contents is only whitespace then I don't see any reason to consider it a real block, moreso if it's just
\n\n. the editor ignores these blocks, so we can too on the server. even if a block has thirty newlines, several tabs, and spaces, it's still all one and the same in HTML.
so we could do something like this…
add_filter( 'render_block', function( $html, $block ) {
if ( empty( $block['blockName'] ) && ! empty( trim( $html ) ) ) {
wp_enqueue_script( … );
}
return $html;
} );There was a problem hiding this comment.
Makes sense to me. I'm happy to try it out for detecting a classic block usage. Stay tuned.
I still think we're still going to need the cookie approach on the client side for instances of plugins that rely on the tinymce globals - there's no way to catch those from the server.
There was a problem hiding this comment.
what about setting a getter on window.tinymce that reloads the page with extra args to request it? no need to opt-in, and for those situations where we guessed wrong while being aggressive, we can correct that with a page refresh
if (!('tinymce' in window)) {
Object.defineProperty(window, 'tinymce', {
get() {
reloadPageWithMCE()
}
});
}
There was a problem hiding this comment.
what about setting a getter on
window.tinymcethat reloads the page with extra args to request it? no need to opt-in, and for those situations where we guessed wrong while being aggressive, we can correct that with a page refresh
I'd like to see this tested too
There was a problem hiding this comment.
Sounds like a plan 👍 I wasn't very fond of the cookie solution either, so while I wanted to try with the block manager, this won't do (see here for more details), so I'll go with a query argument.
The only downside of that is that we might end up needing some extra effort to persist that query argument, but then again, we won't have to worry about cleaning it up either, so it might end up being quite a solid approach.
Will work on it and update y'all.
| * | ||
| * @access public | ||
| */ | ||
| final class _WP_Editors { |
There was a problem hiding this comment.
Much of the TinyMCE assets are hardcoded to be loaded and not enqueued in a pluggable way. So we're creating a new class and requiring it conditionally, in which case it just doesn't load them anymore.
There was a problem hiding this comment.
Not sure I understand this part? How does loading this class prevents TinyMCE from loading?
There was a problem hiding this comment.
This, by default, is a core WP class that takes care of generating and "enqueueing" the editor assets. The problem is that for some of the scripts it doesn't use the traditional way to enqueue scripts, rather it will just output a huge blob of them inline, see:
So the only way to not spit that blob of scripts out is to override this class. This is actually an optimization because none of what it does will even get enqueued that way (look around the class I linked above, it also enqueued some assets), so we won't need to dequeue them either.
Luckily, redeclaring the class is possible because of the way that we check if the class is declared before being required, for example:
and:
I realize this is a bold approach, but it makes it so straightforward to remove all editor assets that would otherwise be either enqueued or plainly spit out as inline scripts in the admin. This is one of the main reasons why I prefer having this whole thing wrapped as an experiment before releasing it to the public.
| @@ -98,7 +98,9 @@ function gutenberg_enable_experiments() { | |||
| if ( $gutenberg_experiments && array_key_exists( 'gutenberg-theme-previews', $gutenberg_experiments ) ) { | |||
| wp_add_inline_script( 'wp-block-editor', 'window.__experimentalEnableThemePreviews = true', 'before' ); | |||
| } | |||
|
|
|||
| if ( $gutenberg_experiments && array_key_exists( 'gutenberg-no-tinymce', $gutenberg_experiments ) ) { | |||
| wp_add_inline_script( 'wp-block-library', 'window.__experimentalDisableTinymce = true', 'before' ); | |||
There was a problem hiding this comment.
This is the global variable that we use to pass the experiment to the client side.
There was a problem hiding this comment.
What's the thinking here? making this an experiment for some time to see the impact? What about removing the experiment but leaving this as a plugin-only (no backport to core) feature until we're certain.
There was a problem hiding this comment.
Yes, I prefer adding it as an experiment just in case, so we can further polish it without breaking the experience for anyone. I've also explained some of the rationale here. After I have more confidence, I'm happy to remove the experiment wrapper altogether.
|
I haven't managed to make any progress on this one this week, but I'm planning to resume work on it next week. |
tyxla
force-pushed
the
try/aggressive-tinymce-deprecation
branch
from
0687bc7 to
514605e
Compare
|
Update: the PR is in a better state now:
After refreshing, the user will see the Classic block as expected. We're using that use case as a last resort to have them convert to an HTML block. At that time they can also convert the HTML to blocks. We could also add a "Convert to blocks" button next to the "Keep as HTML" button if that will help. I still need to test thoroughly and see if there are any edge cases to fix, but I'm happy to accept any feedback already. |
tyxla
force-pushed
the
try/aggressive-tinymce-deprecation
branch
from
baa1535 to
f3d0291
Compare
| * @package gutenberg | ||
| */ | ||
|
|
||
| // add_action( 'admin_footer', 'gutenberg_test_tinymce_access' ); // Uncomment the following line to force an external TinyMCE usage. |
There was a problem hiding this comment.
Uncomment this line to force usage of TinyMCE. That can be used to test the cookie query argument logic.
Does this show the HTML preview of the raw HTML or only show the error box? It can be very disorienting when all you get is the error box, because it removes the context of knowing what you're working with. |
Good question - yes, it shows a preview below the error box - see below. Typing in the code editor: Switching to the visual editor: |
tyxla
added
Needs Design Feedback
|
I'm pretty satisfied with the state of the PR at this point, I think it's a good time to get some more feedback 😉 |
tyxla
force-pushed
the
try/aggressive-tinymce-deprecation
branch
from
7b3ddac to
d448cac
Compare
| * @package gutenberg | ||
| */ | ||
|
|
||
| // add_action( 'admin_footer', 'gutenberg_test_tinymce_access' ); // Uncomment the following line to force an external TinyMCE usage. |
There was a problem hiding this comment.
I'm going to keep that for now, just in case we'd like to test 3rd party global usage.
|
Looking forward to seeing the performance impact of this one! |
Me too! In the meantime, since this is an experiment, I'm planning to run perf tests on it in #52245. |
|
We already have this warning on console. Is due to disabling tinymce?
|
|
@c4rl0sbr4v0 this is likely related - that's exactly why this work is an experiment, so one could be able to disable it and resolve the warning. In the meantime, it would be really helpful to know more about:
Thanks in advance! |
On page and post edit.
Just opening a console inside a post or a page edit.
The one from wp-env. 6.3-alpha-56087/
Just Gutenberg in trunk. |
|
@c4rl0sbr4v0 thanks for providing more information, however I'm unable to reproduce, so I have a few more questions:
|
|
I tried again this morning and now I'm unable to reproduce too.
I was, I updated this morning.
Yep
I didn't know there was such option 😅 . Now I enabled/disabled and the error does not appear.
I will do if a get the error again. Sorry for the trouble! |
|
Thank you for the extra information @c4rl0sbr4v0, I really appreciate it! I tried deleting the option altogether to get to the state you were in before, but couldn't repro it even that way. Please don't hesitate to ping me if it occurs again! |
|
Following up to this with #52325 that makes it more interoperable with some plugins (like the Classic Editor) |
What?
This PR attempts to experiment with an aggressive strategy for deprecating TinyMCE.
I'd love to get some feedback from everyone!
Why?
The end goal is to fully deprecate the Classic block and all TinyMCE assets because they end up adding a lot of burden to the loading experience. With that approach, eventually, we could fully remove TinyMCE one day.
How?
The PR suggests that we wrap the functionality in a Gutenberg Experiment so that everyone can enable it to opt-in.
The experiment disables loading any TinyMCE assets and the classic block by default.
Then, if TinyMCE usage is detected, it refreshes the page and adds an additional query argument. On the subsequent page load, all assets are loaded, classic block as well.
That way, a new installation, or an installation that doesn't utilize tinyMCE will never have to load its assets or see the Classic block anymore.
Furthermore, we detect if the post has a classic block in the content and make sure to load it in that case.
Finally, we detect that the user just inputs some raw HTML in their editor, in which case we alter the messaging to suggest the user either convert to an HTML block (from where they can convert to blocks) or refresh the page if they insist on using the Classic block.
Testing Instructions
trunk./lib/experimental/disable-tinymce.phpand uncomment the 8th line by removing the leading//.requiresTinymcequery argument set.Testing Instructions for Keyboard
None
Screenshots or screencast
The message one sees when they edit their post in "code editor", they add some raw HTML and they switch back to the visual editor: