Page MenuHomePhabricator

TagMultiselectWidget: the input field is editable even if the widget is disabled
Closed, ResolvedPublic


Steps to reproduce:
Go to (for tags) or (for throttle) for example (with an account that does not have the rights to modify abuse filters)
In the field with "Add tags..." or "Split with commas...", add random text
Leave the text field

Actual result:
The random text is displayed as one of the throttle groups or tags, and when reloading the page there is a warning that your changes may not be saved, despite that the user cannot actually save the changes

Expected result:
The fields are hidden when the user cannot actually use them

Event Timeline

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

Interesting find!

We are using a MenuTagMultiselectWidget here, but somehow its "disabled" view in AbuseFilter differs from what you see in OOUI demos. If I were to guess, I would say we are not using the right method/property to mark it as disabled.

Yep, I think so. Maybe there've been an update to the widget. Will check in a few days.

I see that AF is passing the correct value for disabled, that the above part of the widget is indeed greyed out, and the docs say that disabled is still an option for TagMultiselectWidget. But still, the input widget is writeable. Probably some bug/regression in OOUI itself.

Low priority - very unlikely to be encountered by accident, and does no harm for now

I'd leave it untriaged for OOUI people. This is not only impacting AF, but the entire OOUI library, which has a wider use. Sure, it's only a minor, visual glitch, but I can't if low is fine, without knowing what exactly is affected by this bug.

Daimona renamed this task from Editing filter: disable changing tags or throttle groups when user can't modify filters to TagMultiselectWidget: the input field is editable even if the widget is disabled.Aug 10 2019, 1:45 PM
DannyS712 raised the priority of this task from Low to Needs Triage.Aug 10 2019, 1:45 PM
Daimona added a subscriber: Daimona.
Volker_E added a subscriber: Mooeypoo.

I checked this again, and AFICS there are two factors contributing to this bug.

First, the logic for setting the input disabled (here) is:

this.input.setDisabled( !!isDisabled && !this.isUnderLimit() );

At first glance it seems that the logic operator is wrong, it should be a logic OR, not AND. Right now, the "disabled" status of the main widget is ignored when we're under the limit, which for the AF use case is always true.

Second, even after the fix above, the AF code would not work because it sets the "disabled" property when constructing the widget:

selector = new OO.ui.TagMultiselectWidget( {
	// ...
	disabled: config.disabled
} );

but it would work if I change it to

selector = new OO.ui.TagMultiselectWidget( {
	// ...
} );
selector.setDisabled( config.disabled );

In turn, this happens because the TagMultiselectWidget constructor has:

this.input.setDisabled( this.isDisabled() ); // <-- this line correctly disables the input
// ...
if ( config.selected ) {
	this.setValue( config.selected );

And the setValue call is causing the "disabled" status to go away. I haven't investigated any further, though.

Change 668493 had a related patch set uploaded (by Esanders; owner: Esanders):
[oojs/ui@master] TagMultiselectWidget: Fix typo in disable logic

Change 668493 merged by jenkins-bot:
[oojs/ui@master] TagMultiselectWidget: Fix typo in disable logic

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

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

Volker_E edited projects, added OOUI (OOUI-0.41.3); removed OOUI.
Volker_E assigned this task to Esanders.