Page MenuHomePhabricator

Simplify the checkboxHack API
Open, 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.

Related Objects

StatusSubtypeAssignedTask
ResolvedGoalovasileva
OpenNone
Resolvedovasileva
ResolvedSpikeovasileva
ResolvedSpikephuedx
Resolvedovasileva
OpenSpikeVolker_E
OpenNone
Resolvedovasileva
OpenDemian
ResolvedSpikeovasileva
Resolvedovasileva
ResolvedBUG REPORTmatmarex
Resolvedovasileva
ResolvedJdlrobson
Resolvedphuedx
Resolvednray
ResolvedMayakp.wiki
ResolvedMayakp.wiki
Stalledovasileva
OpenNone
ResolvedEdtadros
OpenNone

Event Timeline

Jdrewniak created this task.Jul 3 2020, 8:21 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJul 3 2020, 8:21 PM
Jdrewniak updated the task description. (Show Details)Jul 3 2020, 8:37 PM
Demian added a comment.EditedJul 3 2020, 9:53 PM

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

Demian updated the task description. (Show Details)Jul 3 2020, 10:02 PM
Demian added a comment.EditedJul 3 2020, 10:12 PM

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?

Demian moved this task from Incoming to In progress on the User-Demian board.Jul 3 2020, 11:39 PM

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

Demian added a comment.Jul 6 2020, 2:34 AM

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

ovasileva triaged this task as High priority.Jul 6 2020, 5:25 PM

(To be estimated/discussed in Tuesday standup.)

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

Demian added a comment.Jul 6 2020, 8:56 PM

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

Demian added a comment.Jul 7 2020, 3:24 PM
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