Page MenuHomePhabricator

Simplify the checkboxHack API
Open, MediumPublic5 Estimated Story Points


As discussed at length in patch 606306 - checkboxHack: Trigger 'input' event when checkbox state changes the public API for the checkBox module could be improved by converting it from a model where we have multiple initialization functions to one where we have a single initialization function with an options object and a callback function to hook into the checkbox's change events.

As summarized by @Demian in T254851#6270677 :

In discussion with @Krinkle it came up that the interface of CheckboxHack is not simple enough and the responsibilities of the module and its users aren't clearly separated. The interface now looks like:

 module.exports = {
	updateAriaExpanded: updateAriaExpanded,
	bindUpdateAriaExpandedOnInput: bindUpdateAriaExpandedOnInput,
	bindToggleOnClick: bindToggleOnClick,
	bindDismissOnClickOutside: bindDismissOnClickOutside,
	bindDismissOnFocusLoss: bindDismissOnFocusLoss,
	bind: bind,
	unbind: unbind

It's the client's responsibility to properly combine these functions.
I've reduced this into:

 * Public API
module.exports = CheckboxHack;


checkboxHack = new CheckboxHack( window, checkbox, button, { onChange: saveSidebarState } );

Further named parameters can be provided in the options object to disable individual features, such as click and key handling.


No need to change Vector when the CheckboxHack's implementation is changed, eg. 608451.
Clean, progressively extensible API, separation of concerns.

Patch dependency graph

[ 608744 new API] ->
[ 606541 Vector patch] ->
[ 608756 "onChange() callback"] -> ... ->
[ 608755 "Cleanup"] ->
[ 606306 "Trigger 'input' event"] (optional for analytics)
This sequence enables SPACE, ENTER and a hook for saving the sidebar state for T255727.

The patches are minimal, focused on just one change to make review easier. If wanted, the patches can be squashed after being reviewed.
The code is augmented with strict type annotations, prepared for all jsduck, eslint and tsc. Briefly: type safe. The difficulty was TS and jsduck understanding a bit different dialect.

Event Timeline

@Jdrewniak Thank you for preparing this ticket, much appreciated!
I'll reattach the patches with the next PS.

EDIT: comment moved from T254851#6271404

Re: 606306

To give Krinkle and us a chance to decide which approach to take (his, mine or both) I've crunched out a rewrite of my original SPACE-ENTER patch, split into 6+2 patches.
Patch 7 (608756 "onChange[] callback") is Krinkle's callback approach and patch 8 (606306 Trigger 'input' event) is the event approach.
Note that the callback is not removed and that's the simpler and faster API, therefore the two can coexist and serve the use-case best suited to each. The analytics use-case mentined by Jdrewniak seems to be better suited for the event interface, while saving the sidebar state can be more tightly coupled with a callback.

Using the callback interface to save the sidebar state is demonstrated here:

This is noticeably simpler than the Event solution, therefore I've made the event dispatch logic in this patch the last in the chain, for use-cases like analytics.

I think it would be more beneficial if we review these incrementally, [...]

I've made the chain of 6+2 patches to be minimal, almost trivial steps that incrementally build up to the state change callback (patch 7) and then dispatching the event (patch 8). The more patches, the simpler each one. The changes are mostly renames and function signature changes.

There's significant effort in making these patches as simple as possible, I hope it will make the review easy.

It should be noted that the result functionality-wise is the same as before the patches were split. This can be confirmed from the patch in Vector which changed very little:

What has changed significantly through the 6 patches is the API between CheckboxHack and Vector and the internal implementation of CheckboxHack.
If there's any question about what's changed in any individual patch, feel free to ask and I'll do my best to explain the thinking behind each.

I've moved the old comment from T254851#6271404 and updated the description to reflect the new sequence I've uploaded this day, which moved [ 608756/10 ], the "onChange()" patch - necessary for saving the sidebar state - from the 6th position to the 2nd.

@Jdrewniak wrote on gerrit.

I see the Depends-On patch, but I think it'd be prudent to still keep the old API around to avoid blocking this patch on changes in Vector.

I've attempted to do that, but the new API requires passing the CheckboxHack instance. Unfortunately, the old API does not have the instance.
My thinking was the [ 606541 ] patch in Vector is trivial, received approval from Volker for the pre-refactor version and that patch changed very little, thus it seems to me a reasonable expectation that it will be merged right after the first core patch, which it depends on, as it happened with 608451.

Now you're raising doubts in me 😃 , so I've revisited this. It would need a hacky approach to map the old API functions to new API. Although this would be a short-lived, harmless hack, on ideological grounds I've dismissed this idea immediately and removed the old API, but I'm happy to implement any of the possible solutions if you so choose.
The problem is the class instance is necessary to map the old api functions to new api. A few options to acquire the instance:

  1. Add a property checkbox.checkboxHack. This is dependency inversion.
  2. Keep a singleton reference to the instance in core. There's only one user of the old API, so who needs more...
  3. Add a map: checkbox -> checkboxHack. Overengineering, not a hack.
  4. Merge the Vector patch soon after the 1st core patch, making the removal safe to be merged.

This will be short-lived and presumably removed before reaching prod (could be removed in a day), so which one would you like?

Change 608744 had a related patch set uploaded (by Aron Manning; owner: Aron Manning):
[mediawiki/core@master] checkboxHack: only one CheckboxHack() call to initialize

Change 608746 had a related patch set uploaded (by Aron Manning; owner: Aron Manning):
[mediawiki/core@master] checkboxHack: Simplify unbind()

Change 608755 had a related patch set uploaded (by Aron Manning; owner: Aron Manning):
[mediawiki/core@master] checkboxHack: Cleanup listener returning, function names

Change 606306 had a related patch set uploaded (by Aron Manning; owner: Aron Manning):
[mediawiki/core@master] checkboxHack: Trigger 'input' event when checkbox state changes

... or just doing everything in the first function call and ignoring the rest would do. New PS up: 608756/13.

Change 606541 had a related patch set uploaded (by Aron Manning; owner: Aron Manning):
[mediawiki/skins/Vector@master] [modern] checkboxHack: Use one-call simplified public API

Change 608754 had a related patch set uploaded (by Aron Manning; owner: Aron Manning):
[mediawiki/core@master] checkboxHack: Hiding the controlled element on outside events is simplified

The latest patch was accidentally merged into the next patch in chain. To make review simpler, I've re-split the two.

Change 610053 had a related patch set uploaded (by Aron Manning; owner: Aron Manning):
[mediawiki/core@master] checkboxHack: [squashed] Handle SPACE and ENTER key and simplify CheckboxHack() API

NOTE: the chain of 6 simple patches - split for ease of review in the manner of GitHub pull requests - is squashed into patch 608756 for merging. Less problematic in case of a revert.
Jdlrobson set the point value for this task to 5.Jul 7 2020, 5:15 PM
ovasileva lowered the priority of this task from High to Medium.Jul 8 2020, 6:56 PM
ovasileva moved this task from Incoming to Triaged but Future on the Readers-Web-Backlog board.

As the author of the patches and the task.

[Resetting assignee due to inactive user account]

Change 606541 abandoned by Jdlrobson:
[mediawiki/skins/Vector@master] [modern] checkboxHack: Use one-call simplified public API

271 days old wip patch

@Jdrewniak is this task still relevant to you? I'm wondering if it's worth working on given the changes planned in (T151115) in particular for the pinning functionality.