Stop poll admins from being deleted when the enter keypress is registered on another input.
Based on T269030#6664430, it seems like there's an odd binding out that causes the event to be emitted from the first input possible (usually the admin delete button, hence the symptom).
| Subject | Author | Repo | Branch | Lines +/- | |
|---|---|---|---|---|---|
| Stop cloner add/delete functions from triggering unexpectedly | STran | mediawiki/extensions/SecurePoll | master | +16 -1 |
Leaving some notes here while I continue to attempt to wrangle this:
I think part of the problem is that you can even make something happen by pressing "enter". For instance, if I'm editing any input and I press enter, I do not expect the response from the form to delete my entry. However, that's what happens now because (???) the closest submit button is the delete button and it looks like that's what's triggering that event. I say looks like because a submit button is supposed to submit the entire form (so were we not doing stuff on top of the html, this would, as far as I understand semantic html, not very correct. Therefore, it's also incorrect that the delete button even has the submit attribute, which may be adding to our woes, now that I consider it.
You know what, that sounds a lot like the problem - when you hit enter on an input, you normally end up submitting the form. I'll bet it's taking the first submit it can find (in our case, the admin add/delete) and triggering a click event.
Here is a JSFiddle as a proof of concept: https://jsfiddle.net/jm6w40o8/6/
Hit "enter" on any input and watch it alert that the first button (ie. the submit) has been clicked (it emits the class name as proof) 🙃
I would really like to solve this by changing all create/delete buttons into actual buttons which according to MDN:
button: The button has no default behavior, and does nothing when pressed by default. It can have client-side scripts listen to the element's events, which are triggered when the events occur.
Since the adding/deleting is already JS-dependent I don't think we're causing any accessibility regressions. Also the button as an HTMLForm PHP class already exists (but isn't added as a valid type parameter?).
However, there are a bunch of comments in HTMLFormFieldCloner.php that suggest that the submit button is intentional. This was also apparently 7 years ago and comes with this comment:
* Use of cloner may result in submissions of the page that are not submissions * of the HTMLForm, when non-JavaScript clients use the create or remove buttons.
I'd like to propose we go ahead and make it js-only and non-js users shouldn't expect that their forms submit mid-way. If we want to make it non-js friendly, we should still not consider using submits in this non-semantic way.
The good (?) news is that I don't believe we don't have to worry about this being used elsewhere since the cloner was developed for SecurePoll and is not marketed for general use (see https://gerrit.wikimedia.org/r/c/mediawiki/core/+/118113).
Change 650017 had a related patch set uploaded (by STran; owner: STran):
[mediawiki/extensions/SecurePoll@master] [WIP] Stop cloner add/delete functions from triggering unexpectedly
Some more notes - I tested this with no js and the use of the submit button makes it so that non-js users can still add/delete cloner elements (by passing along a value with that submit input). However, if you press enter on an input (generic submit) the same bug applies, only slower because you have to wait for the form to submit and return from the backend. Since so much work was done to make this functional for non-js users, I'm hesitant to introduce a "fix" that will actually cause a regression.
Therefore, I have a hack - @Tchanders could I get you to weigh in on https://gerrit.wikimedia.org/r/c/mediawiki/extensions/SecurePoll/+/650017 and if you think we should continue with this or if we should still consider using buttons over submit buttons?
@STran Thanks for investigating this so thoroughly and for the detailed explanation.
Since so much work was done to make this functional for non-js users, I'm hesitant to introduce a "fix" that will actually cause a regression.
I agree that its worth keeping this while we can.
Having had some time to mull this over, I think eventually we should use the HTMLUsersMultiselectField (T270634) to enter multiple poll admins, which didn't exist when this form was built. I think we should do this later though, since there's some additional work to do:
I think the current solution is a nice quick fix that we can do now.
Change 650017 merged by jenkins-bot:
[mediawiki/extensions/SecurePoll@master] Stop cloner add/delete functions from triggering unexpectedly