Make WordPress Core

Opened 4 years ago

Closed 3 years ago

#52133 closed defect (bug) (fixed)

TinyMCE editor doesn't load properly when initializing on Visual Tab (Firefox)

Reported by: metalandcoffee's profile metalandcoffee Owned by: azaozz's profile azaozz
Milestone: 5.8 Priority: normal
Severity: normal Version: 5.6
Component: TinyMCE Keywords: reporter-feedback has-patch
Focuses: Cc:

Description

I've noticed that since updating to WordPress 5.6, TinyMCE editors within my custom metaboxes will sometimes fail to load/initialize when using the Firefox browser. This only happens when the editor is initializing on the Visual tab. If I switch to the Text tab and then reload the page, the editor initializing normally. Please see the attached video for a clear understanding of what the issue is (firefox-bug-issue.mov). This screen recording will show that when you first go to the post, the TinyMCE editor initializes properly. But if you refresh the page, then the editor isn't initialized at all. It doesn't always happen like this. Sometimes I'll go to a post edit page and it's not initialized from the start.

Theory:
I say that this "sometimes" happens because it's almost like the tinymce js resources are loading too slow or too fast. This isn't a browser caching issue either because if I hard refresh the page, it still depends on how fast the resources are loading as to whether or not the TinyMCE editors will be properly initialized or not.

I've confirmed that this was working fine on WordPress version 5.5.3.

Steps to reproduce:
I attached a file called "test-code.php" which will add a metabox with the TinyMCE editor to the 'post' post type.

  1. Add test code to theme or plugin.
  2. Open Firefox.
  3. Migrate to post edit page.
  4. See if you are able to click inside of the TinyMCE editor on the Visual tab.
  5. Refresh the page and repeat.

Attachments (4)

firefox-bug-issue.mov (3.9 MB) - added by metalandcoffee 4 years ago.
test-code.php (1.3 KB) - added by metalandcoffee 4 years ago.
firefox-bug-2021.mov (2.8 MB) - added by metalandcoffee 3 years ago.
52133.diff (2.0 KB) - added by azaozz 3 years ago.

Change History (42)

#1 @johnbillion
4 years ago

  • Milestone changed from Awaiting Review to 5.6.1

#2 @spikeuk1
4 years ago

Looks likely to be related to Ticket #52111.

#3 @desrosj
4 years ago

  • Keywords needs-testing added

@metalandcoffee I know that these are editors within custom metaboxes, but can you clarify whether the Classic Editor plugin is active? I'm not sure that would make a difference, but confirming the behavior with and without the plugin could help find the issue here.

Just to be sure, I double checked there were no TinyMCE updates available. 4.9.11 is the latest 4.x version and there are no 4.x bug fixes waiting to be released.

#4 @metalandcoffee
4 years ago

Hi @desrosj - The Classic Editor plugin was not installed when I did the initial testing for this ticket.

This ticket was mentioned in Slack in #core by metalandcoffee. View the logs.


4 years ago

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


4 years ago

#8 @whyisjake
4 years ago

  • Milestone changed from 5.6.1 to 5.6.2

Bumping to 5.6.2

#9 follow-up: @desrosj
4 years ago

  • Milestone changed from 5.6.2 to Future Release

5.6.2 RC is going to be packaged in a few hours. Since this one still requires some investigation and lacks a patch, I'm going to punt to Future Release.

#10 in reply to: ↑ 9 @patkemper
4 years ago

Replying to desrosj:

5.6.2 RC is going to be packaged in a few hours. Since this one still requires some investigation and lacks a patch, I'm going to punt to Future Release.

I really don't understand, why such problems are "punted to Future Releases". Updating jQuery caused severe issues and I don't see any progress on this.

Why is downgrading jQuery no option?

#11 @herrvigg
4 years ago

I'm recopying here what I wrote in other tickets. Not sure but this could explain the problem, asynchronous firing with jQuery3: https://github.com/jquery/jquery/issues/3194

With jQuery3, ready handlers fire asynchronously and may be fired after load... there's no guarantee so that can explain some "random" behaviors.

Long story short, the load trigger should never be set in the ready callback but outside. Ideally it should even be avoided when using ready.

#12 follow-up: @patkemper
4 years ago

  • Resolution set to worksforme
  • Status changed from new to closed

A few clients confirmed, that this bug seems to be fixed due to latest WP-update.

#13 follow-up: @metalandcoffee
4 years ago

  • Resolution worksforme deleted
  • Status changed from closed to reopened

Hi @patkemper - I'm not sure why you closed this ticket. I'm glad that it's working for you but I just tested it when the exact same scenario that led me to open this ticket (WordPress 5.7) and it is still an issue that should be kept open for the time being.

Version 0, edited 4 years ago by metalandcoffee (next)

#14 in reply to: ↑ 12 @spikeuk1
4 years ago

Replying to patkemper:

A few clients confirmed, that this bug seems to be fixed due to latest WP-update.

And my related ticket (https://core.trac.wordpress.org/ticket/52111) is still valid in 5.7, as it was in 5.6.2, 5.6.1, and 5.6.

Spike

#15 in reply to: ↑ 13 @patkemper
4 years ago

Replying to metalandcoffee:

Hi @patkemper - I'm not sure why you closed this ticket. I'm glad that it's working for you but I just tested it with the exact same scenario that led me to open this ticket (on WordPress 5.7) and it is still an issue that should be kept open for the time being.

I'm so sorry. Didn't mean to close it right away.
Anyway, my clients are still using 5.6.2, next week 5.7 will be rolled out. I will still monitor this issue.

#16 follow-up: @azaozz
3 years ago

  • Keywords reporter-feedback added; needs-testing removed

Tried to reproduce this again with the example plugin code but couldn't. TinyMCE seems to be initializing properly in the Test TinyMCE Note postbox. Can somebody confirm if this is still happening and maybe share any additional steps to reproduce it.

#17 in reply to: ↑ 16 ; follow-up: @patkemper
3 years ago

Replying to azaozz:

Tried to reproduce this again with the example plugin code but couldn't. TinyMCE seems to be initializing properly in the Test TinyMCE Note postbox. Can somebody confirm if this is still happening and maybe share any additional steps to reproduce it.

Can Confirm. For me, this bug seems to be fixed. I also asked several clients for confirmation.

#18 in reply to: ↑ 17 @azaozz
3 years ago

  • Milestone Future Release deleted
  • Resolution set to worksforme
  • Status changed from reopened to closed

Replying to patkemper:

Thanks. Closing as worksforme, feel free to reopen with more details if this still happens.

#19 @patkemper
3 years ago

  • Resolution worksforme deleted
  • Status changed from closed to reopened

Sorry, I've been pleased too early. Just today a client informed me, that the issue still remains.

I recorded a video, that shows the issue in firefox.
https://streamable.com/f5smup

Interestingly, you can also trigger the bug using Chrome by moving (drag 'n drop) the meta-box.
https://streamable.com/nx843r

#20 @metalandcoffee
3 years ago

Good morning. I've confirmed that this is still an issue following my exact scenario that I outlined on a baseline install of WordPress 5.7.2.

I keep a close eye on this ticket so I would like to test it before it gets closed since this is an ongoing issue for myself and clients.

I am on Firefox 89.0 64-bit.

#21 follow-up: @metalandcoffee
3 years ago

What @patkemper pointed out can potentially be an important finding. I've confirmed that you can also trigger this issue by initiating a drag-and-drop on the metabox.

See image here: https://metalandcoffee.com/wp-content/uploads/2021/06/tinymce-issue.gif

#22 in reply to: ↑ 21 @azaozz
3 years ago

Replying to metalandcoffee:

I've confirmed that you can also trigger this issue by initiating a drag-and-drop on the metabox.

Unfortunately this is a known problem that has always existed. Nothing to do with TinyMCE, the browsers would "empty" iframes when they are dragged (their position/parent node changes in the DOM). That's a security precaution afaik.

There is a possible workaround for this involving deleting the TinyMCE instance on dragstart (can be quite choppy), and re-initializing it after dragend but it was failing a lot (about 1/5 loosely depending on content length) last time I tested it.

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

#23 follow-up: @azaozz
3 years ago

  • Keywords has-patch needs-testing added
  • Milestone set to 5.8

Thanks @metalandcoffee and @patkemper for testing. Looks like this may be the same as #52050, and perhaps similar to #52111. Could you please test if https://core.trac.wordpress.org/attachment/ticket/52111/52111.diff fixes it.

Moving back to the 5.8 milestone pending testing of the possible patch.

#24 follow-up: @metalandcoffee
3 years ago

Hi @azaozz - I tested the 52111 patch and it does not seem to fix the issue for me.

#25 in reply to: ↑ 24 @spikeuk1
3 years ago

Replying to metalandcoffee:

Hi @azaozz - I tested the 52111 patch and it does not seem to fix the issue for me.

It didn't work for me either :(

Did you see the debug error messages I posted on 52111 earlier today? Do you get similar?

Spike

#26 in reply to: ↑ 23 @patkemper
3 years ago

Replying to azaozz:

Thanks @metalandcoffee and @patkemper for testing. Looks like this may be the same as #52050, and perhaps similar to #52111. Could you please test if https://core.trac.wordpress.org/attachment/ticket/52111/52111.diff fixes it.

Moving back to the 5.8 milestone pending testing of the possible patch.

Many thanks for the patch. Unfortunately, it does not solve the issue.

#27 @azaozz
3 years ago

@metalandcoffee, @spikeuk1, @patkemper thanks for testing!

Yes, I can still reproduce this sometimes when initializing TinyMCE in an (old style) metabox in the block editor. Seems the only way around this that I can think of at the moment would be to delay initialization of TinyMCE in postboxes until the page is fully loaded.

@azaozz
3 years ago

#28 follow-up: @azaozz
3 years ago

In 52133.diff: when a TinyMCE instance is in a postbox delay initialization until the window is loaded.

Please test asap :)

#29 follow-up: @azaozz
3 years ago

Related #52050. 52133.diff should fix it.

#30 in reply to: ↑ 28 @metalandcoffee
3 years ago

Replying to azaozz:

In 52133.diff: when a TinyMCE instance is in a postbox delay initialization until the window is loaded.

Please test asap :)

Wow! It's fixed!!!! Waiting until the window is loaded to initialize TinyMCE was the magic ✨

Thank you so much @azaozz.

#31 in reply to: ↑ 29 @patkemper
3 years ago

Replying to azaozz:

Related #52050. 52133.diff should fix it.

I just tried your new patch. It still does not solve the issue.

However, removing "&& readyState !== 'interactive'" on line 1670 does the trick.

Here's the full patch.

Index: src/wp-includes/class-wp-editor.php
===================================================================
--- src/wp-includes/class-wp-editor.php	(revision 51078)
+++ src/wp-includes/class-wp-editor.php	(working copy)
@@ -1663,19 +1663,27 @@
 		?>
 
 		( function() {
-			var init, id, $wrap;
+			var initialize = function() {
+				var init, id, inPostbox, $wrap;
+				var readyState = document.readyState;
 
-			if ( typeof tinymce !== 'undefined' ) {
-				if ( tinymce.Env.ie && tinymce.Env.ie < 11 ) {
-					tinymce.$( '.wp-editor-wrap ' ).removeClass( 'tmce-active' ).addClass( 'html-active' );
+				if ( readyState !== 'complete' ) {
 					return;
 				}
 
 				for ( id in tinyMCEPreInit.mceInit ) {
-					init = tinyMCEPreInit.mceInit[id];
-					$wrap = tinymce.$( '#wp-' + id + '-wrap' );
+					init      = tinyMCEPreInit.mceInit[id];
+					$wrap     = tinymce.$( '#wp-' + id + '-wrap' );
+					inPostbox = $wrap.parents( '.postbox' ).length > 0;
 
-					if ( ( $wrap.hasClass( 'tmce-active' ) || ! tinyMCEPreInit.qtInit.hasOwnProperty( id ) ) && ! init.wp_skip_init ) {
+					if (
+						! init.wp_skip_init &&
+						( $wrap.hasClass( 'tmce-active' ) || ! tinyMCEPreInit.qtInit.hasOwnProperty( id ) ) &&
+						(
+							( ! inPostbox && ( readyState === 'interactive' || readyState === 'complete' ) ) ||
+							( inPostbox && readyState === 'complete' )
+						)
+					) {
 						tinymce.init( init );
 
 						if ( ! window.wpActiveEditor ) {
@@ -1685,6 +1693,18 @@
 				}
 			}
 
+			if ( typeof tinymce !== 'undefined' ) {
+				if ( tinymce.Env.ie && tinymce.Env.ie < 11 ) {
+					tinymce.$( '.wp-editor-wrap ' ).removeClass( 'tmce-active' ).addClass( 'html-active' );
+				} else {
+					if ( document.readyState === 'complete' ) {
+						initialize();
+					} else {
+						document.addEventListener( 'readystatechange', initialize );
+					}
+				}
+			}
+
 			if ( typeof quicktags !== 'undefined' ) {
 				for ( id in tinyMCEPreInit.qtInit ) {
 					quicktags( tinyMCEPreInit.qtInit[id] );

#32 follow-up: @azaozz
3 years ago

  • Keywords needs-testing removed

@metalandcoffee thanks for testing, glad to hear the patch is working :)

@patkemper that line of code is just a short-circuit to avoid running the loop when the document is still loading (small speed up). If readyState !== 'interactive' is removed it means the loop will run only after the document has fully loaded, i.e. initialization of all TinyMCE instances will wait for images to finish loading.

That's not a good idea especially for the main editor on the old Edit Post screen (when using Classic Editor plugin, etc.). It can add a visible slowdown, even flicker when the page seems "ready" but the editor is still not initialized.

Perhaps what you're seeing is caused by the TinyMCE instance not being in a postbox? For plugins there is a way to prevent initializing a particular TinyMCE instance and initialize it later. Example:

$args = array(
        'quicktags' => false,
        'tinymce' => array( 'wp_skip_init' => true ),
);
wp_editor(  'Welcome to WP'  , 'test-editor', $args );

Then the plugin can decide when to initialize, could be onclick, etc. The js for the above example would be:

tinymce.init( tinyMCEPreInit.mceInit['test-editor'] );

Thinking 52133.diff is ready for commit as it seems to fix initialization of TinyMCE in postboxes.

#33 @azaozz
3 years ago

  • Owner set to azaozz
  • Resolution set to fixed
  • Status changed from reopened to closed

In 51082:

TinyMCE: Fix initialization when the editor is in a postbox by delaying it until document.readyState === 'complete'.

Props metalandcoffee, desrosj, patkemper, herrvigg, spikeuk1, dway, mkdgs, azaozz.
Fixes #52133, #52050.

#34 @azaozz
3 years ago

In 51085:

TinyMCE: Don't attempt to initialize the same instance twice. Follow up to [51082].

See #52133, #52050.

#35 in reply to: ↑ 32 ; follow-up: @patkemper
3 years ago

Replying to azaozz:

@metalandcoffee thanks for testing, glad to hear the patch is working :)

@patkemper that line of code is just a short-circuit to avoid running the loop when the document is still loading (small speed up). If readyState !== 'interactive' is removed it means the loop will run only after the document has fully loaded, i.e. initialization of all TinyMCE instances will wait for images to finish loading.

That's not a good idea especially for the main editor on the old Edit Post screen (when using Classic Editor plugin, etc.). It can add a visible slowdown, even flicker when the page seems "ready" but the editor is still not initialized.

Perhaps what you're seeing is caused by the TinyMCE instance not being in a postbox? For plugins there is a way to prevent initializing a particular TinyMCE instance and initialize it later. Example:

$args = array(
        'quicktags' => false,
        'tinymce' => array( 'wp_skip_init' => true ),
);
wp_editor(  'Welcome to WP'  , 'test-editor', $args );

Then the plugin can decide when to initialize, could be onclick, etc. The js for the above example would be:

tinymce.init( tinyMCEPreInit.mceInit['test-editor'] );

Thinking 52133.diff is ready for commit as it seems to fix initialization of TinyMCE in postboxes.

Many thanks for your hints. After some expriements, I can confirm, this is completly fixed.

I'm very glad, this bug has been fixed. Many thank to @metalandcoffee for this ticket and many thanks @azaozz for fixing this issue.

#36 in reply to: ↑ 35 @azaozz
3 years ago

Replying to patkemper:

I can confirm, this is completely fixed.

Great! Thanks for testing and confirming.

#37 @dway
3 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

I'm retrying now the patch is merged, and with a clean install of 5.8RC2 + test code provided in this ticket added in Twenty Twenty, the issue is still here. TinyMCE does not load properly and remain not editable.
Anyone can confirm ? (I reopen the ticket too).

#38 @azaozz
3 years ago

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

@dway The patches that were committed fix at least some of the reported cases/issues. If you're still experiencing problems please open a new ticket with a detailed description on how to reproduce them, error messages, etc.

Closing this again as the originally reported issue seems fixed.

Note: See TracTickets for help on using tickets.