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

Metaboxes: Trigger tinymce save before saving metaboxes #8762

Closed
wants to merge 1 commit into from

Conversation

youknowriad
Copy link
Contributor

closes #7176

The idea is to fix saving wp_editors used in metaboxes.

Testing instructions

  • Install ACF free from the repository
  • Add a WYSIWYG field
  • Try editing and saving the field
  • Reload the page, it should save properly

@youknowriad youknowriad added [Type] Bug An existing feature does not function as intended [Feature] Meta Boxes A draggable box shown on the post editing screen labels Aug 9, 2018
@youknowriad youknowriad self-assigned this Aug 9, 2018
@youknowriad youknowriad requested review from danielbachhuber and a team August 9, 2018 09:36
@danielbachhuber
Copy link
Member

@youknowriad Your testing instructions also work for me on the master branch:

testingtinymce

Did your testing instructions fail for you on the master branch?

The change itself seems logically reasonable; just want to confirm the change fixes a real test case.

@youknowriad
Copy link
Contributor Author

@danielbachhuber Which version of ACF are you using?

@youknowriad
Copy link
Contributor Author

I'm using the one in the repository, I suspect they did something on their end for the 5.* version

@danielbachhuber
Copy link
Member

@youknowriad I installed 4.4.12 from the WordPress.org repository.

@youknowriad
Copy link
Contributor Author

youknowriad commented Aug 9, 2018

🤷‍♂️ I'd have sworn I tested without my changes and it broke :P but anyway maybe we can ask the people who asked for this in the issue

Edit: you already did :)

@danielbachhuber
Copy link
Member

@youknowriad This pull request fails for me using the test metabox in #7176 (comment)

testingtinymce

However, if I call window.tinyMCE.triggerSave() directly in the console, the text in the TinyMCE field is saved correctly.

@youknowriad
Copy link
Contributor Author

Thanks for the snippet, I'll give it a try when I have some time.

@youknowriad
Copy link
Contributor Author

It's weird because I can't break it with or without the PR. Maybe it's browser specific, but I tried in both Firefox and Chrome.

@danielbachhuber
Copy link
Member

Maybe it's browser specific, but I tried in both Firefox and Chrome.

Or maybe it's a race condition or some deeper bug?

@youknowriad
Copy link
Contributor Author

I'm closing the PR for now and I think we should close the issue until we can break it consistently :P or maybe you can try a fix since you can reproduce

@youknowriad youknowriad deleted the fix/metaboxes-wysiwyg branch August 13, 2018 13:13
@danielbachhuber
Copy link
Member

I think we should close the issue until we can break it consistently :P or maybe you can try a fix since you can reproduce

I'd like to keep the issue open because closing the issue doesn't make the problem go away. I don't have any ideas on what the fix might be though.

@ideadude
Copy link
Contributor

ideadude commented Nov 10, 2018

I've dug into this a bit, but I feel a TinyMCE expert would have better luck figuring out the correct fix. I posted some notes on reproducing with our plugin that uses wp_editor in meta boxes below the edit post body in this related issue: #7176

Some notes:

  • TinyMCE editors work by adding an iframe that shows the editor in all of it's HTML glory.
  • There is also a textarea below the editor iframe that is initialized with the previously saved value for the meta box field.
  • When the form is submitted, TinyMCE finds all of the editors and calls a getContent and/or setContent method to override the value of the textarea. I saw this while stepping through the console debugger, but only on the minimized version of TinyMCE, which is hard to follow.
  • I believe that since Gutenberg isn't actually submitting the form, but instead using REACT/API to update the post this bit of TinyMCE code isn't triggering to update the textareas.
  • One confusion is that at various places autosave methods are fired/triggered, but autosave excludes meta fields.
  • The window.tinyMCE.triggerSave() or different variants isn't working for me to copy the editor content into the textareas.
  • Something like this does work for me: jQuery('#mytextareaname').html(TinyMCE.get('mytextareaname').getContent());

TL;DR:
I think in the classic editor we are relying on something in TinyMCE that copies the editor content into the textareas that are actually posted to the server.

With Gutenberg, this never happens.

Once we figure out the best way to trigger TinyMCE to update all of the wp editor textareas (or write a new way), we should probably insert it into the bottom of this method in post.js: https://github.com/WordPress/WordPress/blob/bdbaccce379b9db3e8a1a6e5c7a0c6a91ae197bf/wp-admin/js/post.js#L334

I don't know where the equivalent of that file is in the Gutenberg plugin.

I hope this helps.

@ideadude
Copy link
Contributor

Another note, it's very possible that certain plugins like Advanced Custom Fields have coded their own solutions for this, which is why the problem might have "gone away" in certain tests.

Ideally, if someone uses wp_editor in a meta box, it should get posted on save post without having to add additional JavaScript to fix this bug.

@azaozz
Copy link
Contributor

azaozz commented Nov 12, 2018

Hmmm, tinymce.triggerSave() is the "proper way" to do this. It loops through the instance collection (tinymce.editors) and calls .save() on each.

There may be a problem where the "Text" editor tab is used and tinymce.triggerSave() is called. It would still overwrite any changes in the textarea with whatever is in the instance/iframe. To avoid that it'll need to do ! editor.isHidden() before the save.

@pbrocks
Copy link

pbrocks commented Nov 14, 2018

@azaozz if I understand you correctly the only way for the metaboxes content to be saved is via js. Is there a snippet that you can point us to? This issue seems to involve only tinymce, ie the Visual tab. Content entered via the text editor tab will save.

@pbrocks
Copy link

pbrocks commented Nov 14, 2018

@youknowriad This issue definitely still exists. @danielbachhuber I ran a test using your snippet for a metabox and was able to get TinyMCE Toolbar1 items to save, but if I removed your 'teeny' => true, so I could use Toolbar2, then things failed. However, the Gutenberg version was a Development copy with the snippet
if ( window.tinyMCE ) { window.tinyMCE.triggerSave(); }
from @davisshaver found here

if ( window.tinyMCE ) {
window.tinyMCE.triggerSave();
}

@davisshaver
Copy link
Contributor

@pbrocks I agree this is still an issue. fwiw I moved away from the patched version and as a workaround I believe that the issue doesn't show up if you use the source rather than visual editor.

@pbrocks
Copy link

pbrocks commented Nov 14, 2018

Actually just tested on an out of the box Gutenberg and had decent results with html and non-tinymce metabox window. Screencast here:

https://monosnap.com/file/ia9Jq03YKZDmiBQrl7x7R9yrwSYt2y

@pbrocks
Copy link

pbrocks commented Nov 14, 2018

It appears that @azaozz has a point regarding the Text tab vs Visual tab. I was able to successfully use the editor and the full range of toolbars 1 and 2 in the metabox editor, as long as when I clicked save or update, I was on the text tab, ie seeing the html as text.

Screencast showing metabox editor saving html when viewed as text

@pbrocks
Copy link

pbrocks commented Nov 16, 2018

Is there any issue with Tinymce being locaded twice?

tinymce-twice

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Meta Boxes A draggable box shown on the post editing screen [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Call tinyMCE.triggerSave() before sending metabox data
6 participants