From d99dd4040e54d881e4eb35fa56bdc9ed19dde3f5 Mon Sep 17 00:00:00 2001 From: Nicholas Ray Date: Fri, 22 Oct 2021 10:49:56 -0600 Subject: [PATCH] [BREAKING] Change cleanup API of checkboxHack Before this commit, each bind function would return an object containing a reference to the function used in the event listener creation with the intention that this could later be used to cleanup the event listeners. This is problematic for the following reasons: * In order to cleanup the event listener, it forces the client to know which element the event listener was added to (button, checkbox, document, window, etc.),. the event type that was used ('click', 'focusin', etc), and whether or not the "useCapture" param was also used. The `removeEventListener` API won't remove the correct event listener unless these three pieces of info are known by the client [1]. * Because the above point is tricky to get right, numerous potential memory leaks were possible with the `unbind` function as the `useCapture` param wasn't being passed in some cases. This commit aims to simplify the cleanup API by returning an anonymous function in each bind function that the client can call to cleanup the relevant event listeners without knowing any of the implementation details. This also makes the `unbind` function unnecessary. ** This is a breaking change to the checkboxHack API ** but because there aren't any known clients of the return values [2], a formal deprecation process was assumed to be unnecessary. Note gadgets should not be using the internal function mw.loader.require so these were not considered. [1] https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/removeEventListener#matching_event_listeners_for_removal [2] https://codesearch.wmcloud.org/search/?q=checkboxHack&i=nope&files=&excludeFiles=&repos= Bug: T291096 Change-Id: Ia2d79f27dd81fdc74a44829da3ca472f4c01d4b5 --- RELEASE-NOTES-1.38 | 4 + .../src/mediawiki.page.ready/checkboxHack.js | 110 +++++++----------- 2 files changed, 45 insertions(+), 69 deletions(-) diff --git a/RELEASE-NOTES-1.38 b/RELEASE-NOTES-1.38 index 7778d280b23..abe46c326a4 100644 --- a/RELEASE-NOTES-1.38 +++ b/RELEASE-NOTES-1.38 @@ -188,6 +188,10 @@ because of Phabricator reports. removed. * UserLoadOptions, UserSaveOptions and UserResetAllOptions hooks, deprecated since 1.37, were removed. +* The return values for each `bind` function in checkboxHack.js has been changed + from an object to a function. In addition, the `unbind` function has been + removed. A deprecation process was assumed unnecessary as there were no known + usages. * … === Deprecations in 1.38 === diff --git a/resources/src/mediawiki.page.ready/checkboxHack.js b/resources/src/mediawiki.page.ready/checkboxHack.js index dd280c2b2ac..6af7a2e6c86 100644 --- a/resources/src/mediawiki.page.ready/checkboxHack.js +++ b/resources/src/mediawiki.page.ready/checkboxHack.js @@ -106,20 +106,6 @@ * [0]: https://developer.mozilla.org/docs/Web/HTML/Element/details */ -/** - * Checkbox hack listener state. - * - * TODO: Change to @-typedef when we switch to JSDoc - * - * @class {Object} CheckboxHackListeners - * @property {Function} [onUpdateAriaExpandedOnInput] - * @property {Function} [onToggleOnClick] - * @property {Function} [onToggleOnSpaceEnter] - * @property {Function} [onDismissOnClickOutside] - * @property {Function} [onDismissOnFocusLoss] - * @ignore - */ - /** * Revise the button's `aria-expanded` state to match the checked state. * @@ -157,7 +143,7 @@ function updateAriaExpanded( checkbox, button ) { * @ignore */ function setCheckedState( checkbox, checked ) { - /** @type {Event} */ + /** @type {Event} @ignore */ var e; checkbox.checked = checked; // Chrome and Firefox sends the builtin Event with .bubbles == true and .composed == true. @@ -215,14 +201,17 @@ function dismissIfExternalEventTarget( checkbox, button, target, event ) { * * @param {HTMLInputElement} checkbox * @param {HTMLElement} button - * @return {CheckboxHackListeners} + * @return {function(): void} Cleanup function that removes the added event listeners. * @ignore */ function bindUpdateAriaExpandedOnInput( checkbox, button ) { var listener = updateAriaExpanded.bind( undefined, checkbox, button ); // Whenever the checkbox state changes, update the `aria-expanded` state. checkbox.addEventListener( 'input', listener ); - return { onUpdateAriaExpandedOnInput: listener }; + + return function () { + checkbox.removeEventListener( 'input', listener ); + }; } /** @@ -230,7 +219,7 @@ function bindUpdateAriaExpandedOnInput( checkbox, button ) { * * @param {HTMLInputElement} checkbox * @param {HTMLElement} button - * @return {CheckboxHackListeners} + * @return {function(): void} Cleanup function that removes the added event listeners. * @ignore */ function bindToggleOnClick( checkbox, button ) { @@ -241,7 +230,10 @@ function bindToggleOnClick( checkbox, button ) { setCheckedState( checkbox, !checkbox.checked ); } button.addEventListener( 'click', listener, true ); - return { onToggleOnClick: listener }; + + return function () { + button.removeEventListener( 'click', listener, true ); + }; } /** @@ -249,12 +241,12 @@ function bindToggleOnClick( checkbox, button ) { * * @param {HTMLInputElement} checkbox * @param {HTMLElement} button - * @return {CheckboxHackListeners} + * @return {function(): void} Cleanup function that removes the added event listeners. * @ignore */ function bindToggleOnSpaceEnter( checkbox, button ) { - function onToggleOnSpaceEnter( /** @type {KeyboardEvent} */ event ) { + function onToggleOnSpaceEnter( /** @type {KeyboardEvent} @ignore */ event ) { // Only handle SPACE and ENTER. if ( event.key !== ' ' && event.key !== 'Enter' ) { return; @@ -264,7 +256,10 @@ function bindToggleOnSpaceEnter( checkbox, button ) { } button.addEventListener( 'keydown', onToggleOnSpaceEnter, true ); - return { onToggleOnSpaceEnter: onToggleOnSpaceEnter }; + + return function () { + button.removeEventListener( 'keydown', onToggleOnSpaceEnter, true ); + }; } /** @@ -275,13 +270,16 @@ function bindToggleOnSpaceEnter( checkbox, button ) { * @param {HTMLInputElement} checkbox * @param {HTMLElement} button * @param {Node} target - * @return {CheckboxHackListeners} + * @return {function(): void} Cleanup function that removes the added event listeners. * @ignore */ function bindDismissOnClickOutside( window, checkbox, button, target ) { var listener = dismissIfExternalEventTarget.bind( undefined, checkbox, button, target ); window.addEventListener( 'click', listener, true ); - return { onDismissOnClickOutside: listener }; + + return function () { + window.removeEventListener( 'click', listener, true ); + }; } /** @@ -292,7 +290,7 @@ function bindDismissOnClickOutside( window, checkbox, button, target ) { * @param {HTMLInputElement} checkbox * @param {HTMLElement} button * @param {Node} target - * @return {CheckboxHackListeners} + * @return {function(): void} Cleanup function that removes the added event listeners. * @ignore */ function bindDismissOnFocusLoss( window, checkbox, button, target ) { @@ -300,7 +298,10 @@ function bindDismissOnFocusLoss( window, checkbox, button, target ) { // listener on the target would be preferable, but this interferes with the click listener. var listener = dismissIfExternalEventTarget.bind( undefined, checkbox, button, target ); window.addEventListener( 'focusin', listener, true ); - return { onDismissOnFocusLoss: listener }; + + return function () { + window.removeEventListener( 'focusin', listener, true ); + }; } /** @@ -317,51 +318,23 @@ function bindDismissOnFocusLoss( window, checkbox, button, target ) { * @param {HTMLElement} button The visible label icon associated with the checkbox. This button * toggles the state of the underlying checkbox. * @param {Node} target The Node to toggle visibility of based on checkbox state. - * @return {CheckboxHackListeners} + * @return {function(): void} Cleanup function that removes the added event listeners. * @ignore */ function bind( window, checkbox, button, target ) { - var spaceHandlers = bindToggleOnSpaceEnter( checkbox, button ); - // ES6: return Object.assign( bindToggleOnSpaceEnter( checkbox, button ), ... ); - // https://caniuse.com/#feat=mdn-javascript_builtins_object_assign - return { - onUpdateAriaExpandedOnInput: bindUpdateAriaExpandedOnInput( checkbox ).onUpdateAriaExpandedOnInput, - onToggleOnClick: bindToggleOnClick( checkbox, button ).onToggleOnClick, - onToggleOnSpaceEnter: spaceHandlers.onToggleOnSpaceEnter, - onKeydownSpaceEnter: spaceHandlers.onKeydownSpaceEnter, - onDismissOnClickOutside: bindDismissOnClickOutside( window, checkbox, button, target ).onDismissOnClickOutside, - onDismissOnFocusLoss: bindDismissOnFocusLoss( window, checkbox, button, target ).onDismissOnFocusLoss - }; -} + var cleanups = [ + bindUpdateAriaExpandedOnInput( checkbox, button ), + bindToggleOnClick( checkbox, button ), + bindToggleOnSpaceEnter( checkbox, button ), + bindDismissOnClickOutside( window, checkbox, button, target ), + bindDismissOnFocusLoss( window, checkbox, button, target ) + ]; -/** - * Free all set listeners. - * - * @param {Window} window - * @param {HTMLInputElement} checkbox The underlying hidden checkbox that controls target - * visibility. - * @param {HTMLElement} button The visible label icon associated with the checkbox. This button - * toggles the state of the underlying checkbox. - * @param {CheckboxHackListeners} listeners - * @return {void} - * @ignore - */ -function unbind( window, checkbox, button, listeners ) { - if ( listeners.onDismissOnFocusLoss ) { - window.removeEventListener( 'focusin', listeners.onDismissOnFocusLoss ); - } - if ( listeners.onDismissOnClickOutside ) { - window.removeEventListener( 'click', listeners.onDismissOnClickOutside ); - } - if ( listeners.onToggleOnClick ) { - button.removeEventListener( 'click', listeners.onToggleOnClick ); - } - if ( listeners.onToggleOnSpaceEnter ) { - button.removeEventListener( 'keydown', listeners.onToggleOnSpaceEnter ); - } - if ( listeners.onUpdateAriaExpandedOnInput ) { - checkbox.removeEventListener( 'input', listeners.onUpdateAriaExpandedOnInput ); - } + return function () { + cleanups.forEach( function ( cleanup ) { + cleanup(); + } ); + }; } module.exports = { @@ -371,6 +344,5 @@ module.exports = { bindToggleOnSpaceEnter: bindToggleOnSpaceEnter, bindDismissOnClickOutside: bindDismissOnClickOutside, bindDismissOnFocusLoss: bindDismissOnFocusLoss, - bind: bind, - unbind: unbind + bind: bind };