Skip to content

Merge conflicting wp.editor objects into a single, non-conflicting module #33228

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.

Sign up for GitHub

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

Merged
youknowriad merged 13 commits into from
Jul 9, 2021
Merged

Merge conflicting wp.editor objects into a single, non-conflicting module #33228

Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
01daa00
Export members of legacy window.wp.editor from the new window.wp.edit…
adamziel Jul 6, 2021
b644a7d
Add an explanatory comment
adamziel Jul 6, 2021
d880b51
Use optional chaining for editor/index.js exports
adamziel Jul 6, 2021
8d4a984
Default to an empty object on export from oldEditor
adamziel Jul 6, 2021
ae66155
Use an inline script instead of modifying the @wordpress/editor package
adamziel Jul 7, 2021
ed8f3a8
Lint
adamziel Jul 7, 2021
f4601f7
Move the inline script to gutenberg_override_script
adamziel Jul 8, 2021
723a9c8
Remove gutenberg_enqueue_wp_old_editor_compatibility_snippet
adamziel Jul 8, 2021
59db57c
Formatting
adamziel Jul 8, 2021
ae4b7ac
Adjust wp-editor-meta-box E2E test
adamziel Jul 8, 2021
48c1929
Only add inline script if the handle is wp-editor
adamziel Jul 8, 2021
7277ce9
Add explanatory comment for the e2e test
adamziel Jul 9, 2021
c714c89
Add even more context to that comment
adamziel Jul 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Adjust wp-editor-meta-box E2E test
  • Loading branch information
@adamziel
adamziel committed Jul 8, 2021
commit ae4b7ac16131944d2a29af47371949258123311c
4 changes: 1 addition & 3 deletions packages/e2e-tests/specs/editor/plugins/wp-editor-meta-box.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,6 @@ describe( 'WP Editor Meta Boxes', () => {
( textarea ) => textarea.value
);

expect( content ).toMatchInlineSnapshot(
`"<p>Typing in a metabox</p>"`
);
expect( content ).toBe( 'Typing in a metabox' );
Copy link
Contributor

@youknowriad youknowriad Jul 8, 2021

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?

Copy link
Contributor Author

@adamziel adamziel Jul 9, 2021 •
edited
Loading

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>&nbsp;</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.
  • 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.

Now, for the purposes of this test, we explicitly set wpautop to true in the test plugin here:

gutenberg/packages/e2e-tests/plugins/wp-editor-metabox.php

Line 36 in 3da717b

'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 .

Copy link
Contributor

@youknowriad youknowriad Jul 9, 2021

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 :)

Copy link
Member

@gziolo gziolo Jul 23, 2021

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 👍🏻

} );
} );