Page MenuHomePhabricator

OO.ui.mixin.GroupElement: Add and remove operations are not no-ops if there are no items given
Closed, ResolvedPublic

Description

... And they should be.

We trust all over our widgets that going "this.addItems( ... )" is a no-op if the given item is null, and it is a reordering process if the item already exists. However, as @kostajh found out, that's not true;

Example:

OO.ui.mixin.GroupElement.prototype.removeItems = function ( items ) {
	var i, len, item, index;

	// Remove specific items elements
	for ( i = 0, len = items.length; i < len; i++ ) {
		item = items[ i ];
		index = this.items.indexOf( item );
		if ( index !== -1 ) {
			item.setElementGroup( null );
			item.$element.detach();
		}
	}

	// Mixin method
	OO.EmitterList.prototype.removeItems.call( this, items );

	this.emit( 'change', this.getItems() );
	return this;
};

https://github.com/wikimedia/oojs-ui/blob/825593aee9/src/mixins/GroupElement.js#L180

The 'removeItems' process emits 'change' event even if there were no items at all. This produces problems for other widgets, like TagMultiselectWidget's "addTag" routine where if a tag isn't actually added in, the change event is still emitted, which causes a resizing process to happen and a reflow.

If the assumption is that GroupElement's methods are safe for no-op when no items are given, we should make sure that's actually true in the code.

Event Timeline

Change 442300 had a related patch set uploaded (by Kosta Harlan; owner: Kosta Harlan):
[oojs/ui@master] GroupElement: Make add/remove operations no-ops if no items given

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

Change 442892 had a related patch set uploaded (by Kosta Harlan; owner: Kosta Harlan):
[oojs/ui@master] GroupElement: Make add/remove operations no-ops if no items given

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

Change 442300 abandoned by Kosta Harlan:
GroupElement: Make add/remove operations no-ops if no items given

Reason:
follow up in https://gerrit.wikimedia.org/r/c/oojs/ui/ /442892

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

Change 442908 had a related patch set uploaded (by Kosta Harlan; owner: Kosta Harlan):
[oojs/core@master] Prevent adding/removing null value items

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

Krinkle updated the task description. (Show Details)
Krinkle updated the task description. (Show Details)

@Krinkle @Mooeypoo I'm a little unclear how to move these two patches forward (*EmitterList, GroupElement).

The motivation for the patches was this bit of now fixed code. We were calling removeDataByTag() but sometimes null was getting passed in as a parameter.

/**
 * Remove tag by its data property.
 *
 * @param {string|Object} data Tag data
 */
OO.ui.TagMultiselectWidget.prototype.removeTagByData = function ( data ) {
	var item = this.findItemFromData( data );

	this.removeItems( [ item ] );
};

and if you follow the chain a bit further it means that an unnecessary change event is emitted. The patches I submitted were to try to address this at a lower level but it seems like we don't have consensus on what exactly we'd want to do different from the existing code.

Looking at this again, perhaps we could address the bug we had with RC filters by adjusting removeTagByData(); if item === null then we can return early instead of calling this.removeItems().

Volker_E edited projects, added OOUI (OOUI-0.30.2); removed OOUI.

Change 442892 merged by jenkins-bot:
[oojs/ui@master] GroupElement: Make add/remove operations no-ops if items is empty

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

Volker_E triaged this task as Medium priority.
Volker_E removed a project: Patch-For-Review.
Volker_E removed a subscriber: gerritbot.

Change 486131 had a related patch set uploaded (by Jforrester; owner: VolkerE):
[mediawiki/core@master] Update OOUI to v0.30.2

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

Change 486131 merged by jenkins-bot:
[mediawiki/core@master] Update OOUI to v0.30.2

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

Change 442908 merged by jenkins-bot:
[oojs/core@master] EmitterList: Throw error on null/undefined item

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