Page MenuHomePhabricator

OO.ui.SelectWidget.removeItems deselects all when removing selected items
Closed, ResolvedPublicBUG REPORT

Description

Steps to replicate the issue (include links if applicable):

  • Create a OO.ui.SelectWidget with multiple options.
  • Select several options, including one to be removed, and some that will not be removed.
  • Call removeItems([optionToRemove]).

What happens?:

All selected options are deselected including ones that were not removed.

I believe the issue is the for loop at https://doc.wikimedia.org/oojs-ui/master/js/widgets_SelectWidget.js.html#line1067

OO.ui.SelectWidget.prototype.removeItems = function ( items ) {
	// Deselect items being removed
	for ( let i = 0; i < items.length; i++ ) {
		const item = items[ i ];
		if ( item.isSelected() ) {
			this.selectItem( null );
		}
	}

The issue is that removeItems calls this.selectItem(null) for any selected item being removed. Passing null deselects all options in the widget, not just the one being removed, which contradicts the comment and causes unexpected deselection of unrelated selected options. The argument should instead be the specific item being removed.

Making the issue worse is that it's inconsistent. If none of the removed options are selected, everything works as expected, but if any one option of the removed set is selected, then everything is deselected including non-removed options.

What should have happened instead?:

Only the removed items should be deselected. Other selections should be unaffected.

Software version (on Special:Version page; skip for WMF-hosted wikis like Wikipedia):

current English Wikipedia

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript

Other information (browser name/version, screenshots, etc.):

Here is a quick demonstration of the issue. Once I select an additional option, the selection of the ignoreCase option is lost when the additional option is removed.

That code was probably written before SelectWidget gained the multiselect option, and needs to be changed to support it.

Change #1191531 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[oojs/ui@master] SelectWidget: Fix removeItems() interaction with 'multiselect'

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

Change #1191531 merged by jenkins-bot:

[oojs/ui@master] SelectWidget: Fix removeItems() interaction with 'multiselect'

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

Change #1219646 had a related patch set uploaded (by VolkerE; author: VolkerE):

[mediawiki/core@master] Update OOUI to v0.53.1

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

Change #1219646 merged by jenkins-bot:

[mediawiki/core@master] Update OOUI to v0.53.1

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