Mentions.js fails on wp-admin post editor

[dcavins]

When using the post editor in wp-admin, this error is cropping up:
TypeError: null is not an object (evaluating 'window.tinyMCE.activeEditor.contentDocument') on mentions.js:250

Console tells me that window.tinyMCE.activeEditor is null.

In mentions.js, I'm reading the following code:

loadMentionsInTinyMCE = function() {
			if ( loadAttempts < 4 || ! $( 'body' ).hasClass( 'wp-admin' ) ) {
				loadAttempts++;

				if ( typeof window.tinyMCE === 'undefined' || window.tinyMCE.activeEditor === null || typeof window.tinyMCE.activeEditor === 'undefined' ) {
					setTimeout( loadMentionsInTinyMCE, 500 );
					return;
				}
			}

			$( window.tinyMCE.activeEditor.contentDocument.activeElement )
				.atwho( 'setIframe', $( '#content_ifr' )[0] )
				.bp_mentions( users );
		};

which, if I understand correctly, it will delay the .atwho.bp_mentions trigger up to five times for half a second each time, but on the sixth time, it's gonna roll anyway.

I'm adding a patch that only runs if the object's property is set, and gives up after the five iterations, but I'm unfamiliar with this functionality, so I have some doubts that I really understand what's happening here.

[DJPaul]

Confirmed; it doesn't seem to obviously *break* the functionality, but it must have some affect that's not immediately obvious at first glance.

I'd like to suggest fixing this issue in a 2.3 release because this definitely did used to work, so I guess something in our JS (or WP's) changed. I am not too worried if people think otherwise as the functionality seems to work.

[dcavins]

With my proposed fix, here’s the behavior I’m seeing. If you load the post edit page with the “visual” editor preselected, then bp_mentions gets attached to the wp_editor iframe as expected. If you switch to the text editor, bp_mentions is attached there as well. If the page loads with the text pane visible, bp_mentions is attached to the text editor as expected, but loadMentionsInTinyMCE uses up its tries and gives up trying to attach the iframe, and when you switch to the visual editor, bp_mentions doesn’t work.

Maybe the better bet would be to attach to the wp_editor iframe when it's loaded (from @boone: ​http://teleogistic.net/2012/02/09/using-init-callbacks-with-tinymce-and-wp_editor-in-wordpress/).

By adding this action:

function bp_add_mentions_on_tiny_mce_init( $initArray ) {
  $initArray['setup'] = 'function(ed) {
        ed.onInit.add(
          function(ed) {
            loadMentionsInTinyMCE();
            });
    }';

  return $initArray;
}
add_filter( 'tiny_mce_before_init', 'bp_add_mentions_on_tiny_mce_init' );

to a functions file, and using the attached modified mentions.js, loadMentionsInTinyMCE() is fired when the editor is loaded. This might be worth looking into?

[dcavins]

Moves loadMentionsInTinyMCE into the global scope.

[DJPaul]

I was just thinking we fix the original problem and worry about support for switching between tabs at some point in the future, but if you want to fix it properly now, that's very responsible of you.

Your patch is actually the entire file :D but I get the idea from your example. If that way is the only way to do it, I can live with putting it into global scope, but let's namespace it like so:

window.bp = window.wp || {};
window.bp.mentions = window.bp.mentions || {};

or bp.activity.mentions, we haven't had a discussion on exactly how we want to store this sort of thing in our JS.

[dcavins]

Add TinyMCE init callback via tiny_mce_before_init filter.

[dcavins]

I've added a refreshed patch that uses a TinyMCE 4-style init callback. (So it will only work with WP 3.9+. Is that OK?)

Any feedback is welcome. Also testing with various setups, please.

[imath]

@dcavins i had a look at your patch, the fix is working fine, great work :)

I have some suggestions about how all this is organized:

• i think we should only load the "tinymce" part if the editor has tinymce available, so using the action wp_enqueue_editor can help us to get this information.
• As a result, i suggest to put your fix in a specific file > mentions.tinymce.js
• it may not be directly relative to this ticket, but i needed to register the 'bp-mentions' script into wp scripts so i've added a bunch of functions to use the core-cssjs functions that are pretty helpful.

All these suggestions are available in 6487.03.patch

[DJPaul]

Now our minimum WP version supported is 3.8, fixing this using something in 3.9+ is, I think, okay. Our fix for 3.8 users can be to get them to update WordPress.

[DJPaul]

@imath Moving the bp-mentions scripts enqueue into bp_core_register_common_scripts makes sense but is not necessarily required to fix this ticket. Let's do that separately (should be quick since you've already written it).

I am not yet convinced by the addition of a new JS file. Especially for a 2.3.x release. I would like to hold off doing that generally until we have modular JS which we can have in multiple files, and build into a single file with a Grunt task.

We do need the loadAttempts stuff back in the patch, at any rate, because it handles if window.tinyMCE tinyMCE.activeEditor is not loaded *yet* (slow internet, slow browsers, eyc).

[dcavins]

I agree that, in the short term, we should enqueue this all as one js file pending some sort of clever loader thing in a future BP.

We shouldn't need the loadAttempts stuff anymore given that the big switch here is to have tinyMCE perform our new setup callback when it completes its init run.

I'll refresh @imath's patch above to make it slightly more minimal--so that we're just fixing this null object problem here.

[dcavins]

Minimal change; uses the init callback option in TinyMCE.

[imath]

@DJPaul i agree my patch was a bit too much for a 2.3.x milestone.

@dcavins i haven't tested the patch, but i think i'd put the global bp / window.bp = window.bp || {} thing at the top of the file. Then i don't think we need to use window.bp inside the function, using directly bp should be ok ;)

[dcavins]

Replying to imath:

@dcavins i haven't tested the patch, but i think i'd put the global bp / window.bp = window.bp || {} thing at the top of the file. Then i don't think we need to use window.bp inside the function, using directly bp should be ok ;)

Ha ha, I really don't have any idea what you mean, but I tried to do something anyway, since DJPaul was calling the progress on this ticket slow, ha ha.

So, this new version seems to work in every context and doesn't throw errors. Let me know what we need to do to make this a checked-off task.

[djpaul]

In 10053:

Activity: fix @mentions in wp-admin post editor.

If the screen loads with the text editor visible, bp_mentions is attached to the text editor as expected, but loadMentionsInTinyMCE uses up its tries and gives up trying to attach the iframe, so when you switch to the visual editor, bp_mentions didn't work.

This change removes the looped loadMentionsInTinyMCE approach and instead hooks into TinyMCE configuration to specify an init callback, where we then load bp_mentions.

Fixes #6487 (branch). Props dcavins, imath, DJPaul.

[djpaul]

In 10054:

Activity: fix @mentions in wp-admin post editor.

If the screen loads with the text editor visible, bp_mentions is attached to the text editor as expected, but loadMentionsInTinyMCE uses up its tries and gives up trying to attach the iframe, so when you switch to the visual editor, ` doesn't work.

This change removes the looped loadMentionsInTinyMCE approach and instead hooks into TinyMCE configuration to specify an init callback, where we then load bp_mentions.

Fixes #6487 (trunk). Props dcavins, imath, DJPaul.

[dcavins]

Thanks for making this go, @DJPaul!