Page MenuHomePhabricator

Simplify the checkboxHack API
Open, Stalled, MediumPublic5 Estimated Story Points

Description

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;

Usage:

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.

Benefit

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:
https://gerrit.wikimedia.org/r/c/mediawiki/skins/Vector/+/607188/16/resources/skins.vector.js/skin.js#b6

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:
https://gerrit.wikimedia.org/r/c/mediawiki/skins/Vector/+/607188/13..14/resources/skins.vector.js/skin.js#65

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

https://gerrit.wikimedia.org/r/608744

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

https://gerrit.wikimedia.org/r/608746

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

https://gerrit.wikimedia.org/r/608755

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

https://gerrit.wikimedia.org/r/606306

... 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

https://gerrit.wikimedia.org/r/606541

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

https://gerrit.wikimedia.org/r/608754

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

https://gerrit.wikimedia.org/r/610053

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 Web-Team-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

Reason:
271 days old wip patch

https://gerrit.wikimedia.org/r/606541

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

Change 608754 abandoned by Ladsgroup:

[mediawiki/core@master] checkboxHack: Hiding the controlled element on outside events is simplified

Reason:

The author has been banned indef from our tech spaces and the patch has not been touched for really long time.

https://gerrit.wikimedia.org/r/608754

Change 608746 abandoned by Ladsgroup:

[mediawiki/core@master] checkboxHack: Simplify unbind()

Reason:

The author has been banned indef from our tech spaces and the patch has not been touched for really long time.

https://gerrit.wikimedia.org/r/608746

Change 608755 abandoned by Ladsgroup:

[mediawiki/core@master] checkboxHack: Cleanup listener returns, function names

Reason:

The author has been banned indef from our tech spaces and the patch has not been touched for really long time.

https://gerrit.wikimedia.org/r/608755

@bwang what does sequencing look like for this one? Is it something we need to work on earlier in the quarter or later?
@Jdrewniak @nray how does this relate to pinning functionality?

Change 610053 abandoned by Bartosz Dziewoński:

[mediawiki/core@master] checkboxHack: [squashed] Handle SPACE and ENTER key and simplify CheckboxHack() API

Reason:

The functional changes have already been done in T254851, the API changes would need to be rewritten for the current code.

https://gerrit.wikimedia.org/r/610053

Change 608744 abandoned by Bartosz Dziewoński:

[mediawiki/core@master] checkboxHack: Only one CheckboxHack() call to initialize

Reason:

https://gerrit.wikimedia.org/r/608744

Change 606306 abandoned by Bartosz Dziewoński:

[mediawiki/core@master] checkboxHack: Trigger 'input' event when checkbox state changes

Reason:

https://gerrit.wikimedia.org/r/606306

matmarex changed the task status from Open to Stalled.Sep 12 2023, 12:53 AM
matmarex subscribed.

It seems like the goals of this task have been accomplished, although in a very different way – you can use checkboxHack by just calling a single function .bind(), example in Vector: https://gerrit.wikimedia.org/g/mediawiki/skins/Vector/+/8ac1339771fdd818df0ee42d50a92d798744296b/resources/skins.vector.js/dropdownMenus.js#27

I'm not convinced that it would benefit from yet another rewrite. I suggest closing this task.

As I was working on these changes, more and more race conditions were emerging… You can see in the changes history how it went horribly.
(and actually, there is at least one other issue remaining, that I had fixed locally, but lacked the motivation to commit.)

I had noticed the new approach that has been implemented since. (I haven't reviewed it though)
As the old approach was an unmaintainable nest of race conditions, changing to a different paradigm was definitely the way to go.