Page MenuHomePhabricator

MenuTagMultiselectWidget: `allowEditTags` doesn't work
Open, MediumPublic

Description

Looking at the docs, MenuTagMultiselect inherits the allowEditTags attribute from TagMultiselect. However, while it perfectly works for the latter, it doesn't for the former. I discovered this while writing https://gerrit.wikimedia.org/r/#/c/421487/, but it also doesn't work on the snippet in the doc page (just add allowEditTags: true).

Event Timeline

Volker_E added subscribers: Mooeypoo, Volker_E.

@Mooeypoo Would you look into this?

I found the culprit: https://gerrit.wikimedia.org/g/mediawiki/core/+/master/resources/lib/oojs-ui/oojs-ui-widgets.js#5859. The comment is right, usually you don't want to allow users edit tags on the fly. However, it should instead be allowed if allowArbitrary is set to true. So basically overriding the listener iff allowArbitrary is false should do the trick, right?

Change 427611 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/core@master] OOUI: Make MenuTagMultiselect use default onTagSelect if allowArbitrary

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

Change 427871 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[oojs/ui@master] Make MenuTagMultiselect use default onTagSelect if allowArbitrary

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

After a bit of discussion on the ticket, I've merged the code. It's a good fix for the current completely broken behavior.

However, we need to discuss the general behavior of a Menu*Widget with "allowArbitrary" in general. I don't know if it makes sense the way it works at the moment at all, and while this commit at least fixes a broken piece, the overall UX should be thought through.

Let's try and hash out the issues in this (or another?) ticket and improve for followups.

@Daimona can you specify briefly the behavior you described in the gerrit discussion, so @Volker_E can pitch in about maybe assessing a good general UX for the widget in these cases?

When @Volker_E and I talked about it a little while ago, we were thinking of actually cancelling "allowArbitrary" for MenuTagMultiselectWidget, since if you have a menu, it makes no sense to have items outside it. I had an example where that might not be true (RCFilters) but that system is already quite a big edge case.

But it sounds like the behavior you wanted to target is not an edge case, and may shed light on how to better represent that state in the widget. Can you lay out the general specifications your example requires?

Change 427871 merged by jenkins-bot:
[oojs/ui@master] Make MenuTagMultiselect use default onTagSelect if allowArbitrary

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

Sure! We're planning to use this widget on AbuseFilter to show a list of already used tags, and of course the user is allowed to create new ones. So we need the list and the possibility to insert arbitrary values. In this case, it would probably be good to always keep parent's behaviour if allowArbitrary is true: we don't have a clear distinctiom between listed and arbitrary values, and by allowing users to modify the former we may make things easier. I gave an example on gerrit, and will give another one here: suppose that one of the listed items has a typo: it's be quicker to directly edit it, instead of a full rewrite. This way the original item would remain untouched, and the fixed one would be created fresh.

Change 427611 abandoned by Bartosz Dziewoński:
OOUI: Make MenuTagMultiselect use default onTagSelect if allowArbitrary

Reason:
Changes done in the oojs/ui repository

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

@Mooeypoo I'd prefer to have follow-up discussion in a follow-up task. Would you file what you think is necessary…?

Volker_E renamed this task from allowEditTags doesn't work MenuTagMultiselect to MenuTagMultiselect: `allowEditTags` doesn't work .Jun 13 2018, 2:47 PM
Volker_E renamed this task from MenuTagMultiselect: `allowEditTags` doesn't work to MenuTagMultiselectWidget: `allowEditTags` doesn't work .
Volker_E triaged this task as Medium priority.
Volker_E removed a project: Patch-For-Review.
Volker_E removed a subscriber: gerritbot.