Make WordPress Core

Opened 13 years ago

Last modified 3 years ago

#19173 reopened defect (bug)

Issues with wp_editor() when used inside of a meta box.

Reported by: goto10's profile goto10 Owned by: azaozz's profile azaozz
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 3.3
Component: Editor Keywords: has-patch needs-testing
Focuses: Cc:

Description

I ran into a couple of obscure issues when adding an editor inside of a meta box. Errors are generated when moving the meta box around and then switching between the Visual and HTML editors. Also, the resize handle (.mceResize) is a bit too low when viewing the Visual tab when our custom editor has been added within a meta box. Tested in FF7 7.0.1 WPv3.3beta2

I'm adding the Editor within a meta box because it's the easiest way to add an Editor field type within the meta box/ CPT plugin I've made. It seems like it will be a common use case.

Here's a simple plugin that adds an editor within a meta box.

<?php
/**
 * Plugin Name: WP Editor in a Meta Box
 * Plugin URI: 
 * Description: Demonstration of WP Editor in a meta box.
 * Version: 1.0
 * Author: goto10
 * Author URI: 
 * License: 
 */

// file name: wp_editor-in-meta-box-test.php 

/* Meta box code based on http://codex.wordpress.org/Function_Reference/add_meta_box */

/* Define the custom box */
add_action( 'add_meta_boxes', 'myplugin_add_custom_box' );

/* Do something with the data entered */
add_action( 'save_post', 'myplugin_save_postdata' );

/* Adds a box to the main column on the Post and Page edit screens */
function myplugin_add_custom_box() {
	add_meta_box( 'wp_editor_test_1_box', 'WP Editor Test #1 Box', 'wp_editor_meta_box' );
}

/* Prints the box content */
function wp_editor_meta_box( $post ) {

  // Use nonce for verification
  wp_nonce_field( plugin_basename( __FILE__ ), 'myplugin_noncename' );
	
	$field_value = get_post_meta( $post->ID, '_wp_editor_test_1', false );
	wp_editor( $field_value[0], '_wp_editor_test_1' );
}

/* When the post is saved, saves our custom data */
function myplugin_save_postdata( $post_id ) {
  
	// verify if this is an auto save routine. 
  // If it is our form has not been submitted, so we dont want to do anything
  if ( defined( 'DOING_AUTOSAVE' ) && DOING_AUTOSAVE ) 
      return;

  // verify this came from the our screen and with proper authorization,
  // because save_post can be triggered at other times
  if ( ( isset ( $_POST['myplugin_noncename'] ) ) && ( ! wp_verify_nonce( $_POST['myplugin_noncename'], plugin_basename( __FILE__ ) ) ) )
      return;

  // Check permissions
  if ( ( isset ( $_POST['post_type'] ) ) && ( 'page' == $_POST['post_type'] )  ) {
    if ( ! current_user_can( 'edit_page', $post_id ) ) {
			return;
		}		
  }
	else {
		if ( ! current_user_can( 'edit_post', $post_id ) ) {
			return;
		}
	}

  // OK, we're authenticated: we need to find and save the data
	if ( isset ( $_POST['_wp_editor_test_1'] ) ) {
		update_post_meta( $post_id, '_wp_editor_test_1', $_POST['_wp_editor_test_1'] );
	}
	
}

Case 1:

Steps to reproduce error:

Activate test plugin.
HTML tab is selected in our custom editor instance.
Drag meta box containing editor around (ensuring the jQuery UI Sortable drop placeholder shows up, but the box does not need to be docked to a new location)
Release the box
Switch to the editor's Visual tab


The following error occurs:

j is null
In file: /wp-includes/js/tinymce/wp-tinymce.php?c=1&ver=345-20110918
The top row of tinymce icons is also duplicated. Once the error occurs, we can no longer switch between tabs.



Case 2:

Steps to reproduce error:

Activate test plugin.
Visual tab selected.
Drag meta box containing editor around.
Release the box.
switch to the editor's HTML tab.


The following error occurs:

Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIDOMHTMLDocument.implementation]
In file: /wp-includes/js/tinymce/wp-tinymce.php?c=1&ver=345-20110918

Note: It may take a couple of tries when switching between visual/HTML tabs to trigger the errors. Sometimes the errors won't happen, but usually they do.

Attachments (2)

metabox-editor-reinit.diff (2.2 KB) - added by guillaumemolter 8 years ago.
Proposal of fix for #19173
drag-tinymce-1.gif (772.2 KB) - added by dgwyer 6 years ago.

Download all attachments as: .zip

Change History (40)

#1 @ocean90
13 years ago

  • Owner set to azaozz
  • Status changed from new to reviewing

It works in Chrome 17 dev.

Opera error log:

Uncaught exception: TypeError: Cannot convert 'p.getBody()' to object

Error thrown at line 1, column 309502 in <anonymous function: getContent>(q) in http://wpms.dev/wp-includes/js/tinymce/tiny_mce.js?ver=345-20111105:
    r=p.getBody().innerHTML
called from line 1, column 340171 in g() in http://wpms.dev/wp-includes/js/tinymce/tiny_mce.js?ver=345-20111105:
    return b.trim(f.getContent({format:"raw",no_events:1}))
called from line 1, column 340603 in <anonymous function: add>(m) in http://wpms.dev/wp-includes/js/tinymce/tiny_mce.js?ver=345-20111105:
    m.content=g();
called from line 1, column 307310 in <anonymous function: save>(u) in http://wpms.dev/wp-includes/js/tinymce/tiny_mce.js?ver=345-20111105:
    p.undoManager.add()
called from line 1, column 306249 in <anonymous function: hide>() in http://wpms.dev/wp-includes/js/tinymce/tiny_mce.js?ver=345-20111105:
    p.save();
called from line 36, column 4 in <anonymous function: go>(a) in http://wpms.dev/wp-admin/js/editor.dev.js?ver=20110820:
    ed.hide();
called from line 1, column 0 in <anonymous function>(event):
    switchEditors.go(this);

Safari error log:

TypeError: 'undefined' is not an object (evaluating 'j.createRange')

#2 @jakemgold
13 years ago

I'm going to break out the position of the resize handle into a separate ticket, as this is a more granular issue that isn't limited to meta boxes.

#3 @nacin
13 years ago

  • Milestone changed from Awaiting Review to 3.3

I'm going to break out the position of the resize handle into a separate ticket, as this is a more granular issue that isn't limited to meta boxes.

Indeed.

#4 @azaozz
13 years ago

The problem is that TinyMCE, once initialized cannot be moved in the DOM, or rather the browsers cannot handle it being moved. That's why the errors are so inconsistent in different browsers. Moving the postbox triggers this. Some browsers/versions handle that better than others as @ocean90 mentions above but generally the editor textarea and iframe shouldn't be moved.

As $goto10 mentions using wp_editor() would be quite common inside a postbox, so we will need to find an easy way to disable moving of that postbox (dragging it on the screen doesn't count as moving although it's very CPU intensive, re-inserting the parent element somewhere else in the DOM is the problem).

Looks like we will need yet another arg on the postboxes to disallow moving but other suggestions are welcome.

Last edited 13 years ago by azaozz (previous) (diff)

#5 follow-up: @azaozz
13 years ago

Another possibility would be to get the content of the editor and then destroy the instance on dragstart. Then on dragend initialize it again and set the content. Such code should go in postboxes.js inside the UI.sortable, will run some tests with that too.

#6 follow-up: @goto10
13 years ago

...so we will need to find an easy way to disable moving of that postbox...

Looks like we will need yet another arg on the postboxes to disallow moving but other suggestions are welcome.

I think it would be nice to be able to add a meta box that is invisible. It would just show the content that is output by the callback function passed to add_meta_box(). The normal draggable/collapsible meta box stuff would not be there. The content within these boxes would act similarly to the title/permalink and main editor fields - they can't be moved and there is no visible wrapping container.

It seems like this would be a good way of facilitating the requirement of not allowing editors to be moved and, it would follow the existing convention of non movable items established by the title and editor fields.

#7 in reply to: ↑ 6 @azaozz
13 years ago

Replying to goto10:

I think it would be nice to be able to add a meta box that is invisible.

Thinking more about that, not sure it's a good idea to have a static (non-movable) metabox in between the "normal" metaboxes.

We have do_action('edit_page_form') or do_action('edit_form_advanced') that can be used to output any html under div#normal-sortables, perhaps these should be used to add extra editors? BTW we probably should pass $post_type to these actions for better control.

Alternatively we could add another action that would output html right under the main editor, i.e. above div#normal-sortables.

Last edited 13 years ago by azaozz (previous) (diff)

#8 @goto10
13 years ago

@azaozz

I think it would be nice to be able to add a meta box that is invisible.

Thinking more about that, not sure it's a good idea to have a static (non-movable) metabox in between the "normal" metaboxes.


Yes, you're right. Static boxes intermingled with traditional meta boxes would feel strange.

Typically I have all of my form fields within various meta boxes, and I loop through the array of boxes/fields to show them (in order) on the appropriate post edit screen. I was trying to think of a way that editor fields could still be added by using the same call to add_meta_box() (albeit with an extra parameter for boxes containing an editor).

Thanks for the heads up on the 'edit_page_form' and 'edit_form_advanced' hooks. These sound like good alternatives, though I do think it's important to be able to able place editors exactly where we want to. I guess I just want to have my cake and eat it too :) Maybe your second proposal with saving editor content, destroying the editor, then recreating it would be better then.

#9 @azaozz
13 years ago

  • Milestone 3.3 deleted
  • Resolution set to maybelater
  • Status changed from reviewing to closed

Per the IRC discussion today, wp_editor() shouldn't be used in a metabox. do_action('edit_page_form') or do_action('edit_form_advanced') are the proper places for it.

Last edited 13 years ago by azaozz (previous) (diff)

#10 follow-up: @nacin
13 years ago

In fairness, this only applies for wp_editor() with the visual editor enabled. HTML-only should be fine, I imagine.

#11 in reply to: ↑ 10 @azaozz
13 years ago

Replying to nacin:

Right. The limitation is that TinyMCE cannot be moved. Quicktags works fine.

#12 follow-up: @goto10
13 years ago

...wp_editor() shouldn't be used in a metabox.

Ah, I see. Thanks for giving it a go and for the tip on being able to use the HTML-only editor within metaboxes. That's pretty cool!

#13 in reply to: ↑ 12 ; follow-up: @azaozz
13 years ago

Replying to goto10:

Ah, I see. Thanks for giving it a go and for the tip on being able to use the HTML-only editor within metaboxes. That's pretty cool!

Yes, looked at this yesterday in IRC and the consensus was not to try finding a workaround for moving TinyMCE in the DOM. Also even if we output a non-movable postbox, it would still move in the DOM when other postboxes are added/removed above or below it, so that's not going to work.

#14 in reply to: ↑ 13 ; follow-up: @JPry
13 years ago

Replying to azaozz:

Yes, looked at this yesterday in IRC and the consensus was not to try finding a workaround for moving TinyMCE in the DOM. Also even if we output a non-movable postbox, it would still move in the DOM when other postboxes are added/removed above or below it, so that's not going to work.

Based on my testing, it appears that the current output for a custom editor when using either do_action('edit_page_form') or do_action('edit_form_advanced') puts the editor in the midst of the custom meta boxes. My assumption is that this means that the additional editors are still being moved in the DOM, but I admit that the DOM and how it changes with javascript is not something I'm really familiar with.

Regardless of this, wouldn't it also make sense to have additional actions before and after the custom meta boxes that could also be hooked into? That can give us better control of where the additional editors are displayed. Of course, if I'm missing something or doing it the wrong way, let me know.

#15 in reply to: ↑ 14 @azaozz
13 years ago

Replying to JPry:

No, do_action('edit_page_form') or do_action('edit_form_advanced') run after the postbox container div and their position in the DOM is static (the editor wrap DIV becomes a sibling of div#normal-sortables and div#advanced-sortables).

Last edited 13 years ago by azaozz (previous) (diff)

#16 @simonwheatley
12 years ago

  • Cc simon@… added

#17 in reply to: ↑ 5 ; follow-up: @devesine
12 years ago

Replying to azaozz:

Another possibility would be to get the content of the editor and then destroy the instance on dragstart. Then on dragend initialize it again and set the content. Such code should go in postboxes.js inside the UI.sortable, will run some tests with that too.

(Since we just addressed this, I thought I'd post what information we have.)

We're using the following code to do this in WP 3.5, and it seems to be working well and robustly (with multiple metaboxes containing multiple TinyMCE editors). The selectors could probably be refined a bit.

// TinyMCE doesn't handle being moved in the DOM.  Destroy the
// editor instances at the start of a sort and recreate 
// them afterwards.
var _triggerAllEditors = function(event, creatingEditor) {
	var postbox, textarea;

	postbox = $(event.target);
	textarea = postbox.find('textarea.wp-editor-area');

	textarea.each(function(index, element) {
		var editor;
		editor = tinyMCE.EditorManager.get(element.id);
		if (creatingEditor) {
			if (!editor) {
				tinyMCE.execCommand('mceAddControl', true, element.id);
			}
		}
		else {
			if (editor) {
				editor.save();                                          
				tinyMCE.execCommand('mceRemoveControl', true, element.id);
			}       
		} 
	});
};
$('#poststuff').on('sortstart', function(event) {
	_triggerAllEditors(event, false);
}).on('sortstop', function(event) {
	_triggerAllEditors(event, true);
});

#18 @deanmarktaylor
12 years ago

  • Cc deanmarktaylor added

#19 @sscovil
12 years ago

  • Cc sscovil@… added

#20 in reply to: ↑ 17 @bjorsq
11 years ago

I've tested this, but have encountered errors when the editor is switched to text/html mode using the tabs in the top right. It works great as long as the editor is in tinymce mode, but when it is in text/html mode there are sometimes problems when dragging the metabox, and tinymce toolbars are added when the metabox is dropped (under the bbcode toolbar). I added a check for each editor which looks for the tmce-active class on a parent first, and it seems to work OK - here is the modified _triggerAllEditors function:

	var _triggerAllEditors = function(event, creatingEditor) {
		var postbox, textarea;

		postbox = $(event.target);
		textarea = postbox.find('textarea.wp-editor-area');

		textarea.each(function(index, element) {
			var editor, is_active;

			editor = tinyMCE.EditorManager.get(element.id);
			is_active = $(this).parents('.tmce-active').length;

			if (creatingEditor) {
				if (!editor && is_active) {
					tinyMCE.execCommand('mceAddControl', true, element.id);
				}
			}
			else {
				if (editor && is_active) {
					editor.save();
					tinyMCE.execCommand('mceRemoveControl', true, element.id);
				}       
			} 
		});
	};

Replying to devesine:

Replying to azaozz:

Another possibility would be to get the content of the editor and then destroy the instance on dragstart. Then on dragend initialize it again and set the content. Such code should go in postboxes.js inside the UI.sortable, will run some tests with that too.

(Since we just addressed this, I thought I'd post what information we have.)

We're using the following code to do this in WP 3.5, and it seems to be working well and robustly (with multiple metaboxes containing multiple TinyMCE editors). The selectors could probably be refined a bit.

// TinyMCE doesn't handle being moved in the DOM.  Destroy the
// editor instances at the start of a sort and recreate 
// them afterwards.
var _triggerAllEditors = function(event, creatingEditor) {
	var postbox, textarea;

	postbox = $(event.target);
	textarea = postbox.find('textarea.wp-editor-area');

	textarea.each(function(index, element) {
		var editor;
		editor = tinyMCE.EditorManager.get(element.id);
		if (creatingEditor) {
			if (!editor) {
				tinyMCE.execCommand('mceAddControl', true, element.id);
			}
		}
		else {
			if (editor) {
				editor.save();                                          
				tinyMCE.execCommand('mceRemoveControl', true, element.id);
			}       
		} 
	});
};
$('#poststuff').on('sortstart', function(event) {
	_triggerAllEditors(event, false);
}).on('sortstop', function(event) {
	_triggerAllEditors(event, true);
});

This ticket was mentioned in IRC in #wordpress-dev by ericandrewlewis. View the logs.


11 years ago

#22 follow-up: @pawelkmpt
9 years ago

  • Resolution maybelater deleted
  • Status changed from closed to reopened

I see that 4 years ago you decided to not find workaround for this problem but it's still an issue and I think that maybe there's a chance that you'll look at it once more. Maybe TinyMCE has changed and it's easier to resolve the problem today.

Problem appears in WooCommerce, but as they use wp_editor() I believe it's WP error. So when I'm editing content in Product Short Description using Visual editor and than move meta box to the other place (drag&drop) than:

  • content from visual editor disappears (but it still stays in text editor),
  • it's not possible to edit content in visual editor anymore,
  • I can switch to Text editor in Chrome but cannot do that in Firefox having NS_ERROR_UNEXPECTED: wp-tinymce.php:6:0 error in console

Setup:

  • WordPress 4.3
  • WooCommerce 2.4.6
  • Chrome 45
  • Firefox 40

#23 in reply to: ↑ 22 ; follow-up: @azaozz
9 years ago

Replying to pawelkmpt:

content from visual editor disappears (but it still stays in text editor),
it's not possible to edit content in visual editor anymore

This is a browser limitation. Happens because iframes cannot be moved safely in the DOM. They get reset/reloaded. If you look around, you'll probably find few old bug reports for Firefox and WebKit/Chrome that are closed as "wontfix".

So there are two options to improve this:

  • "Lock" the contaier postbox so it cannot be dragged when using the visual editor (preferred).
  • Safe the content and destroy the editor instance on dragStart, then initialize the editor again and load the content on dragEnd. This is more complex and will make the dragging somewhat hard as processing and saving the editor content can be slow.

IMHO the plugin that requires visual editor in a draggable container should decide which approach to use. We probably can add support for the first approach to wp_editor() when used in a postbox, but that's not a priority.

Version 0, edited 9 years ago by azaozz (next)

#24 in reply to: ↑ 23 ; follow-up: @mboynes
9 years ago

  • Safe the content and destroy the editor instance on dragStart, then initialize the editor again and load the content on dragEnd. This is more complex and will make the dragging somewhat hard as processing and saving the editor content can be slow.

I've tried that, and it provides a crummy UX. The editor disappears and leaves an HTML-filled textarea on drag. In Fieldmanager, we reinit the editors after the user drags them. This way, TinyMCE sticks around for the drag, which is much less confusing for users. In terms of performance, this seems to work well (though I've never tried it on an older machine).

It would be nice if there were a more elegant solution, but this is the best I was able to come up with, and it works. To @azaozz's point, this is a solution I implemented on the plugin level, and I didn't think much of needing to do so. I don't think that core necessarily needs to support this behavior, but I also wouldn't oppose it.

#25 in reply to: ↑ 24 @aware
9 years ago

Replying to mboynes:

  • Save the content and destroy the editor instance on dragStart, then initialize the editor again and load the content on dragEnd. This is more complex and will make the dragging somewhat hard as processing and saving the editor content can be slow.

Completely agree. We're doing this now for a plugin we are releasing soon. I did have some performance issues on an older machine. But I think it was less to do with destroying and rebuilding and more so to do with having a lot of editors on the same page.

#26 @azaozz
9 years ago

  • Resolution set to maybelater
  • Status changed from reopened to closed

In Fieldmanager, we reinit the editors after the user drags them.

Yeah, that should look better. Only concern is possible "choppy" dragging of a big/complex element that contains the editor iframe.

Looking at https://github.com/alleyinteractive/wordpress-fieldmanager/blob/master/js/richtext.js#L58-L95:

  • You can check if the Text editor is currently being used by doing ed.isHidden().
  • If true, can re-initialize the instance without restoring the content, as the latest content is in the textarea and MCE will load it on initialization.
  • If false (i.e. the Visual editor is being used) will have to "save" to the textarea first.

So the code will be something like (untested):

if ( ! ed.isHidden() ) {
  ed.save();
}

tinymce.remove( ed );
tinymce.init( tinyMCEPreInit.mceInit[ this.id ] );

This should work in WP 4.3+ as the textarea value is run through the JS wpautop. There is no need to set the content or switch editors. For older versions you'll still need to load the content in the new instance when MCE is being used to keep the <p> tags.

I'm not sure what can be added to wp_editor() to make this simpler/easier. It is quite straightforward to implement in a plugin. Going to re-close as "maybelater". Feel free to reopen and attach a patch if you come up with something that can improve it.

#27 @ocean90
9 years ago

#36321 was marked as a duplicate.

@guillaumemolter
8 years ago

Proposal of fix for #19173

#28 @guillaumemolter
8 years ago

  • Resolution maybelater deleted
  • Status changed from closed to reopened

Hi,

I have attached a patch based on @azaozz solution. The only things that needed to be added were:
1) handle the use case where multiple editors are used in multiple meta-boxes
2) If the user was currently viewing quicktags, @azaozz solution was bringing the editor in the foreground. I'm switching back to TinyMCE.

Please let me know if you think that could be improved in any ways.

This is my first contribution to Core so be indulgent :-)

Best,
Guillaume

#29 @swissspidy
8 years ago

#39887 was marked as a duplicate.

#30 @SergeyBiryukov
8 years ago

  • Milestone set to Awaiting Review

#31 @swissspidy
8 years ago

  • Keywords has-patch added

#32 @dgwyer
6 years ago

Just wondering if there is any chance of this being added to core soon?

A plugin I'm working on is entirely reliant on being able to drag drop multiple instances of TinyMCE editors inside a meta box.

In the meantime is there a way to add this functionality manually? I'd love to get this working.

@dgwyer
6 years ago

#33 follow-up: @dgwyer
6 years ago

I've managed to get this working by manually reinitializing the tinyMCE editor where required. So as for adding this to core I'd aggress with @mboynes.

I don't think that core necessarily needs to support this behavior, but I also wouldn't oppose it.

Thanks to all for posting helpful code suggestions to make this workaround possible.

#34 @noisysocks
4 years ago

  • Keywords needs-testing added

#35 @grapestain
4 years ago

I have an issue on a site now with wp_editor() inside a MetaBox, which seems to be related, but it is not caused by moving the MetaBoxes around. It looks more like a race condition or so, because approx. 50% of the times the edit post page is loaded the editors do not work. I assume there may be some other things on the same page, which cause some critical DOM changes, and when those changes finish before TinyMCE is initialised, the editors work, but other times they not. Reloading the page few times causes them to work, as the issue is intermittent, but it is less than ideal. But this is only an assumption.

Unfortunately I see no errors in the console or any other clear indication of the problem.

On one hand I'm writing this so we can see if others have similar issues too?

I've monkey patched my editor.js & postbox.js (and the minified ones), but when I try to run wp.editor.reinit_metabox_editors() from the console, it says wp.editor.reinit_metabox_editors is not a function. Actually the reinit_metabox_editors() function is exposed under switchEditors.reinit_metabox_editors(). Maybe I've done something wrong...

Amyway, just for reference, I've after all included function reinit_metabox_editors() from the patch as a standalone function in a separate JS file, and set up into the edit post pages to run 1.5s after window.onload, and now it works fine. Still don't know the root cause though...

FF v84.0.2
WP v5.6

Last edited 4 years ago by grapestain (previous) (diff)

#36 in reply to: ↑ 33 ; follow-up: @idmcalculus
3 years ago

Replying to dgwyer:

I've managed to get this working by manually reinitializing the tinyMCE editor where required. So as for adding this to core I'd aggress with @mboynes.

I don't think that core necessarily needs to support this behavior, but I also wouldn't oppose it.

Thanks to all for posting helpful code suggestions to make this workaround possible.


@dgwyer Can you kindly share a pastebin or code snippet of how you got this working? Thank you

Last edited 3 years ago by idmcalculus (previous) (diff)

#37 in reply to: ↑ 36 ; follow-up: @dgwyer
3 years ago

@idmcalculus I don't have the exact code to hand but it used the same approach as described in the accecpted answer here. It seemed to work pretty well as I recall.

https://stackoverflow.com/questions/4651676/how-do-i-remove-tinymce-and-then-re-add-it

#38 in reply to: ↑ 37 @idmcalculus
3 years ago

Replying to dgwyer:

@idmcalculus I don't have the exact code to hand but it used the same approach as described in the accecpted answer here. It seemed to work pretty well as I recall.

https://stackoverflow.com/questions/4651676/how-do-i-remove-tinymce-and-then-re-add-it

This looks good. I'm trying it out right away. Thank you @dgwyer!

Last edited 3 years ago by idmcalculus (previous) (diff)
Note: See TracTickets for help on using tickets.