Skip to content
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

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

Merged
merged 13 commits into from
Jul 9, 2021
+29 −3
Merged

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

Show file tree
Hide file tree
Changes from all commits
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
14 changes: 14 additions & 0 deletions lib/client-assets.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,20 @@ function gutenberg_override_script( $scripts, $handle, $src, $deps = array(), $v
$output = sprintf( "wp.i18n.setLocaleData( { 'text direction\u0004ltr': [ '%s' ] }, 'default' );", $ltr );
$scripts->add_inline_script( 'wp-i18n', $output, 'after' );
}

/*
* Wp-editor module is exposed as window.wp.editor.
* Problem: there is quite some code expecting window.wp.oldEditor object available under window.wp.editor.
* Solution: fuse the two objects together to maintain backward compatibility.
* For more context, see https://github.com/WordPress/gutenberg/issues/33203
*/
if ( 'wp-editor' === $handle ) {
$scripts->add_inline_script(
'wp-editor',
'Object.assign( window.wp.editor, window.wp.oldEditor );',
'after'
);
}
}

/**
Expand Down
18 changes: 15 additions & 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,20 @@ describe( 'WP Editor Meta Boxes', () => {
( textarea ) => textarea.value
);

expect( content ).toMatchInlineSnapshot(
`"<p>Typing in a metabox</p>"`
);
/*
* `content` may or may not contain the <p> tags depending on hasWpautop value in this line:
* https://github.com/WordPress/wordpress-develop/blob/2382765afa36e10bf3c74420024ad4e85763a47c/src/js/_enqueues/vendor/tinymce/plugins/wordpress/plugin.js#L15
*
* Now, for the purposes of this e2e test we explicitly set wpautop to true in the test plugin:
* https://github.com/WordPress/gutenberg/blob/3da717b8d0ac7d7821fc6d0475695ccf3ae2829f/packages/e2e-tests/plugins/wp-editor-metabox.php#L36
*
* If this test randomly fails because of the actual value being wrapped in <p> like <p>Typing in a metabox</p>, it means that
* hasWpautop has been errorneously set to false in the line above. You may want to check:
* * Is window.wp.editor.autop a function? It should be one since https://github.com/WordPress/gutenberg/pull/33228
* * Is wpautop still true in the second link mentioned in this comment?
*
* For more context, see https://github.com/WordPress/gutenberg/pull/33228/files#r666897885
*/
expect( content ).toBe( 'Typing in a metabox' );
Copy link
Contributor

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

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

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

} );
} );