Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Do not boost forms with method="dialog" #2752

Merged
merged 2 commits into from
Aug 5, 2024
Merged

Do not boost forms with method="dialog" #2752

merged 2 commits into from
Aug 5, 2024

Conversation

pokonski
Copy link
Contributor

@pokonski pokonski commented Jul 22, 2024 •
edited
Loading

Description

Boosting forms with method="dialog" should not trigger AJAX requests because these are only meant to close parent <dialog> elements and not submit anything.

When a <form> within a <dialog> is submitted via the dialog method, the dialog box closes, the states of the form controls are saved but not submitted

source: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/dialog#usage_notes

Testing

Can be tested with the following snippet:

<body hx-boost="true">
  <dialog id="modal">
    <form method="dialog">
      <button>Close modal</button>
    </form>
  </dialog>
</body>

<script type="text/javascript">
   document.getElementById("modal").showModal()
</script>

Clicking the "Close modal" button should not trigger any AJAX requests

Checklist

  • I have read the contribution guidelines
  • I have targeted this PR against the correct branch (master for website changes, dev for
    source changes)
  • This is either a bugfix, a documentation update, or a new feature that has been explicitly
    approved via an issue
  • I ran the test suite locally (npm run test) and verified that it succeeded

@Telroshan
Copy link
Collaborator

Hey, while I perfectly understand the concern of following the specs, the issue is always about retro-compatibility ; as you've noticed yourself on PR #1867 and related issue #1866, since it's been merged, at least the OP and likely other persons have been running this setup, which would suddenly not work anymore as they would expect it to make the request with the fallback method specified.

That's exactly the kind of stuff that led us to make htmx 2 ; making breaking changes, changing some defaults to either fit the specs, or set new defaults that felt more logical (security ones for ex such as selfRequestsOnly).

And now that htmx 2 is officially released, it would take an htmx 3, which I'll be honest about, is very, very unlikely to happen anytime soon (if at all).
So, for this kind of change, I'm afraid you'd have to go for an opt-in approach, i.e. leave the default behavior as is and provide a configuration option to make dialog skip submit altogether.

I know it can be frustrating, I've been there, I hope you'll understand!

@geoffrey-eisenbarth geoffrey-eisenbarth mentioned this pull request Jul 22, 2024
Open
4 tasks
@pokonski
Copy link
Contributor Author

pokonski commented Jul 22, 2024 •
edited
Loading

Hey @Telroshan ! this fix won't break #1867 since that one uses explicit hx-verb attribute. Hx-boosting is a separate logic from my brief experience with the issue and I haven't seen any attempts to fix yet.

In any case, I can add a config value for this, probably also needed for #2753?

@Telroshan Telroshan added the bug Something isn't working label Jul 22, 2024
@Telroshan
Copy link
Collaborator

Oh ok! I misunderstood indeed, doesn't cover the same situation.

Then, if we didn't have any test for it, I'd consider this as an undocumented behavior, thus sounds fine to directly fix to me, as it's no more a breaking change as it would have been otherwise.
Take that with a grain of salt though as I'm not the decision-maker here! Wouldn't want to give false hope either

@Telroshan Telroshan added the ready for review Issues that are ready to be considered for merging label Jul 22, 2024
@pokonski
Copy link
Contributor Author

I misunderstood indeed, doesn't cover the same situation.

No worries, I confused myself while doing this fix 😁

@1cg 1cg merged commit ee9b0e0 into bigskysoftware:dev Aug 5, 2024
1 check passed
@pokonski pokonski deleted the fix-dialog-boost branch August 5, 2024 19:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
bug Something isn't working ready for review Issues that are ready to be considered for merging
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pokonski @Telroshan @1cg