Last modified: 2012-11-28 22:47:05 UTC
This is a followup to bug 33497. On further consultation with some JavaScript developers, WikiEditor seems to be doing its beforeunload handler incorrectly, or at least not optimally. The style of: window.onbeforeunload = function() { return something(); }; assumes there is only one such handler on beforeunload, and will clash with other beforeunload handlers in extensions, such as this style: $(window) .bind('beforeunload', myFunc); WikiEditor's logic should be rewritten to play nicely with other handlers.
This is in IE8.
Agreed that this logic is in jQuery and we should use it. However I don't think it's causing any problems right now. Let's look at the snippet in question: context.fallbackWindowOnBeforeUnload = window.onbeforeunload; window.onbeforeunload = function() { context.$textarea.val( context.$textarea.textSelection( 'getContents' ) ); if ( context.fallbackWindowOnBeforeUnload ) { return context.fallbackWindowOnBeforeUnload(); } }; It preserves the previous value, including support for multiple levels. If this is broken, it's not because of WikiEditor, since it doesn't overwrite itself. If this handler isn't called, it's because another script removes it, not the other way around. Ironically, it's actually $(window).bind that overwrites any pre-existing on-* handler. The normal event queue doesn't work cross-browser (in that jQuery can't use window.addEventListener internally) so it keeps its own load queue and in that jQuery boldly overwrites any existing handler. Solution is the same though, as long as everything uses jQuery the queue will stay in tact.
The offending code is actually in extensions/Vector/modules/ext.vector.editWarning.js . There is one occurrence in WikiEditor, but it's in a deprecated module. The code in EditWarning has been very carefully calibrated not to break the back/forward cache in Firefox, so I don't want to touch it right before a release. I'll submit a patch here that you can try out, and if it doesn't break Firefox's bfcache, we can put it in 1.20.
Created attachment 9948 [details] Untested patch
(In reply to comment #4) > Created attachment 9948 [details] > Untested patch Is there any reason to keep this out of 1.19?
Can somebody please review the patch and afterwards replace the "patch-need-review" keyword by "patch-reviewed"?
@Roan: Please push to gerrit for review. That'll make it easier to test and build upon.
(In reply to comment #7) > @Roan: Please push to gerrit for review. That'll make it easier to test and > build upon. Rewrote the patch against current master and submitted as https://gerrit.wikimedia.org/r/#/c/22745/
(In reply to comment #8) > https://gerrit.wikimedia.org/r/#/c/22745/ Status Merged
