Thijs/wiki.techinc.nl
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

[BREAKING] Change cleanup API of checkboxHack

Browse source 
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
This commit is contained in:
Nicholas Ray 2021-10-22 10:49:56 -06:00 committed by Jdlrobson
parent b2d441658f
commit d99dd4040e
2 changed files with 45 additions and 69 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

4
RELEASE-NOTES-1.38
View file

@ -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 ===

110
resources/src/mediawiki.page.ready/checkboxHack.js
View file

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