Skip to:
Content

Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#6487 closed defect (bug) (fixed)

Mentions.js fails on wp-admin post editor

Reported by: dcavins Owned by: djpaul
Milestone: 2.3.3 Priority: normal
Severity: normal Version: 2.3.1
Component: Activity Keywords: dev-feedback
Cc:

Description

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.

Attachments (5)

mentions.js (8.2 KB) - added by dcavins 3 years ago.
Moves loadMentionsInTinyMCE into the global scope.
6487.02.patch (3.4 KB) - added by dcavins 2 years ago.
Add TinyMCE init callback via tiny_mce_before_init filter.
6487.03.patch (5.9 KB) - added by imath 2 years ago.
6487.04.patch (3.4 KB) - added by dcavins 2 years ago.
Minimal change; uses the init callback option in TinyMCE.
6487.05.patch (3.5 KB) - added by dcavins 2 years ago.

Download all attachments as: .zip

Change History (23)

#1 @DJPaul
3 years ago

  • Milestone changed from Awaiting Review to 2.3.3

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.

#2 @dcavins
3 years ago

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
3 years ago

Moves loadMentionsInTinyMCE into the global scope.

#3 @DJPaul
3 years ago

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.

This ticket was mentioned in Slack in #buddypress by hnla. View the logs.


3 years ago

This ticket was mentioned in Slack in #buddypress by djpaul. View the logs.


2 years ago

@dcavins
2 years ago

Add TinyMCE init callback via tiny_mce_before_init filter.

#6 @dcavins
2 years ago

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.

This ticket was mentioned in Slack in #buddypress by dcavins. View the logs.


2 years ago

#8 @imath
2 years ago

@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

@imath
2 years ago

#9 @DJPaul
2 years ago

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.

#10 @DJPaul
2 years ago

  • Keywords needs-refresh added; has-patch removed

@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).

#11 @dcavins
2 years ago

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
2 years ago

Minimal change; uses the init callback option in TinyMCE.

#12 @dcavins
2 years ago

  • Keywords dev-feedback added; needs-refresh removed

#13 follow-up: @imath
2 years ago

@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 ;)

This ticket was mentioned in Slack in #buddypress by djpaul. View the logs.


2 years ago

#15 in reply to: ↑ 13 @dcavins
2 years ago

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.

@dcavins
2 years ago

#16 @djpaul
2 years ago

  • Owner set to djpaul
  • Resolution set to fixed
  • Status changed from new to closed

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.

#17 @djpaul
2 years ago

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.

#18 @dcavins
2 years ago

Thanks for making this go, @DJPaul!

Note: See TracTickets for help on using tickets.