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.

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

Call tinyMCE.triggerSave() before sending metabox data #7176

Closed
danielbachhuber opened this issue Jun 6, 2018 · 52 comments
Closed

Call tinyMCE.triggerSave() before sending metabox data #7176

danielbachhuber opened this issue Jun 6, 2018 · 52 comments
Labels
Backwards Compatibility Issues or PRs that impact backwards compatability [Feature] Meta Boxes A draggable box shown on the post editing screen Needs Technical Feedback Needs testing from a developer perspective. [Type] Bug An existing feature does not function as intended

Comments

@danielbachhuber
Copy link
Member

Prior to sending metabox data for save, we need to call window.tinyMCE.triggerSave() to update any <textarea> fields controlled by wp_editor() instances.

If TinyMCE is used on a <textarea> field, then the data doesn't end up in the actual <textarea> until a save is triggered.

Originally #6293

@danielbachhuber danielbachhuber added [Type] Bug An existing feature does not function as intended [Component] TinyMCE [Feature] Meta Boxes A draggable box shown on the post editing screen Backwards Compatibility Issues or PRs that impact backwards compatability labels Jun 6, 2018
@krisgale-zz
Copy link

also currently experiencing this problem.

@krisgale-zz
Copy link

workaround:

document.addEventListener( 'DOMContentLoaded', () => {
  wp.editor.initialize( 'field_name', {
    'textarea_name': 'field_name',
    'tinymce': {
      'init_instance_callback': ( editor ) => {
        editor.on( 'change', function() {
          const editor = tinymce.get( this.id );
          document.getElementById( 'field_name' ).value = editor.getContent();
        } );
      },
    },
  } );
} );

where field_name is the id= and name= of the textarea being 'enhanced.'

@danielbachhuber
Copy link
Member Author

@krisgale Can you share a code snippet and testing instructions to reproduce your specific problem? I'd like to verify the change in #8762

@krisgale-zz
Copy link

krisgale-zz commented Aug 9, 2018

no code snippet as such (already replaced with above workaround), but adding any metabox that contains an instance of wp_editor would replicate the issue... disable gutenberg, content saves; enable gutenberg, content does not save.

also, #8762 mentions ACF, which i'm not using.

@danielbachhuber
Copy link
Member Author

I put together a code snippet:

add_action( 'add_meta_boxes', function(){
	add_meta_box( 'test_tinymce', 'Test TinyMCE', function( $post ){
		 $field_value = get_post_meta( $post->ID, 'test_tinymce', true );
		 wp_editor( $field_value, 'test_tinymce_id', array(
			'wpautop'       => true,
			'media_buttons' => false,
			'textarea_name' => 'test_tinymce',
			'textarea_rows' => 10,
			'teeny'         => true
		) );
	}, null, 'advanced', 'high' );
});
add_action( 'save_post', function( $post_id ){
	if ( ! isset( $_POST['test_tinymce'] ) ) {
		return;
	}
	update_post_meta( $post_id, 'test_tinymce', $_POST['test_tinymce'] );
});

@danielbachhuber
Copy link
Member Author

See conversation in #8762 (comment) for the failed attempt at fixing the issue.

@krisgale-zz
Copy link

@danielbachhuber thanks for staying on top of it

@michakrapp
Copy link
Contributor

So the current state is that Gutenberg will not save content from wp_editor() fields in metaboxes, correct?

@krisgale Unfortunately I can't get your workaround to correctly work.
field_name should be the $editor_id parameter you set on wp_editor() corrrect?
So my code would look like this:

document.addEventListener( 'DOMContentLoaded', () => {
    wp.editor.initialize( '_wp_editor_eve_post_links', {
        'textarea_name': '_wp_editor_eve_post_links',
        'tinymce': {
            'init_instance_callback': ( editor ) => {
                editor.on( 'change', function() {
                    const editor = tinymce.get( this.id );
                    document.getElementById( '_wp_editor_eve_post_links' ).value = editor.getContent();
                } );
            },
        },
    } );
} );

But the change event does not get triggered.

@krisgale-zz
Copy link

important note per the above ^
in implementing this workaround, i replaced the call in php to wp_editor() with only js to instantiate the tinymce instance

@michakrapp
Copy link
Contributor

@danielbachhuber has this bug been fixed with a change in Version 3.9 or 3.8? (I think with 3.7 the issue has existed)

Because after the newest update it seams to work now.

@danielbachhuber
Copy link
Member Author

@michakrapp I'm not sure whether the issue has been fixed accidentally. When we worked on this last, we weren't able to reproduce the issue reliably: #8762 (comment)

@michakrapp
Copy link
Contributor

Strange thing - after the update it has been working a few times.
Now the error is back.

But I can confirm that call window.tinyMCE.triggerSave() before saving works like you demonstrated in #8762

@rilwis
Copy link

rilwis commented Oct 25, 2018

Hello,

Our users experience a similar problem. They're using Meta Box plugin to create repeatable editors. The data is not saved correctly. Details are here.

I tested and see that when the editor in the Visual mode, the content is not saved. When it's in Text mode, it seems to work fine.

I tried calling window.tinyMCE.triggerSave() before saving, and it works. So I'd suggest merging the PR #8762.

Thanks

@ideadude
Copy link
Contributor

ideadude commented Nov 9, 2018

I am able to recreate this issue with our memberlite-elements plugin. It adds a few wp_editor instances into the meta boxes below the page. When I click to update the page, the meta box fields aren't saved. When I debugged, it's because the $_POST['field_name'] value was not updated. It seems that TinyMCE would update this before the post got saved and the suggestion solution here would work.

I tried typing window.tinyMCE.triggerSave() into the JS console and then saving and that doesn't work for me. Something else is needed.

I am using WP 5.0-beta3-43878, Gutenberg Version 4.2.0, Chrome

FWIW here is where we insert our editor:
https://github.com/strangerstudios/memberlite-elements/blob/development/elements/page_banners.php#L73

Here is where we process it. It fails on this line since the POST content is empty.
https://github.com/strangerstudios/memberlite-elements/blob/development/elements/page_banners.php#L184

Note that this plugin is meant to work with the Memberlite theme, but does still work (in terms of saving values) with Twenty* themes and fails with a Twenty* theme active.

Do we need a better minimal example? Do we need someone to dig into this further and look for the fix? Let me know. I might have time.

@robincornett
Copy link

Wondering if this will be fixed in WP Core. Currently, any meta boxes with wysiwyg editors don't save in the block editor, and the promised meta box compatibility flags do not work either: meta boxes marked as incompatible are simply skipped for output, rather than falling back to the classic editor.

@pbrocks
Copy link

pbrocks commented Nov 17, 2018

@robincornett I suspect that this will be fixed in WP Core when the issue is understood fully. In my view this is related to TinyMCE not loading correctly for Metaboxes. That is a developer concern. For users, however, it may be helpful to know that Metaboxes will save correctly and fully if done using the Text tab, not Visual. That is, if you make all of the changes that you wish using the Visual tab, ie using the TinyMCE editor, and you switch to the Text tab, meaning you will see the text rendering of the html markup, when you click Publish or Update, the data in the metaboxes, including the markup, will save. Because of I am confident that this will be corrected in Core, but it will take someone who understands the waterfall of javascript in order to place the .triggerSave() where it will be recognized and the change effectuated.

@robincornett
Copy link

Thanks, @pbrocks--I get that it will theoretically be fixed in Core, but at the moment, I'm blocked on updating a plugin because of this issue (telling users to use the text tab isn't a great option for me) combined with no backwards compatibility in place yet. I hate to tell my users that they have to add the Classic Editor plugin just to keep using the plugin they've been using for an extended period of time, but right now that is all I can do. My concern is as a developer, not as a user.

@mtias mtias added the Needs Technical Feedback Needs testing from a developer perspective. label Nov 20, 2018
@robincornett
Copy link

I understand (about 5.0.1), although that's unfortunate with the current state of the incompatible metabox notices--this would mean I need to go ahead and update my plugin(s) with the metaboxes marked as incompatible with the block editor--so users will be told to install the Classic Editor. Even if this is fixed in 5.0.1, users who've encountered this will have activated the Classic Editor based on the warnings; I wonder how many will bother deactivating it, especially since the warning disappears in the Classic Editor once the metabox has been marked as compatible with the block editor.

It seems like affected users may enable the Classic Editor and then never know to leave it, even if they wanted to, once the compatibility warning disappears. I realize that the metabox notifications are a separate issue, but these two together are a concern to me.

@youknowriad
Copy link
Contributor

@robincornett That's not the only thing that's not compatible with previous plugins with Gutenberg. And the solution has always been the classic editor until the plugin is made compatible. So I don't think it will have a big impact.

@webmandesign
Copy link
Contributor

@youknowriad Sure, I must have written it badly ;) I've tested with Gutenberg (from the zip) active. And it wasn't working. Then, for comparison, I've deactivated Gutenberg and tested again, and it worked with WP4.9.8.
So, what's my point is that the fix from the zip file is not working for me.

@youknowriad
Copy link
Contributor

@webmandesign Maybe it's fixing one incompatibility (the triggerSave call) and we're missing another one.

@youknowriad
Copy link
Contributor

@azaozz Aside the window.tinyMCE.triggerSave() do you have anything that comes to your mind that may have an impact on the persistence of the TinyMCE fields?

@robincornett
Copy link

robincornett commented Nov 27, 2018

Just a note: the fix/PR works with Gutenberg active running 5.0RC, but not trunk (which I believe is still equivalent with 4.9.8, right?). So some difference between 5.0 and trunk is making the fix not work (outside of the RC), it appears.

@webmandesign
Copy link
Contributor

@youknowriad That may be possible.
I was actually digging into metabox builder code of my WebMan Amplifier plugin as I haven't touched that part for a long time. However:

So, I think we can rule out WebMan Amplifier issue here?

Maybe, is there any JS action triggered when Gutenberg post is being updated? So I can use it to hook window.tinyMCE.triggerSave() code from WebMan Amplifier to test?

@youknowriad
Copy link
Contributor

@robincornett Interesting and also good news I'd say. Maybe it's related to the TinyMCE config alignments we did recently.

@youknowriad
Copy link
Contributor

@webmandesign Could you test with 5.0 RC and this plugin to see if it fixes the issue?

@webmandesign
Copy link
Contributor

webmandesign commented Nov 27, 2018

@youknowriad Yes, I've done that immediately as I saw @robincornett 's comment. And I can confirm it works perfectly fine now and the issue is fixed! Great catch there @robincornett!

So, the combination of WP5.0RC + Gutenberg from zip works OK and fixes the issue.

@youknowriad
Copy link
Contributor

Awesome, this means we can merge the PR once approved and close this issue

@azaozz
Copy link
Contributor

azaozz commented Nov 27, 2018

Aside the window.tinyMCE.triggerSave() do you have anything that comes to your mind...

The other thing is that when the "default" wp_editor() is used, and the editor has the two tabs: Visual and Text, triggerSave() should not be called when the Text tab is "active".

The reason is that if the user has edited something in Text, it will be overridden by the editor instance (the hidden Visual tab).

Best is to always use:

var editor = tinymce.get( 'my-editor-id' );

if ( ! editor.isHidden() ) {
    editor.save(); // this also returns the cleaned up html
}

@webmandesign
Copy link
Contributor

Thank you so much for looking at this ans providing compiled zip file for testing @youknowriad !

@youknowriad
Copy link
Contributor

@azaozz Interesting, since we don't control the TinyMCE instances in Gutenberg, is there a way to retrieve all the instances in a given DOM container?

@azaozz
Copy link
Contributor

azaozz commented Nov 27, 2018

TinyMCE keeps a collection of the instances, in tinymce.editors. Can perhaps look them up by (HTML) id?

In Gutenberg, only the Classic Block instance is "exposed" that way.

@robincornett
Copy link

CMB2 has added a fix for this in their plugin now; their solution may be worth examining. I'm implementing it for my own work and it is working in 5.0RC without Gutenberg active.

@youknowriad
Copy link
Contributor

@azaozz In my testing, even if I'm in the text mode, the fix in #12363 doesn't reset the value to be saved. I'd prefer to keep the fix simple. Would you mind reviewing the PR?

@webmandesign
Copy link
Contributor

@youknowriad @azaozz I can confirm, the fix does not override anything during post saving even when in "Text" mode of TinyMCE.

I've tested with 2 different TinyMCE editors within 1 metabox, switching between modes, trying different combinations for both. Everything works perfectly fine.

Would be great to get this with WP5.0 ;)

@pglewis
Copy link
Contributor

pglewis commented Dec 3, 2018

5.0 is frozen at the moment. This will more likely land in 5.0.1 which should happen just two weeks after the 5.0 but this could change, not my decision.

Do you know if this is still the plan? I understand feature freeze but this is a back-compat fix and seems like feature freeze should not stop it.

If it does get punted to 5.0.1 what is the recommended course of action for plugin developers? Sniff out 5.0 specifically and manually call a triggerSave hack-around, but not on 5.0.1+?

@pglewis
Copy link
Contributor

pglewis commented Dec 3, 2018

Also: preliminary testing confirms the combination of the zip file above and the latest 5.0RC (but not 4.9.8) does resolve the problem for us with Pods Framework.

Many Imaginary Internet Points for this fix landing in 5.0 because it makes one of my own issues evaporate :D.

mrhead added a commit to memberful/memberful-wp that referenced this issue Dec 5, 2018
Our metabox doesn't work correctly with the new editor because of
WordPress/gutenberg#7176.
rilwis added a commit to wpmetabox/meta-box that referenced this issue Dec 6, 2018
Bug report: https://metabox.io/support/topic/data-are-not-saved-into-the-database/
Issue: WordPress/gutenberg#7176

The fix is based on CMB2 fix (CMB2/CMB2#1190) and the PR(WordPress/gutenberg#12568)

When the PR is merged into core/Gutenberg, this fix should be removed.
@youknowriad
Copy link
Contributor

closed by #12568

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backwards Compatibility Issues or PRs that impact backwards compatability [Feature] Meta Boxes A draggable box shown on the post editing screen Needs Technical Feedback Needs testing from a developer perspective. [Type] Bug An existing feature does not function as intended
Projects
None yet