Page MenuHomePhabricator

TagMultiselectWidget clearItems() doesn't work properly when the widget is disabled
Closed, ResolvedPublic


When the widget is disabled, and clearItems() is called when the widget has more than one tag, it leaves phantom tags in the widget.

To reproduce:

  1. Open the demos page and find the first TagMultiselectWidget. Open its command line.
  2. Enter widget.setValue( [ 'a', 'b' ] );
    • The widget will be showing tags 'a' and 'b'.
  3. Enter widget.setDisabled( true );
    • The widget will become disabled, and oddly will change the order of the tags.
  4. Enter widget.clearItems();.
    • The widget will remove the 'b' tag, but 'a' will be left behind.
  5. Enter widget.getValue();
    • The console will report that the value was "", despite the widget displaying 'a'.
  6. Enter widget.setDisabled( false );
    • The widget will be enabled, but the phantom 'a' tag will remain disabled.
  7. Enter widget.setValue( [ 'c', 'd' ] );
    • The widget will display normal tags 'c' and 'd' after the phantom 'a' tag.

This seems to have been caused by rGOJUaca140acaf80: TagMultiselectWidget: Handle disabled items.

When the TagMultiselectWidget widget is disabled, it disables all its tags. And when a tag fires its 'disabled' event, TagMultiselectWidget moves it to the start of its list. That explains the weird order changing in step 3.

In TagMultiselectWidget's clearItems() method, it loops over all the items to set the item's group to null and then detach it from the DOM.

	for ( i = 0, len = this.items.length; i < len; i++ ) {
		this.items[ i ].setElementGroup( null );
		this.items[ i ].$element.detach();

But the group setting triggers a call to updateDisabled(), which fires 'disabled', which causes TagMultiselectWidget to move the item to the start, so this.items[ i ] no longer refers to the same item for the detach() call.

IMO the best fix would be to change the "move any disabled item to the start" logic so it doesn't weirdly reorder all the tags when the widget as a whole is disabled. You could probably accomplish this by moving the newly-disabled item to just before the first still-enabled item.

Event Timeline

Yowza, nice catch, thank you!

I'll try to work on a workaround by having teh widget check if the entire widget ise disabled vs. having the widget enabled but specific items disabled, this will at least make some of the logic a lot better.

That said, @Volker_E this is bringing back up the idea of whether we even WANT to have specific items that are disabled rather than the entire widget disabled.

Change 430280 had a related patch set uploaded (by Mooeypoo; owner: Mooeypoo):
[oojs/ui@master] [BREAKING CHANGE] TagItemWidget: Replace 'disabled' items with 'static'

Mooeypoo moved this task from Backlog to Doing on the OOUI board.
Mooeypoo added subscribers: Lea_WMDE, matmarex.

I'm replacing the concept of setDisabled() on an item with a setStatic() on an item; this will (a) prevent the bad behavior when the entire widget is disabled, and (b) properly separate the actual meaning of wanting to specifically "disable" a single item inside the widget -- which, really, means, it's not disabled, it's just "always included" and is static.
This also means that T181578: Backspace allows editing/deleting disabled items in TagMultiSelectWidget will be changed.

This is a breaking change;

There's a depracation notice, but the old behavior of setting individual items to disabled will no longer work (with a depracation note)

The main things with this code change:

  • From now, to get the behavior in the original ticket, the item should be marked "static: true" on creation (or set by widget.getItems()[1].setStatic( true ); in the demos.
  • The 'disabled' item event was removed; there's no purpose to get each item to emit that state when from now on 'disabled' is a widget-based (and not sub-item-based) action. If anything relied on this event (it shouldn't have, hopefully) it would stop working.
  • setDisabled called directly on a TagItemWidget is marked as deprecated, will not behave as previously expected, and will throw a deprecation warning.

@Volker_E I think this solves your concern, as well. I am using for the moment the same design as a disabled state (I'm also disabling the item so it cannot be clicked or removed) but this also opens the possibility for you to adjust the design for "static" items like you wanted to initially, since there's an additional '-static' class on them now.

@matmarex and @Lea_WMDE since you were both involved with T181578: Backspace allows editing/deleting disabled items in TagMultiSelectWidget please pitch in.

@Mooeypoo we removed the disabled state, so this should not cause any problems for us, but thanks for the heads up! And to be super safe, fyi WMDE-FUN-Team (@Tonina_Zhelyazkova_WMDE , @Tim_WMDE , @gabriel-wmde, @aniansson )

Change 430280 merged by jenkins-bot:
[oojs/ui@master] [BREAKING CHANGE] TagItemWidget: Replace 'disabled' items with 'fixed'

Vvjjkkii renamed this task from TagMultiselectWidget clearItems() doesn't work properly when the widget is disabled to cudaaaaaaa.Jul 1 2018, 1:12 AM
Vvjjkkii removed Mooeypoo as the assignee of this task.
Vvjjkkii triaged this task as High priority.
Vvjjkkii updated the task description. (Show Details)
Vvjjkkii removed subscribers: gerritbot, Aklapper.
CommunityTechBot renamed this task from cudaaaaaaa to TagMultiselectWidget clearItems() doesn't work properly when the widget is disabled.Jul 1 2018, 9:52 AM
CommunityTechBot assigned this task to Mooeypoo.
CommunityTechBot updated the task description. (Show Details)
CommunityTechBot added subscribers: gerritbot, Aklapper.
CommunityTechBot raised the priority of this task from High to Needs Triage.Jul 3 2018, 1:56 AM
Volker_E triaged this task as High priority.
Volker_E moved this task from Reviewing to OOUI-0.27.0 on the OOUI board.
Volker_E edited projects, added OOUI (OOUI-0.27.0); removed OOUI.
Volker_E removed a subscriber: gerritbot.