jenkins-bot has submitted this change and it was merged. Change subject: Reduce complexity around onbeforeunload handler ......................................................................
Reduce complexity around onbeforeunload handler We used to attempt to not break Firefox's bfcache, but this didn't really work very well, and it's not clear that avoiding bfcache breakage is even a good idea. It's more sensible and consistent to deliberately break bfcache while VE is active. On top of that, the Firefox people believe that our trick shouldn't even have worked to begin with: https://bugzilla.mozilla.org/show_bug.cgi?id=1102664 Because the onbeforeunload handler is removed when VE is deactivated, bfcache still works if you first click Read, then navigate away. Also clean up the management of the unload handler, using addEventListener() and removing the return value fallback code that is needed for beforeunload but not unload. For beforeunload this is harder to clean up because the addEventListener()-based API for returning a value isn't consistent across browsers. Change-Id: Ie4fe9ea3a59a54ba462733aa5e42bfc0ed5b15eb --- M modules/ve-mw/init/targets/ve.init.mw.ViewPageTarget.js 1 file changed, 17 insertions(+), 79 deletions(-) Approvals: Trevor Parscal: Looks good to me, approved jenkins-bot: Verified diff --git a/modules/ve-mw/init/targets/ve.init.mw.ViewPageTarget.js b/modules/ve-mw/init/targets/ve.init.mw.ViewPageTarget.js index b903c0b..372617c 100644 --- a/modules/ve-mw/init/targets/ve.init.mw.ViewPageTarget.js +++ b/modules/ve-mw/init/targets/ve.init.mw.ViewPageTarget.js @@ -35,9 +35,7 @@ this.toolbarSaveButton = null; this.saveDialog = null; this.onBeforeUnloadFallback = null; - this.onBeforeUnloadHandler = null; - this.onUnloadFallback = null; - this.onUnloadHandler = null; + this.onUnloadHandler = this.onUnload.bind( this ); this.active = false; this.activating = false; this.deactivating = false; @@ -1549,24 +1547,16 @@ }; /** - * Add onunload and unbeforeunload handlesr. + * Add onunload and onbeforeunload handlesr. * * @method */ ve.init.mw.ViewPageTarget.prototype.setupUnloadHandlers = function () { - // Remember any already set on handlers + // Remember any already set beforeunload handler this.onBeforeUnloadFallback = window.onbeforeunload; - this.onUnloadFallback = window.onunload; // Attach our handlers - window.onbeforeunload = this.onBeforeUnloadHandler = this.onBeforeUnload.bind( this ); - window.onunload = this.onUnloadHandler = this.onUnload.bind( this ); - - // Attach page show handlers - if ( window.addEventListener ) { - window.addEventListener( 'pageshow', this.onPageShow.bind( this ), false ); - } else if ( window.attachEvent ) { - window.attachEvent( 'pageshow', this.onPageShow.bind( this ) ); - } + window.onbeforeunload = this.onBeforeUnload.bind( this ); + window.addEventListener( 'unload', this.onUnloadHandler ); }; /** * Remove onunload and onbeforunload handlers. @@ -1574,9 +1564,10 @@ * @method */ ve.init.mw.ViewPageTarget.prototype.tearDownUnloadHandlers = function () { - // Restore whatever previous onunload/onbeforeunload hooks existed + // Restore whatever previous onbeforeunload hook existed window.onbeforeunload = this.onBeforeUnloadFallback; - window.onunload = this.onUnloadFallback; + this.onBeforeUnloadFallback = null; + window.removeEventListener( 'unload', this.onUnloadHandler ); }; /** @@ -1648,53 +1639,25 @@ }; /** - * Handle page show event. - * - * @method - */ -ve.init.mw.ViewPageTarget.prototype.onPageShow = function () { - // Re-add onunload and onbeforeunload handlers - window.onbeforeunload = this.onBeforeUnloadHandler; - window.onunload = this.onUnloadHandler; -}; - -/** * Handle before unload event. * * @method */ ve.init.mw.ViewPageTarget.prototype.onBeforeUnload = function () { - var fallbackResult, - message, - onBeforeUnloadHandler = this.onBeforeUnloadHandler; + var fallbackResult; // Check if someone already set on onbeforeunload hook if ( this.onBeforeUnloadFallback ) { // Get the result of their onbeforeunload hook fallbackResult = this.onBeforeUnloadFallback(); - } - // Check if their onbeforeunload hook returned something - if ( fallbackResult !== undefined ) { - // Exit here, returning their message - message = fallbackResult; - } else { - // Override if submitting - if ( this.submitting ) { - return undefined; - } - // Check if there's been an edit - if ( this.getSurface() && this.edited && mw.user.options.get( 'useeditwarning' ) ) { - // Return our message - message = ve.msg( 'visualeditor-viewpage-savewarning' ); + // If it returned something, exit here and return their message + if ( fallbackResult !== undefined ) { + return fallbackResult; } } - // Unset the onbeforeunload handler so we don't break page caching in Firefox - window.onbeforeunload = null; - if ( message !== undefined ) { - // ...but if the user chooses not to leave the page, we need to rebind it - setTimeout( function () { - window.onbeforeunload = onBeforeUnloadHandler; - } ); - return message; + // Check if there's been an edit + if ( this.getSurface() && this.edited && !this.submitting && mw.user.options.get( 'useeditwarning' ) ) { + // Return our message + return ve.msg( 'visualeditor-viewpage-savewarning' ); } }; @@ -1704,36 +1667,11 @@ * @method */ ve.init.mw.ViewPageTarget.prototype.onUnload = function () { - var fallbackResult, - message, - onUnloadHandler = this.onUnloadHandler; - // Check if someone already set on onunload hook - if ( this.onUnloadFallback ) { - // Get the result of their onunload hook - fallbackResult = this.onBeforeUnloadFallback(); - } - // Check if their onunload hook returned something - if ( fallbackResult !== undefined ) { - // Exit here, returning their message - message = fallbackResult; - } else { - // Override if submitting - if ( this.submitting ) { - return undefined; - } + if ( !this.submitting ) { ve.track( 'mwedit.abort', { type: this.edited ? 'unknown-edited' : 'unknown', mechanism: 'navigation' } ); - } - // Unset the onunload handler so we don't break page caching in Firefox - window.onunload = null; - if ( message !== undefined ) { - // ...but if the user chooses not to leave the page, we need to rebind it - setTimeout( function () { - window.onunload = onUnloadHandler; - } ); - return message; } }; -- To view, visit https://gerrit.wikimedia.org/r/177107 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: merged Gerrit-Change-Id: Ie4fe9ea3a59a54ba462733aa5e42bfc0ed5b15eb Gerrit-PatchSet: 3 Gerrit-Project: mediawiki/extensions/VisualEditor Gerrit-Branch: master Gerrit-Owner: Catrope <[email protected]> Gerrit-Reviewer: Catrope <[email protected]> Gerrit-Reviewer: Jforrester <[email protected]> Gerrit-Reviewer: Trevor Parscal <[email protected]> Gerrit-Reviewer: jenkins-bot <> _______________________________________________ MediaWiki-commits mailing list [email protected] https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
