Page MenuHomePhabricator

Removing admins in SecurePoll sometimes has unintended effects
Closed, ResolvedPublic

Description

I removed myself as an admin here but the only admin I removed was myself; yet somehow two other users were also removed.

I cannot replicate this, but it has happened twice for me. There is a bug somewhere here.

UPDATE PER MERGED TASKS:

It seems like pressing Enter within the form is causing this problem. The admin that is accidentally removed is always the "first" one in the Admins section. Submitting the form using the Enter key also causes this.

Event Timeline

This happened again here when I attempted to add Huji back. No other fields were altered.

Still happening, though as Huji indicates in T150714: SecurePoll occassionally drops last custom option label this probably needs server logs to be looked at.

Huji added subscribers: Anomie, Tgr.

Fun fact: if you press enter while you are in the "Question text" field, it will make the first of the Admins fields disappear. If you keep pressing Enter, the rows in the Admin section disappear one by one. And if you press enter again after all this, it will make a new empty input box appear in the Admins section!

Technically, these are all "cloner" boxes (as in HTMLFormFieldCloner.php) so I am guessing the fact that we have more than on cloner objects on the same page is somehow causing a side effect. Adding MediaWiki-HTMLForm tag for that reason.

Detailed analysis

When you press enter in the Question textarea (or any other textarea, indeed including the textarea for one of the Admins fields), the browser doesn't know what to do with it (because the form is not supposed to submit when pressing enter) so it passes that keypress event to the first button on the form, which happens to always be the "Delete" button next to the very first textarea in Admins. Hence the deletion of the first row. This repeats until you are out of rows in Admins (and therefore, out of "Delete" buttons). At this point, the first button on the page will become the "Add more" button of the admins section, so that's the one that handles the enter key.

And so my solution to this problem is that the pressing of "Enter" in cloner text areas should be handled without propagation (that is, it should not be passed up to the higher level form element).

To verify this, I injected the following JS to the entire page:

$(document).keypress(function(e) {
    if(e.which == 13) {
        return false; // DO NOT PROPAGATE
    }
});

This fixed the problem.

Now, this is a hacky fix, and we need to convert this to a proper fix. At this point, I need someone more experienced to propose ideas on how to make it a real solution. @Tgr and @Anomie are my go to people in a situation like this.

Huji updated the task description. (Show Details)

Is equivalent to submitting the form by pressing the default button, which is the first button in document tree order. I'm not familiar with the extension / form, but if this results in unexpected behavior, the buttons should probably be rearranged.

Clarakosi subscribed.

Untagging because we were tagged by Herald

I think this and T150714 were likely fixed with the recent changes to SecurePoll by AHT. I certainly haven't experienced either bug since then.

I think this and T150714 were likely fixed with the recent changes to SecurePoll by AHT. I certainly haven't experienced either bug since then.

This had the same root cause as T275588 only with a different form, (implicit submission in cloned fields) - could the SecurePoll fix be applied in core so that it works for all uses?

jrbs claimed this task.

I believe this was indeed fixed with the move to OOUI.

This had the same root cause as T275588 only with a different form, (implicit submission in cloned fields) - could the SecurePoll fix be applied in core so that it works for all uses?

I think this request would need an additional task to split it out from this one.