-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Merge conflicting wp.editor
objects into a single, non-conflicting module
#33228
Conversation
wp.editor
objects into a single, non-conflicting module
Size Change: +4.77 kB (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
This will like fix other issues, where other plugins have called |
I fear that this approach is not ideal. If I'm understanding this correctly, it basically means that the gutenberg I wonder, instead, if we could monkey-patch/proxy |
packages/editor/src/index.js
Outdated
// Problem: there is quite some code expecting another, legacy object available under window.wp.editor | ||
// Solution: export all the members of legacy window.wp.editor from this new module to maintain backward compatibility | ||
// For more context, see https://github.com/WordPress/gutenberg/issues/33203 | ||
export const { autop, removep, initialize, remove, getContent } = |
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 didn't even know this is valid JS 🤯
It's regrettable that we ever added My only issue with this approach is that it means we're adding five functions to |
There’s also this similar issue with the Makes me wonder if a “fix” could be to add code to Core which fires off |
Both deprecated and doing_it_wrong sounds great to me, let's absolutely add that. Still, we need to make sure the existing code keeps working for another version or two as we start deprecating things. A slightly different approach here would be to add another inline script like the one that creates
And use it to bolt these five functions on top of the “new” wp.editor immediately after it’s loaded. |
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.
Feels like this should be an inline script to wp-editor
instead of something in the package directly because it's very WP specific, wdyt?
Yup, let's try that – I'll publish a new PR soon |
Here's the inline-script approach: WordPress/wordpress-develop#1481 |
@adamziel that looks good to me, should we do the same in Gutenberg. |
@youknowriad my intuition says no because maybe then we'd run both code paths and end up having two inline scripts enqueued; also in Gutenberg there's no |
@adamziel we do support WP 5.7 and WP 5.6 for now in Gutenberg though and when 5.8 get released, we'd shift to support 5.7 and 5.8. |
@youknowriad Good point! I'll start a new PR for Gutenberg then |
795da26
to
59db57c
Compare
expect( content ).toMatchInlineSnapshot( | ||
`"<p>Typing in a metabox</p>"` | ||
); | ||
expect( content ).toBe( 'Typing in a metabox' ); |
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.
Is this change expected, I feel this test has been updated back and forth several times already in the last couple releases, do we understand why this change is needed?
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 turns out that TinyMCE's WordPress plugin does the following:
https://github.com/WordPress/wordpress-develop/blob/2382765afa36e10bf3c74420024ad4e85763a47c/src/js/_enqueues/vendor/tinymce/plugins/wordpress/plugin.js#L15
https://github.com/WordPress/wordpress-develop/blob/2382765afa36e10bf3c74420024ad4e85763a47c/src/js/_enqueues/vendor/tinymce/plugins/wordpress/plugin.js#L132-L133
https://github.com/WordPress/wordpress-develop/blob/2382765afa36e10bf3c74420024ad4e85763a47c/src/js/_enqueues/vendor/tinymce/plugins/wordpress/plugin.js#L593-L609
hasWpautop = ( wp && wp.editor && wp.editor.autop && editor.getParam( 'wpautop', true ) ),
editor.on( 'BeforeSetContent', function( event ) {
// ...
if ( hasWpautop ) {
event.content = wp.editor.autop( event.content );
} else {
// Prevent creation of paragraphs out of multiple HTML comments
event.content = event.content.replace( /-->\s+<!--/g, '--><!--' );
}
});
// ...
editor.on( 'SaveContent', function( event ) {
// ...
// Keep empty paragraphs :(
event.content = event.content.replace( /<p>(?:<br ?\/?>|\u00a0|\uFEFF| )*<\/p>/g, '<p> </p>' );
if ( hasWpautop ) {
event.content = wp.editor.removep( event.content );
} else {
// Restore formatting of block boundaries.
event.content = event.content.replace( /-->\s*<!-- wp:/g, '-->\n\n<!-- wp:' );
}
});
So:
- if
Object.assign( window.wp.editor, window.wp.oldEditor );
is in place, then hasWpauto is set to true by editor.getParam( 'wpautop', true ); .- When I switch from HTML to visual mode of TinyMCE, event.content is wrapped in a
<p>
- When I switch back from the visual mode to the HTML mode, SaveContent is dispatched and it calls wp.editor.removep which removes the
<p>
tags.
- When I switch from HTML to visual mode of TinyMCE, event.content is wrapped in a
- If Object.assign thing is not in place, then hasWpauto is set to false.
- When I switch from HTML to visual mode of TinyMCE, event.content is not wrapped in a
<p>
but TinyMCE wraps it anyway. - When I switch back from the visual mode to the HTML mode, SaveContent is dispatched and it does not call wp.editor.removep so the
<p>
tags are not automatically removed at that stage.
- When I switch from HTML to visual mode of TinyMCE, event.content is not wrapped in a
Now, for the purposes of this test, we explicitly set wpautop to true in the test plugin here:
'wpautop' => true, |
This means that we do not want to see the the <p>
tags in the editor. I added some documentation to that test to explain how to proceed in case it flips back again at some point @youknowriad .
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.
Thanks for the detailed explanation :)
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.
Good one @adamziel, this test should be finally reliable 👍🏻
We don't need to backport this PR as there's already a commit awaiting backport here: https://core.trac.wordpress.org/ticket/53437 |
@noisysocks going to backport this one because of issues in e2e tests in wp/5.8 branch |
Description
Solves #33203
Situation:
packages/editor
is exposed aswindow.wp.editor
in wp-adminProblem: there is quite some code expecting to see a different (legacy) object under
window.wp.editor
Proposed solution: assign all the members of legacy window.wp.editor to the new window.wp.editor like this:
This PR is a Gutenberg counterpart of WordPress/wordpress-develop#1481 in core
How has this been tested?
Screenshots
(Credits for the screenshot and the test plan to @spacedmonkey)
Types of changes
Bug fix (non-breaking change which fixes an issue)