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

Properly resolve referenced forms. #3094

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from
Open

Properly resolve referenced forms. #3094

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
24dd1e1
Properly resolve referenced forms.
geoffrey-eisenbarth Dec 28, 2024
c5cce73
Clarify variable.
geoffrey-eisenbarth Dec 28, 2024
3c5a376
Cast elt to avoid TS exceptions.
geoffrey-eisenbarth Dec 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions src/htmx.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2401,7 +2401,7 @@ var htmx = (function() {
return true
}
if (matches(elt, 'input[type="submit"], button') &&
(matches(elt, '[form]') || closest(elt, 'form') !== null)) {
(/** @type HTMLInputElement|HTMLButtonElement */(elt).form)) {
return true
}
if (elt instanceof HTMLAnchorElement && elt.href &&
Expand Down Expand Up @@ -2804,7 +2804,7 @@ var htmx = (function() {
if (!elt) {
return
}
const form = resolveTarget('#' + getRawAttribute(elt, 'form'), elt.getRootNode()) || closest(elt, 'form')
const form = (/** @type HTMLInputElement|HTMLButtonElement */(elt).form) || closest(elt, 'form')
if (!form) {
return
}
Expand Down Expand Up @@ -3521,9 +3521,10 @@ var htmx = (function() {
validate = validate && internalData.lastButtonClicked.formNoValidate !== true
}

// for a non-GET include the closest form
// for a non-GET include the associated form, which may or may not be a parent element of elt
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 •
edited
Loading

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.

const form = (/** @type HTMLInputElement|HTMLButtonElement */(elt).form) || closest(elt, 'form')
processInputValue(processed, priorityFormData, errors, form, validate)
}

// include the element itself
Expand Down
42 changes: 42 additions & 0 deletions test/core/ajax.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1184,6 +1184,48 @@ describe('Core htmx AJAX Tests', function() {
values.should.deep.equal({ t1: 'textValue', b1: ['inputValue', 'buttonValue'] })
})

it('sends referenced form values when a button referencing another form is clicked', function() {
var values
this.server.respondWith('POST', '/test3', function(xhr) {
values = getParameters(xhr)
xhr.respond(205, {}, '')
})

make('<form id="externalForm" hx-post="/test">' +
'<input type="text" name="t1" value="textValue">' +
'<input type="hidden" name="b1" value="inputValue">' +
'</form>' +
'<form hx-post="/test2">' +
'<input type="text" name="t1" value="checkValue">' +
'<button id="submit" form="externalForm" hx-post="/test3" type="submit" name="b1" value="buttonValue">button</button>' +
'</form>')

byId('submit').click()
this.server.respond()
values.should.deep.equal({ t1: 'textValue', b1: ['inputValue', 'buttonValue'] })
})

it('sends referenced form values when a submit input referencing another form is clicked', function() {
var values
this.server.respondWith('POST', '/test3', function(xhr) {
values = getParameters(xhr)
xhr.respond(204, {}, '')
})

make('<form id="externalForm" hx-post="/test">' +
'<input type="text" name="t1" value="textValue">' +
'<input type="hidden" name="b1" value="inputValue">' +
'</form>' +
'<form hx-post="/test2">' +
'<input type="text" name="t1" value="checkValue">' +
'<input id="submit" form="externalForm" hx-post="/test3" type="submit" name="b1" value="buttonValue">' +
'</form>')

byId('submit').click()
this.server.respond()
values.should.deep.equal({ t1: 'textValue', b1: ['inputValue', 'buttonValue'] })
})

it('properly handles inputs external to form', function() {
var values
this.server.respondWith('Post', '/test', function(xhr) {
Expand Down
4 changes: 2 additions & 2 deletions www/content/docs.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -640,8 +640,8 @@ will include the values of all inputs within it.

As with HTML forms, the `name` attribute of the input is used as the parameter name in the request that htmx sends.

Additionally, if the element causes a non-`GET` request, the values of all the inputs of the nearest enclosing form
will be included.
Additionally, if the element causes a non-`GET` request, the values of all the inputs of the associated form will be
included (typically this is the nearest enclosing form, but could be different if e.g. `<button form="associated-form">` is used).

If you wish to include the values of other elements, you can use the [hx-include](@/attributes/hx-include.md) attribute
with a CSS selector of all the elements whose values you want to include in the request.
Expand Down