WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

#19189 closed defect (bug) (fixed)

thickbox.js .trigger("unload") causes conflicts

Reported by: ocean90 Owned by: azaozz
Milestone: 3.3 Priority: normal
Severity: major Version: 3.3
Component: Administration Keywords: has-patch
Focuses: Cc:

Description

In current trunk the closing of the upload thickbox seems to lag. It's a bit slower as in 3.2.

Sergey had the time to take a screenshot: http://i42.tinypic.com/2sblbn6.jpg

Maybe there is something which can be improved for 3.3.

Attachments (4)

19189.addEventListener.diff (535 bytes) - added by duck_ 4 years ago.
19189.addEventListener.2.diff (758 bytes) - added by duck_ 4 years ago.
19189.triggerHandler.diff (840 bytes) - added by duck_ 4 years ago.
19189-2.patch (1.2 KB) - added by azaozz 4 years ago.

Download all attachments as: .zip

Change History (38)

comment:1 @ocean904 years ago

nacin: it's made worse by jQuery 1.7.

I couldn't verify this.

comment:2 @ocean904 years ago

azaozz: hmm, looks like on every closing of thickbox on the edit screen it triggers 2 "wp-remove-post-lock" XHR calls

comment:3 @nacin4 years ago

  • Owner set to nacin
  • Status changed from new to reviewing

comment:4 @nacin4 years ago

  • Owner nacin deleted

comment:5 follow-up: @duck_4 years ago

This is caused by .trigger("unload") in tb_remove(). The unload event is bubbled up to window causing the post lock code to fire. Using plain window.addEventListener instead of jQuery's .unload() in autosave.js fixed this for me in testing, but I don't know much about the cross browser implications of this.

I'm not really sure why, but our callback being fired twice only occurs twice since jQuery 1.7 (bug?) which is why nacin noticed that it was made worse.

comment:6 in reply to: ↑ 5 @duck_4 years ago

Replying to duck_:

I'm not really sure why, but our callback being fired twice only occurs twice since jQuery 1.7 (bug?) which is why nacin noticed that it was made worse.

jQuery('#TB_window,#TB_overlay,#TB_HideSelect').trigger("unload")

Two elements, #TB_window and #TB_overlay, are matched so jQuery.event.trigger is called twice. Since jQuery 1.7 the unload handler can be called more than once, see commit and ticket. So we get the event propagation up to window happening twice.

comment:7 follow-up: @SergeyBiryukov4 years ago

Tested 19189.addEventListener.diff in Firefox 8, Chrome 15, IE 8, Opera 11.52.

Looks good everywhere but IE 8:

Message: Object doesn't support this property or method
Line: 51
Char: 2
Code: 0
URL: http://trunk.wordpress/wp-includes/js/autosave.dev.js?ver=20111013

comment:8 in reply to: ↑ 7 @duck_4 years ago

Replying to SergeyBiryukov:

Tested 19189.addEventListener.diff in Firefox 8, Chrome 15, IE 8, Opera 11.52.

Looks good everywhere but IE 8:

Thanks for testing. I thought that would be the case. Updated patch to check addEventListener exists before using it, see element.addEventListener legacy IE.

comment:9 @SergeyBiryukov4 years ago

19189.addEventListener.2.diff appears to be working in all of the four browsers and in Safari 5 as well.

comment:10 @SergeyBiryukov4 years ago

  • Keywords has-patch added; needs-patch removed

comment:11 @duck_4 years ago

Unfortunately this is cropping up elsewhere, see #19212, so I don't think one off fixes like this will be the solution. Can we remove .trigger("unload") from thickbox?

comment:12 @duck_4 years ago

  • Summary changed from Upload Thickbox fading issue to thickbox.js .trigger("unload") causes conflicts

comment:13 @GaryJ4 years ago

Having already stumbled across this issue (closing thickbox stopped other script actions from working on a theme admin page), I've got the following in a theme and I'm getting no new problems on WP 3.3b3:

jQuery('#TB_window, #TB_overlay, #TB_HideSelect').live('mytheme.unload', mytheme.thickbox_fix);

/**
 * Fix the use of unload() when Thickbox window closes.
 *
 * Credit: http://themeforest.net/forums/thread/wordpress-32-admin-area-thickbox-triggering-unload-event/46916?page=1#434388
 * More info: http://wordpress.org/support/topic/wp-32-thickbox-jquery-ui-tabs-conflict
 *
 * @return {boolean} Returns false
 */
thickbox_fix: function (event) {
	'use strict';
	event.stopPropagation();
	event.stopImmediatePropagation();
	return false;
},
...

If removing trigger('unload') is not possible, can you at least namespace it so it doesn't unload everything else?

comment:14 follow-up: @duck_4 years ago

If we don't want to completely remove .trigger("unload") we could modify thickbox to use .triggerHandler() to stop event bubbling. Also, thickbox was written before .trigger() bubbled events up the DOM anyway.

@duck_4 years ago

comment:15 @azaozz4 years ago

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

In [19264]:

Thickbox: do .triggerHandler() instead of .trigger(), props duck_, fixes #19189

comment:16 in reply to: ↑ 14 ; follow-up: @azaozz4 years ago

Replying to duck_:

Confirmed, using triggerHandler() fixes it nicely. There's no point of trigger() there anyways, no default action in the browser. Also seems this is only needed on #TB_window, there's no events attached to TB_overlay.

comment:17 in reply to: ↑ 16 ; follow-up: @duck_4 years ago

Replying to azaozz:

Also seems this is only needed on #TB_window, there's no events attached to TB_overlay.

I wrote the patch to trigger unload handlers on all three for back compat in case third-party JavaScript was targeting #TB_overlay or #TB_HideSelect. Can we really just assume none do?

comment:18 in reply to: ↑ 17 @azaozz4 years ago

Replying to duck_:

Right. The problem is that the "unload" event fires only on "window". What Thickbox did is hacky in the first place, firing "unload" on DIVs. That's why all the other scripts that use window.unload have problems when used together with Thickbox (btw this should have been a custom event).

Internally this "unload" is used only for the #TB_window DIV, seems it was fired on the other DIVs because they all were selected at the time. Don't think any other script uses/expects "unload" on anything else than "window".

comment:19 @aesqe4 years ago

  • Cc aesqe@… added

comment:20 follow-up: @aesqe4 years ago

Could this please be changed into a custom .trigger() event? Because it looks like .triggerHandler("unload") does nothing at all in 3.3-beta4 with jQuery 1.7.1.

Also, .triggerHandler() doesn't work with .live(), so what's the point of having it there, anyway?

These both worked in 3.2.1:

$("#TB_overlay, #TB_window").live("unload", function(e){
	console.debug(this.id + " live unload");
});

$(window).bind("unload", function(e){
	console.debug("window unload");
});

Before 3.3-beta4 and jQuery 1.7.1, $("#TB_window").triggerHandler("unload") was registered with window.unload, now it's not :/

Any thoughts?

comment:21 in reply to: ↑ 20 ; follow-ups: @azaozz4 years ago

Replying to aesqe:

Could this please be changed into a custom .trigger() event? Because it looks like .triggerHandler("unload") does nothing at all in 3.3-beta4 with jQuery 1.7.1.

This seems to work properly in 3.3-beta4 with jQuery 1.7.1:

jQuery('#TB_window').unload(function(e){
  console.log(e);
});

Also, .triggerHandler() doesn't work with .live(), so what's the point of having it there, anyway?

This is a jQuery limitation/decision. Actually .live() is being phased out in favor of .delegate().

Before 3.3-beta4 and jQuery 1.7.1, $("#TB_window").triggerHandler("unload") was registered with window.unload, now it's not :/

Don't think so. The unload event only fires on window. The bug in Thickbox was that it used to fire unload on a div which then propagated to window and tricked all handlers on jQuery(window).unload() into running despite that the window didn't unload.

.triggerHandler() doesn't propagate up the DOM.

I see the problems in trying to use the Thickbox unload. Wondering if it's possible to both keep full back-compat and avoid triggering window.unload at the same time. Custom event would work but will require all plugins that use it to be updated.

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

comment:22 in reply to: ↑ 21 @GaryJ4 years ago

Replying to azaozz:

Actually .live() is being phased out in favor of .delegate().

Actually, bind(), live(), and delegate() are all recommended to be replaced with on().

comment:23 in reply to: ↑ 21 @aesqe4 years ago

Replying to azaozz:

This seems to work properly in 3.3-beta4 with jQuery 1.7.1:

jQuery('#TB_window').unload(function(e){
  console.log(e);
});

Ugh, that doesn't work for me - I've tried it in latest Firefox, Chrome and Opera and nothing shows up in the console log. It only works if I change it to trigger('unload') in thickbox.js and use live() to bind it. I've used a fresh WP3.3-beta4 install, no WP plugins, no browser plugins. And isn't .unload() just an alias of bind("unload")? How can it then be bound to an element that's not been created yet? :|

Before 3.3-beta4 and jQuery 1.7.1, $("#TB_window").triggerHandler("unload") was registered with window.unload, now it's not :/

Don't think so. The unload event only fires on window. The bug in Thickbox was that it used to fire unload on a div which then propagated to window and tricked all handlers on jQuery(window).unload() into running despite that the window didn't unload.

.triggerHandler() doesn't propagate up the DOM.

Sorry about that, you're right, that worked on 3.2.1, not on 3.3.

I see the problems in trying to use the Thickbox unload. Wondering if it's possible to both keep full back-compat and avoid triggering window.unload at the same time. Custom event would work but will require all plugins that use it to be updated.

Thanks :)

comment:24 @GaryJ4 years ago

The almost equivalent code to azaozz's snippet, for jQuery 1.7+, using a delegated event (as #TB_window doesn't exist at the time the handler is being attached, as you pointed out) would be:

jQuery(document).on('unload', '#TB_window', function(e){
  console.log('#TB_window unloaded.');
  console.log(e);
});
Last edited 4 years ago by GaryJ (previous) (diff)

comment:25 follow-up: @aesqe4 years ago

OK, I really feel a bit stupid for writing this, and I apologize in advance, but, azaozz and GaryJ, have you tested your suggestions with beta 4? Because I've tried it on three different computers, in different browsers and I get nothing in the console.

Again, sorry for bugging so much, but this is driving me crazy :(

comment:26 in reply to: ↑ 25 @azaozz4 years ago

Replying to aesqe:

...sorry for bugging so much, but this is driving me crazy :(

Not your fault, sorry I didn't explain it better above. jQuery('#TB_window').unload... will only work after thickbox is opened (#TB_window exists in the DOM), jQuery(document).on('unload', '#TB_window',... is not going to work since we trigger the 'unload' with triggerHandler that doesn't propagate. These are not solutions for the problem, more like possible ideas to try to make it work.

Unfortunately the only way to stay back-compat here seems to be to revert this change and fix any improperly triggered window.unload on a case by case basis. This is "the last nail" for thickbox, we either have to completely remove any use of it from core or replace it with better script in 3.4.

comment:27 follow-up: @GaryJ4 years ago

The jQuery docs say that unload() method should only have ever been attached to the window - so why was it ever triggered for thickbox?

comment:28 in reply to: ↑ 27 @azaozz4 years ago

Replying to GaryJ:

It's a bug. The 'unload' event only exists for 'window'. Think it was used before jQuery was force-bubbling events up the DOM so it was more of a custom event only on #TB_window, that unfortunately matches the "real" unload. Later on other JS started using it too so now we have to still support it...

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

comment:29 @azaozz4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

comment:30 @azaozz4 years ago

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

Fixed in [19451]: Revert [19264] to keep back-compat. Any improperly fired window.unload will have to be dealt with on a case by case basis.

comment:31 @azaozz4 years ago

Here's the code from autosave.js that deals with this:

$(window).unload( function(e) {
  if ( e.target && e.target.nodeName != '#document' )
    return;
...
Last edited 4 years ago by azaozz (previous) (diff)

comment:32 @azaozz4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

After a chat with @duck_ in IRC, thinking best would be to make that 'unload' a custom event. Apparently this Thickbox bug breaks some UI components too so we can't leave it in...

Plugins that use the current event can easily change to a custom event, however fixing this in UI is not possible.

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

@azaozz4 years ago

comment:33 @aesqe4 years ago

thanks, azaozz :)

comment:34 @azaozz4 years ago

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

In [19456]:

Rename the problematic "unload" event in Thickbox to "tb_unload", making it a true custom event, fixes #19189

Note: See TracTickets for help on using tickets.