Skip to content

fix: prevent <button>'s from becoming form submit when they shouldn't #4221

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jun 1, 2025

Conversation

dsevillamartin
Copy link
Member

Changes proposed in this pull request:
Adds type=button to most <button> usage (except the AdminPage#submitButton). The first button of type submit (which is the default in HTML if undeclared) becomes what the enter keybind presses in a form. This makes the behavior with these components make more sense.

I found this behavior while trying to make an extension page use a

, and enter kept opening a dropdown way down the page. Even though I could've rectified this by implementing onsubmit, the submit behavior was already declared in the button (using AdminPage#submitButton) so there was no reason to if I could stop the dropdown from becoming the form's submit button.

Reviewers should focus on:
I believe all the buttons that were missing a type declaration (except the one in AdminPage, which I added just for clarity) should all be regular buttons. They can still be clicked by pressing enter when focused, but they won't override the submit button of a <form> if they're the first submit button anymore.

Necessity

  • Has the problem that is being solved here been clearly explained?
  • If applicable, have various options for solving this problem been considered?
  • For core PRs, does this need to be in core, or could it be in an extension?
  • Are we willing to maintain this for years / potentially forever?

Confirmed

  • Frontend changes: tested on a local Flarum installation.
    • Did not test extensions, but just adding type="submit" and having the code compile should be fine.
  • Backend changes: tests are green (run composer test).
  • Core developer confirmed locally this works as intended.
  • Tests have been added, or are not appropriate here.

Required changes:

  • Related documentation PR: (Remove if irrelevant)

Adds `type=button` to most `<button>` usage (except the AdminPage#submitButton). The first button of type submit (which is the default in HTML if undeclared) becomes what the enter keybind presses in a form. This makes the behavior with these components make more sense.
@dsevillamartin dsevillamartin requested a review from a team as a code owner May 30, 2025 16:38
@SychO9 SychO9 added this to the 2.0 milestone Jun 1, 2025
@SychO9 SychO9 added the type/bug label Jun 1, 2025
Copy link
Member

@SychO9 SychO9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@SychO9 SychO9 merged commit 8e404e4 into 2.x Jun 1, 2025
28 checks passed
@SychO9 SychO9 deleted the ds/button-type-button branch June 1, 2025 07:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants