Page MenuHomePhabricator

checkboxShift.js is incompatible with OOUI checkboxes
Closed, ResolvedPublic

Description

In the Special:EventDetails page defined by the CampaignEvents extension we have a list of CheckboxInputWidget elements generated with OOUI. Each checkbox has an event listener attached via OOUI's .on( 'change' ... method.

MediaWiki core loads checkboxShift.js, which is used to enable shift-selection of checkboxes. However, that is done by manually setting the "checked" property on the input, which does not work well with OOUI. The immediate consequence that I noticed is that the 'change' event won't be fired, so anything expecting the event to fire when the checkbox become selected will break.

I haven't verified, but I suspect that every page using CheckboxInputWidget is affected, because checkboxShift.js is loaded automatically, and you don't need to do anything special (e.g. weird config) to trigger this bug, AFAICS.

Event Timeline

More precisely, the problem on the OOUI side is that seem CheckboxInputWidget.setSelected is called by onEdit, but it does not fire any event because the if ( this.selected !== state ) { check fails (this.selected is not updated).

One more thing to note: checkboxShift can be disabled by adding the noshiftselect class to the checkboxes. Unfortunately, OOUI does not let you add CSS classes to the underlying <input> (AFAICS), so it's also impossible to work around this bug.

Would it suffice to have checkboxShift.js trigger the change event (not the OOUI event but the DOM event) on each checkbox it changes? It looks to me like it would, because that would cause onEdit to run and update this.selected. This could be as simple as adding .trigger( 'change' ); after .prop( 'checked', e.target.checked ) on line 28 of checkboxShift.js,

Some additional historical intel shared by @Catrope: @Krinkle wrote the checkboxShift.js file in October 2010, before OOUI was developed in 2012-13ish

Would it suffice to have checkboxShift.js trigger the change event (not the OOUI event but the DOM event) on each checkbox it changes? It looks to me like it would, because that would cause onEdit to run and update this.selected. This could be as simple as adding .trigger( 'change' ); after .prop( 'checked', e.target.checked ) on line 28 of checkboxShift.js,

Yes, SGTM and it worked in my quick test. I'm a bit perplexed because my comment above seemed to suggest that onEdit was already called but it would fail to trigger the event. However, onEdit is not called at all, so I'm not sure why I was thinking the opposite.

Should I write the patch, or were you going to (in which case I can review it)?

@Daimona please go ahead and write it if you have capacity (though @ifried confirmed this is not a hard requirement for V1); in chatting with @Catrope it sounds like he'll be able to support if needed. Thanks to you both!

Change 831158 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/core@master] Trigger the 'change' event in checkboxShift.js

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

@Daimona please go ahead and write it if you have capacity (though @ifried confirmed this is not a hard requirement for V1); in chatting with @Catrope it sounds like he'll be able to support if needed. Thanks to you both!

Thank you. It's a simple fix that addresses a potentially annoying bug, so I think it'd be great to have it fixed asap.

Change 831158 merged by jenkins-bot:

[mediawiki/core@master] mediawiki.page.ready: Emit the 'change' event in checkboxShift.js

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

@Daimona In testing here, this now functions correctly on the first 20 users, but shift click doesn't work to select multiple users after additional users are loaded in to the infinite scroll. See example here:

Screen Recording 2022-09-20 at 11.52.26 AM.gif (1×1 px, 2 MB)

@Daimona In testing here, this now functions correctly on the first 20 users, but shift click doesn't work to select multiple users after additional users are loaded in to the infinite scroll. See example here:

Ah yes, I knew that... The core code which makes the shift+click work only runs with the server-generated checkboxes, but not with the ones added dynamically. I guess there might be ways to fix it, but I'd leave that for another task.

@Daimona In testing here, this now functions correctly on the first 20 users, but shift click doesn't work to select multiple users after additional users are loaded in to the infinite scroll. See example here:

Ah yes, I knew that... The core code which makes the shift+click work only runs with the server-generated checkboxes, but not with the ones added dynamically. I guess there might be ways to fix it, but I'd leave that for another task.

Okay, got it. I am moving this to product sign off and I created T318261 to deal with the remaining small bug. I didn't yet tag OOUI or MediaWiki-interface on that ticket because I am not sure if this remaining bit is just on our end?

Okay, got it. I am moving this to product sign off and I created T318261 to deal with the remaining small bug. I didn't yet tag OOUI or MediaWiki-interface on that ticket because I am not sure if this remaining bit is just on our end?

Yep, that's just on our end.

@vaughnwalters I have conducted a basic test of shift select on beta, and it looks good. I'm marking this as Done, and we can handle T318261 separately.