Page MenuHomePhabricator

Pressing enter on site list field removes the current field
Closed, ResolvedPublicDesign

Description

Steps to reproduce:

  1. Type something into a field of Special:GlobalWatchlistSettings
  2. Hit enter

Expected:

  • Either nothing happens or the "add" button is "pressed"

Actual:

  • The remove button of the current URL field is "pressed"

Browser in case it matters is Safari 14.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript
DannyS712 changed the subtype of this task from "Task" to "Design".
DannyS712 moved this task from Unsorted to Next on the User-DannyS712 board.
DannyS712 subscribed.

Can reproduce with Chrome - I guess enter = "press the button next to this input" which is the remove button

Correction—it actually removes the first input (i.e., the topmost one), which is strangely mildly amusing.

So started looking into this, and found that if you click on any of the other inputs (eg one of the radio options or checkboxes) and then hit enter, it *still* removes the first input...
Using chrome's developer tools I removed all of the event listeners I could find for the form, and yet enter still removed the inputs.
Then I ran $('*').off() which removes all event listeners for everything on the page, and that didn't remove the issue, though it did change it - instead of removing the entry in place, it refreshed with the entry gone (which is what would happen if you didn't have javascript and clicked the remove button).
So I'm thinking this isn't from OOUI, but rather because hitting enter triggers the first button on the page?

A bunch of google searching later, I found https://stackoverflow.com/questions/4763638/enter-triggers-button-click and https://html.spec.whatwg.org/#implicit-submission - indeed, if you're focused within the form, hitting enter will try to submit the form, and it thinks that hitting the first button of the "submit" type will do that. But, the "Remove" buttons are "submit" type, because that is required for the HTMLFormFieldCloner[1] (which is how we manage the multiple rows)

Some more investigation into the HTML spec, plus the above stackoverflow page, suggested that the easiest way to deal with this is to just add a different button earlier, and hitting enter would trigger that instead. But, if we trigger that button we submit the form, and we don't want enter to do that either. Solution: disable the button. Plus, since its just there to intercept the enter, it doesn't need to be shown. Properly adding this button in the php is really tricky though.

We can't use HTMLHiddenField, that is for hidden input fields and we want a button
Trying to use a manual HTMLSubmitField would work, but to be put at the top it would need to belong to a section, either its own or the existing top section (sitelist). If its in its own section, then there is a new section label, which is bad, so put it in the sitelist section. Then we need to figure out how to hide it. We could use the hide-if functionality and make something that will always be true, but then if someone doesn't have javascript it won't be hidden.[2]
We could hide it manually with a css class, but the existing ResourceLoader module with the styles for that page is loaded with addModules() instead of addaddModuleStyles() so that would have the same issue. Except, I don't think it really needs to be done with addModules() - thats used because it has a dependency on a javascript module (mediawiki.htmlform.ooui) but we can just load that manually.

[1] https://gerrit.wikimedia.org/g/mediawiki/core/+/8941540dc71cacf37c1196b6480d51067dc28daf/includes/htmlform/fields/HTMLFormFieldCloner.php#71

Change 698544 had a related patch set uploaded (by DannyS712; author: DannyS712):

[mediawiki/extensions/GlobalWatchlist@master] Special:GlobalWatchlistSettings - 'enter' should not remove site rows

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

Okay, as explained on the patch, I was only able to fix this for hitting enter while you're in one of the site rows, elsewhere the old behavior still applies (remove the top site row, or add a new row if there is no existing one) - the patch is better than nothing, since enter is most likely to be hit while you're typing, but I'll try again to figure it out at some point.

Change 698544 merged by jenkins-bot:

[mediawiki/extensions/GlobalWatchlist@master] Special:GlobalWatchlistSettings - 'enter' should not remove site rows

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

Okay, as I documented on the patch, this doesn't fix everything, just if you hit enter from within the site input fields, and is does this by adding a dummy button that is triggered instead. We should QA that

  • the button is never showed[1]
  • the fix works for hitting enter in the site input fields

and I'll file a task to address hitting enter in the rest of the form separately

[1] Its current hidden via a ResourceLoader module (styles only) so it should be hidden as long as the browser loads that, but technically I think we are meant to support viewing without JavaScript enabled, which I think also prevents style modules from loading, so I'll need to check that

It turns out since we switched to OutputPage::addModuleStyles this works even if javascript is disabled, https://www.mediawiki.org/wiki/ResourceLoader/Architecture#Resource:_Styles

DannyS712 claimed this task.
DannyS712 moved this task from In progress to Done on the MediaWiki-extensions-GlobalWatchlist board.
DannyS712 removed a project: TestMe.

Filed T286146: Figure out weird implicit form submission (follow-up T275588) to fix the rest, going to say this is done since the biggest issue (hitting enter from within a text input) was fixed

I think it was a bug of HTMLFormFieldCloner itself rather than GlobalWatchlist. Is there a ticket for HTMLFormFieldCloner?