Metaboxes: Refactor to render inline without iFrame #3345

youknowriad · 2017-11-06T07:40:14Z

Description

This PR refactors the metaboxes rendering to drop the iFrame.

no iframe
no duplicate stylesheets
no link issues
no modal issues.
no accessibility issues

codecov · 2017-11-06T07:46:00Z

Codecov Report

Merging #3345 into master will increase coverage by 0.15%.
The diff coverage is 1.49%.

@@            Coverage Diff            @@
##           master   #3345      +/-   ##
=========================================
+ Coverage   33.15%   33.3%   +0.15%     
=========================================
  Files         249     249              
  Lines        6711    6683      -28     
  Branches     1206    1202       -4     
=========================================
+ Hits         2225    2226       +1     
+ Misses       3786    3761      -25     
+ Partials      700     696       -4

Impacted Files	Coverage Δ
editor/components/meta-boxes/index.js	`0% <0%> (ø)`	⬆️
...tor/components/meta-boxes/meta-boxes-area/index.js	`0% <0%> (ø)`
editor/actions.js	`43.39% <0%> (-0.84%)`	⬇️
editor/reducer.js	`90.27% <0%> (-0.5%)`	⬇️
editor/effects.js	`47.61% <12.5%> (-3.7%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9225541...7db33b9. Read the comment docs.

gziolo · 2017-11-06T09:18:56Z

I see the following PHP notice when loading the new post page:

Notice: Undefined variable: admin_body_class in /srv/www/wordpress-default/public_html/wp-content/plugins/lib/meta-box-partial-page.php on line 163

When I click on Group from Advanced Custom Fields nothing happens. When using Classic Editor it toggles the group properly.

It doesn't load WYSWIG editor:

Sometimes Google map is no longer rendered after changes are saved and WYSWIG editor area changes size.

WYSWIG editor works properly in the solution that uses iframe. What's interesting Group header isn't displayed in the iframe solution.

gziolo · 2017-11-06T09:51:00Z

editor/meta-boxes/meta-boxes-area/index.js

+	const head = document.getElementsByTagName( 'head' )[ 0 ] || document.documentElement,
+		script = document.createElement( 'script' );
+	script.type = 'text/javascript';
+	script.appendChild( document.createTextNode( data ) );


Should it have a different handling for scripts that have src attribute?

gziolo · 2017-11-06T10:48:02Z

editor/effects.js

 } from './selectors';

 const SAVE_POST_NOTICE_ID = 'SAVE_POST_NOTICE_ID';
 const TRASH_POST_NOTICE_ID = 'TRASH_POST_NOTICE_ID';

+// Hold jquery.ready until the metaboxes load
+jQuery.holdReady( true );


Should we guard it with a check for jQuery existence to make sure it works in test env, too?

Okey, you mocked it. It's okayish :)

The wp-editor script does not define a dependency on jQuery. We can't otherwise assume this global exists.

gutenberg/lib/client-assets.php

Lines 603 to 609 in 4da6c0f

wp_enqueue_script(

'wp-editor',

gutenberg_url( 'editor/build/index.js' ),

array( 'wp-api', 'wp-date', 'wp-i18n', 'wp-blocks', 'wp-element', 'wp-components', 'wp-utils', 'word-count', 'editor', 'heartbeat' ),

filemtime( gutenberg_dir_path() . 'editor/build/index.js' ),

true // enqueue in the footer.

);

Is it possible to move this into the effect for one of the initialization actions? Not sure I like the side-effect we're introducing to the top-level scope here. Maybe better to at least consolidate this somewhere, as in editor/index.js?

As I'm reviewing this, I'm still working through understanding why we need this; can you clarify more in a comment why we need to hold?

I'm still working through understanding why we need this; can you clarify more in a comment why we need to hold?

I believe we need to currently hold due to waiting on the areas to load, so that any scripts can properly add their listeners when jQuery is ready.

I don't think that is a long term need, now that we are on post.php we can render the metaboxes server side, then take those dom nodes and bring them into React.

tomjn · 2017-11-06T11:02:03Z

lib/meta-box-partial-page.php

-	<html class="gutenberg-meta-box-html">
-	<head>
+	</head>
+	<body class="wp-admin wp-core-ui no-js <?php echo $admin_body_classes . ' ' . $admin_body_class; ?>">


Missing escaping

This is the same code as the default admin_header. The idea is to have the same result. We may also remove it, I think it may be useless

As a security measure, esc_attr should have no effect here if that's the case, unless an attack is in progress. Instead of trusting it's safe, lets eliminate all doubt and move forward with absolute rock solid certainty by escaping properly.

Yeah, it is most likely useless now as we load on post.php

youknowriad · 2017-11-06T12:26:13Z

It doesn't load WYSWIG editor:

That's a downside for metaboxes relying on the global TinyMCE variables. We'll have to look deeper, but Gutenberg is not a TinyMCE instance and metaboxes relying on these globals break. We can try to mitigate the breakage in the future.

Precision: this is true no matter the approach

BE-Webdesign · 2017-11-06T14:18:24Z

Precision: this is true no matter the approach

Yup, this is pretty much the only unavoidable breakage.

BE-Webdesign · 2017-11-06T15:09:29Z

Not sure if this is just on my end, but the collapse and expanding headers for the sidebar do not appear to be working, the main area looks good though.

BE-Webdesign · 2017-11-06T15:30:29Z

What do you think about server rendering the meta boxes as well then hoist them into the React DOM? Maybe that would be a good next iteration? That way there is no spinning wheel on load.

youknowriad · 2017-11-06T15:33:34Z

What do you think about server rendering the meta boxes as well then hoist them into the React DOM? Maybe that would be a good next iteration?

Totally on board with this. It will also fix some JS issues I believe.

aduth · 2017-11-06T16:36:47Z

Example of stylesheets loaded in the same context causing issues:

Note alignment of publish caret; this is from a standard meta-box being enqueued with wp-includes/css/buttons.css (specifically the .wp-core-ui .button-primary styles).

aduth · 2017-11-06T16:20:17Z

assets/js/meta-box-resize.js

@@ -1,47 +0,0 @@
-( function() {


If this folder no longer exists, it should be removed from .eslintignore and build-plugin-zip.sh:

gutenberg/.eslintignore

Line 5 in 4da6c0f

/assets/js

gutenberg/bin/build-plugin-zip.sh

Line 96 in 4da6c0f

assets/js/*.js \

aduth · 2017-11-06T16:21:17Z

docs/meta-box.md

@@ -56,15 +48,10 @@ between the `MetaBoxIframe` and the Redux Store. *If no meta boxes are active,
 nothing happens. This will be the default behavior, as all core meta boxes have
 been stripped.*

-#### MetaBoxIframe Component


There are still two lingering references to MetaBoxIframe components in this README (in section "Redux and React Meta Box Management")

aduth · 2017-11-06T16:21:47Z

editor/actions.js

+ */
+export function metaBoxLoaded( location ) {
+	return {
+		type: 'METABOX_LOADED',


If we're treating "meta box" as two distinct words (evidenced by action creator name and other action types in this file), this action type should be META_BOX_LOADED

aduth · 2017-11-06T16:23:54Z

editor/effects.js

 } from './selectors';

 const SAVE_POST_NOTICE_ID = 'SAVE_POST_NOTICE_ID';
 const TRASH_POST_NOTICE_ID = 'TRASH_POST_NOTICE_ID';

+// Hold jquery.ready until the metaboxes load
+jQuery.holdReady( true );


The wp-editor script does not define a dependency on jQuery. We can't otherwise assume this global exists.

gutenberg/lib/client-assets.php

Lines 603 to 609 in 4da6c0f

wp_enqueue_script(

'wp-editor',

gutenberg_url( 'editor/build/index.js' ),

array( 'wp-api', 'wp-date', 'wp-i18n', 'wp-blocks', 'wp-element', 'wp-components', 'wp-utils', 'word-count', 'editor', 'heartbeat' ),

filemtime( gutenberg_dir_path() . 'editor/build/index.js' ),

true // enqueue in the footer.

);

aduth · 2017-11-06T16:25:22Z

editor/effects.js

 } from './selectors';

 const SAVE_POST_NOTICE_ID = 'SAVE_POST_NOTICE_ID';
 const TRASH_POST_NOTICE_ID = 'TRASH_POST_NOTICE_ID';

+// Hold jquery.ready until the metaboxes load
+jQuery.holdReady( true );


Is it possible to move this into the effect for one of the initialization actions? Not sure I like the side-effect we're introducing to the top-level scope here. Maybe better to at least consolidate this somewhere, as in editor/index.js?

As I'm reviewing this, I'm still working through understanding why we need this; can you clarify more in a comment why we need to hold?

aduth · 2017-11-06T16:30:03Z

lib/meta-box-partial-page.php

 	wp_enqueue_script( 'utils' );
 	wp_enqueue_script( 'common' );
 	wp_enqueue_script( 'svg-painter' );
+	?>
+	<!-- Add an html class so that scroll bars can be removed in css and make it appear as though the iframe is one with Gutenberg. -->


Reference to iframe.

aduth · 2017-11-06T16:30:52Z

lib/meta-box-partial-page.php

-	<script type="text/javascript">
-		document.body.className = document.body.className.replace('no-js','js');
-	</script>
-	<?php


Another comment below referencing iframes:

THIS IS SOMEHOW REALLY IMPORTANT FOR IFRAMES TO RESIZE CORRECTLY

aduth · 2017-11-06T16:32:24Z

lib/meta-box-partial-page.php

 	wp_enqueue_script( 'utils' );
 	wp_enqueue_script( 'common' );
 	wp_enqueue_script( 'svg-painter' );
+	?>
+	<!-- Add an html class so that scroll bars can be removed in css and make it appear as though the iframe is one with Gutenberg. -->
+	<html class="gutenberg-meta-box-html">


No styles exist anymore for this class name, we can drop it?

aduth · 2017-11-06T16:38:35Z

editor/meta-boxes/meta-boxes-area/index.js

+				if ( ! this.mounted ) {
+					return;
+				}
+				jQuery( this.node ).html( body );


Why do we use jQuery for this? this.node.innerHTML = body ?

(To earlier point that we don't have an explicit dependency on jQuery for the editor)

We need to run the scripts contained in body. jQuery does this pretty well. I tried recreating the behavior myself, it "mostly" worked but not 100%. Is there a specific lib to do so?

I would think innerHTML should be running any newly injected scripts.

Can't find official documentation about this, but it's not running the injected scripts. See https://stackoverflow.com/questions/2592092/executing-script-elements-inserted-with-innerhtml

aduth · 2017-11-06T16:39:32Z

editor/meta-boxes/meta-boxes-area/index.js

+				this.form = this.node.querySelector( '.meta-box-form' );
+				this.form.onSubmit = ( event ) => event.preventDefault();
+				this.originalFormData = this.getFormData();
+				this.form.addEventListener( 'change', this.checkState );


Need to be removed in componentWillUnmount?

youknowriad · 2017-11-06T16:42:01Z

(specifically the .wp-core-ui .button-primary styles).

We need to accommodate core stylesheets though

gziolo · 2017-11-13T09:09:49Z

I'm using Advanced Custom Fields plugins for testing. When I navigate from the posts list to edit a post using Gutenberg I see the following items on the very bottom of the page:

It looks like see this issue every time I load Gutenberg.

To give you a better context, it works perfectly fine when using the Classic editor.

Another issue I observed here is that when I update location on the map, it Save draft link doesn't appear next to Publish button. It works properly for other field types.

gziolo · 2017-11-13T09:22:51Z

lib/meta-box-partial-page.php

@@ -188,7 +217,6 @@ function gutenberg_meta_box_partial_page_admin_header( $hook_suffix, $current_sc
 		decimalPoint = '<?php echo addslashes( $wp_locale->number_format['decimal_point'] ); ?>',
 		isRtl = <?php echo (int) is_rtl(); ?>;
 	</script>
-	<meta name="viewport" content="width=device-width,initial-scale=1.0">


Shouldn't be viewport kept?

No this is not rendered at all, it's an AJAX request where we just render the body

It makes perfect sense :)

gziolo · 2017-11-13T10:52:13Z

I played a bit more with the map. I updated the locations several times and clicked Save draft a few times after updating another field. I see the following error on the console:

youknowriad · 2017-11-13T12:45:59Z

Ok, I'm merging this. Acknowledging this doesn't fix all the metaboxes issues. It could also create some new issues. But this gets us closer to where we want to go.

Future steps could include:

Prerendering metaboxes,
Migration path

StaggerLeee · 2017-11-16T04:13:03Z

Want just to report custom Metaboxes in right sidebar are near perfect, from design point of view.
I like how they look. Small corrections are needed but nothing important. Some things will third party developers need to adjust and correct of course.

Under Gutenberg content area is very different thing.
Maybe it is good idea to make this whole area with slight gray background, Metaboxes centered and let third party Metabox to chose full wide width, or smaller width centered.

To make third party developers more acceptable later when converting Metaboxes to Gutenberg blocks. Less work in the beginning if their old Metaboxes can be full width. They will have to do it twice, change a lot of code. First adapt it to look nice for transition period (compatibility fallback), second to code Metaboxes to be real Gutenberg blocks. Not small amount of coding and work.

mattiman2 · 2017-11-16T18:55:09Z

The regular metaboxes do show up. But are the data supposed to be saved as well? The normal function doesn't seem to work if you click the Update button
add_action( 'save_post', 'my_save_meta_function' ); function my_save_meta_function( $post_id ) { update_post_meta( $post_id, '_mgc_address', strip_tags( $_POST['_mgc_address'] ) ); }

youknowriad · 2017-11-16T19:02:22Z

@mattiman2 Probably a small bug somewhere, we'll look into it. A drop-in metabox example could help debug ;). Feel free to create an issue.

mattiman2 · 2017-11-16T19:38:24Z

@youknowriad OK I'll try to create an issue about this. Thanks.

aduth · 2017-11-27T20:24:57Z

Something in this pull request introduced a regression where images from my media library are no longer appearing:

If I had to guess, I'd think it might have something to do with the jQuery dependency changes?

Aside: This branch was very difficult to bisect effectively to find the start of this issue, because most of the commits in the branch would not compile until the "fix" in 6eec188 . The branch could have used some more squashing, or at least ensuring that each commit could independently compile without errors.

StaggerLeee · 2017-11-29T10:29:50Z

Listen, if You decided to put Metaboxes inside max-width: 636px then it is ugly as hell as it is now.

Give them 100% wrapper width, or do some nice boxed CSS styling. As it is now not so appealing.
And separate custom Metaboxex with some margins. They are stuck to each other and very difficult to get visual preview what any of them is doing and what part belongs to what. Specially when everything is with pure white background.

It is not written in constitution everything in WP backend has to have boring white background.
If you insist on white background then Metaboxes need to have different background, border, anything to make them get out from background.

So huge amount of energy and time You devote to smallest Gutenberg CSS and styling. Again it looks like you treat old Metaboxes as second class citizens.

mrleemon · 2017-12-15T09:45:28Z

@mattiman2 I'm having the same problem. Although the meta boxes show up correctly, changes in them are not saved. My custom function hooked to the save_post action is not being called or so it seems.

youknowriad self-assigned this Nov 6, 2017

gziolo reviewed Nov 6, 2017

View reviewed changes

tomjn suggested changes Nov 6, 2017

View reviewed changes

mtias added [Feature] Meta Boxes A draggable box shown on the post editing screen Gutenberg Plugin Issues or PRs related to Gutenberg Plugin management related efforts labels Nov 6, 2017

tomjn mentioned this pull request Nov 6, 2017

Are iframes a viable long-term solution for meta boxes? #3304

Closed

aduth reviewed Nov 6, 2017

View reviewed changes

youknowriad added 12 commits November 10, 2017 09:58

Metaboxes: Refactor to render inline without iFrame

d8d5eba

Metaboxes: No flickering on save

0ac886a

Metaboxes: Fix URL when saving new posts

7e439d1

Metaboxes: Hold initialization until the metaboxes load

76ea2e1

Metaboxes: Rely on jQuery to insert content and scripts

478e781

Metaboxes: Drop the body classname as useless in this approach

6bc0b75

Metaboxes: Fix publish button style when using metaboxes

1e0207e

Metaboxes: Add jquery dependency to wp-editor

4a44cca

Metaboxes: Remove lingering useless code or mentions of iframe

6961ba1

Metaboxes: unbind form events on unmount

c8cc737

Metaboxes: Rename META_BOX_LOADED action

7a65b3e

Metaboxes: Fix MetaBoxesArea component after rebase

7db33b9

youknowriad force-pushed the update/metabox-container branch from 948b0af to 7db33b9 Compare November 10, 2017 09:03

pento mentioned this pull request Nov 13, 2017

The content is locked by another user overlay is not shown correcly #3446

Closed

2 tasks

gziolo reviewed Nov 13, 2017

View reviewed changes

youknowriad merged commit 6eec188 into master Nov 13, 2017

youknowriad deleted the update/metabox-container branch November 13, 2017 12:46

pento mentioned this pull request Nov 29, 2017

Supporting Metaboxes #952

Closed

	wp_enqueue_script(
	'wp-editor',
	gutenberg_url( 'editor/build/index.js' ),
	array( 'wp-api', 'wp-date', 'wp-i18n', 'wp-blocks', 'wp-element', 'wp-components', 'wp-utils', 'word-count', 'editor', 'heartbeat' ),
	filemtime( gutenberg_dir_path() . 'editor/build/index.js' ),
	true // enqueue in the footer.
	);

Metaboxes: Refactor to render inline without iFrame #3345

Metaboxes: Refactor to render inline without iFrame #3345

Conversation

youknowriad commented Nov 6, 2017

Description

codecov bot commented Nov 6, 2017 • edited Loading

Codecov Report

gziolo commented Nov 6, 2017 • edited Loading

Choose a reason for hiding this comment

gziolo Nov 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BE-Webdesign Nov 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad commented Nov 6, 2017 • edited Loading

BE-Webdesign commented Nov 6, 2017

BE-Webdesign commented Nov 6, 2017

BE-Webdesign commented Nov 6, 2017

youknowriad commented Nov 6, 2017

aduth commented Nov 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad commented Nov 6, 2017

gziolo commented Nov 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo commented Nov 13, 2017

youknowriad commented Nov 13, 2017 • edited Loading

StaggerLeee commented Nov 16, 2017 • edited Loading

mattiman2 commented Nov 16, 2017

youknowriad commented Nov 16, 2017 • edited Loading

mattiman2 commented Nov 16, 2017

aduth commented Nov 27, 2017

StaggerLeee commented Nov 29, 2017 • edited Loading

mrleemon commented Dec 15, 2017

codecov bot commented Nov 6, 2017 •

edited

Loading

gziolo commented Nov 6, 2017 •

edited

Loading

gziolo Nov 6, 2017 •

edited

Loading

BE-Webdesign Nov 6, 2017 •

edited

Loading

youknowriad commented Nov 6, 2017 •

edited

Loading

aduth commented Nov 6, 2017 •

edited

Loading

gziolo commented Nov 13, 2017 •

edited

Loading

youknowriad commented Nov 13, 2017 •

edited

Loading

StaggerLeee commented Nov 16, 2017 •

edited

Loading

youknowriad commented Nov 16, 2017 •

edited

Loading

StaggerLeee commented Nov 29, 2017 •

edited

Loading