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.

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

Properly resolve referenced forms. #3094

Merged
merged 9 commits into from
Jan 9, 2025

Conversation

geoffrey-eisenbarth
Copy link
Contributor

Description

I would like to start off by thanking @scrhartley for their help with this PR.

When a non-GET htmx request is triggered on an element inside a form, htmx includes the other values in the form. However, it is currently including incorrect values when e.g. a <button form="referenced"> is inside a different form. This PR addresses that bug. Details are explained in the documentation change and the added test.

Corresponding issue: None

Testing

Using standard HTML behavior as a reference, consider the following:

<form id="externalForm" action="/test" method="post">
    <input type="text" name="t1" value="textValue">
    <input type="hidden" name="b1" value="inputValue">
</form>
<form action="/test2" method="post">
    <input type="text" name="t1" value="checkValue">
    <button id="submit" form="externalForm" formaction="/test3" formmethod="post" type="submit" name="b1" value="buttonValue">button</button>
</form>

clicking #submit will result in a request whose body contains t1=textValue&b1=inputValue&b1=buttonValue, which agrees with the new tests.

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

@@ -3523,7 +3523,7 @@ var htmx = (function() {

// for a non-GET include the closest form
if (verb !== 'get') {
processInputValue(processed, priorityFormData, errors, closest(elt, 'form'), validate)
Copy link
Contributor

Choose a reason for hiding this comment

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

would you mind extracting this to a local variable to avoid the complicated expression in the parameter position, w/ maybe a comment explaining the situation?

thank you, PR looks great love the tests!

Copy link
Contributor Author

@geoffrey-eisenbarth geoffrey-eisenbarth Dec 28, 2024

Choose a reason for hiding this comment

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

I addressed your request, but also there's been some discussion about a better way forward on #issues-and-pull-requests that I'm happy to implement (I am ironbeard on discord).

One thing that might need clarifying is the original intent of the sentence in the docs that this PR changes. Specifically, what kind of elements inside a form should also be sending form values on non-GET?

  • <select hx-post=/foo>?
  • <input type=text hx-post=/foo>?
  • <input type=submit hx-post=/foo>?
  • <div hx-post=/foo>?

Furthermore, personally I'm wondering why this is being limited to non-GET? Wouldn't we want to submit the entire form for e.g.

<form method=get action=/foo>
  <input name=bar value=baz>
  <button hx-get=/foo>Submit</button>
</form>

I'm actually not 100% on whether HTMX currently submits bar=baz in this situation or not (I haven't been able to find an associated test and I'm on mobile now), but the docs and the code appear to indicate that it won't. I will make sure to cover any of those situations with tests once I understand the specifics of that line in the docs.

In case it's helpful, here's the original PR of that line in the docs: a3c9cf6#diff-c7d06af83d22e155470d44a366cc1ba76cd5dc9da3405fc9e54a237174736b6aR962

Copy link
Contributor

Choose a reason for hiding this comment

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

htmx has not submitted values for GETs that occur within a form so that link-like things do not include them when navigating. This behavior may not be ideal but we can't change it without breaking backwards compatibility. (In general at this point backwards compatibility is becoming more important than correctness in htmx.)

I think the situation where the user has explicitly called out a form via the form attribute is obvious enough to fix so I'm in favor of this change so long as it minimizes any additional changes, in particular with an eye towards backwards compatibility.

@geoffrey-eisenbarth
Copy link
Contributor Author

geoffrey-eisenbarth commented Jan 2, 2025

I refactored a bit based on some much appreciated input from @dz4k and @MichaelWest22 , would appreciate their eyes on this result if possible.

@geoffrey-eisenbarth
Copy link
Contributor Author

@MichaelWest22 suggested on discord that we use ts-ignore instead of introducing new types.

@geoffrey-eisenbarth
Copy link
Contributor Author

There's some ongoing discussion here:

https://discord.com/channels/725789699527933952/1324452338429067316

@geoffrey-eisenbarth
Copy link
Contributor Author

This PR will slightly change existing behavior. Previously, something like <button type=reset hx-post=/endpoint> would not reset the form bc of a loose if clause in shouldCancel. I have added test to support imo the desired behavior, which will still reset the form but also submit current form values to /endpoint.

@scrhartley
Copy link
Contributor

This PR will slightly change existing behavior. Previously, something like <button type=reset hx-post=/endpoint> would not reset the form bc of a loose if clause in shouldCancel. I have added test to support imo the desired behavior, which will still reset the form but also submit current form values to /endpoint.

I suspect that reset doesn't get used much and this a bugfix as much as anything.

@Telroshan Telroshan added the bug Something isn't working label Jan 6, 2025
@1cg 1cg merged commit f46989b into bigskysoftware:dev Jan 9, 2025
1 check passed
geoffrey-eisenbarth added a commit to geoffrey-eisenbarth/htmx that referenced this pull request Jan 9, 2025
* Properly resolve referenced forms.

* Clarify variable.

* Cast elt to avoid TS exceptions.

* Refactor for JSDoc.

* Clarify shouldCancel.

* Remove complicated JSDoc in favor of ts-ignore.

* More coverage for button scenarios.

* Use properties instead of matching.

* Mention reset button change.
1cg pushed a commit that referenced this pull request Jan 9, 2025
* Enforce no-op on submit buttons with formmethod=dialog.

* Properly resolve referenced forms. (#3094)

* Properly resolve referenced forms.

* Clarify variable.

* Cast elt to avoid TS exceptions.

* Refactor for JSDoc.

* Clarify shouldCancel.

* Remove complicated JSDoc in favor of ts-ignore.

* More coverage for button scenarios.

* Use properties instead of matching.

* Mention reset button change.

* Mention formmethod=dialog change.
@geoffrey-eisenbarth geoffrey-eisenbarth deleted the button-w-outside-form branch January 10, 2025 01:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants