bigskysoftware / intercooler-js

Making AJAX as easy as anchor tags
http://intercoolerjs.org
MIT License
4.78k stars 235 forks source link

Button name is not included in form requests #115

Closed OvermindDL1 closed 8 years ago

OvermindDL1 commented 8 years ago

I have a couple of buttons in a form, they all have "name" attributes, and when one is clicked the form submits and the browser includes the button name in the POST parameters, however when adding an "ic-post-to" to the buttons the form submits as expected, but the button name that was clicked on is not included.

OvermindDL1 commented 8 years ago

Also happens if the "ic-post-to" is on the form instead.

1cg commented 8 years ago

Hmmm.  We serialize the form:

  https://github.com/LeadDyno/intercooler-js/blob/master/src/intercooler.js#L546

I guess the button clicked is a dynamic value, though, so there is no way for the form to know to include it.

Hmmmm.  Maybe in that first block we should test if the elt is a button an include it as well….

Does that sound right?

Cheers, Carson

On September 12, 2016 at 3:46:26 PM, OvermindDL1 (notifications@github.com) wrote:

I have a couple of buttons in a form, they all have "name" attributes, and when one is clicked the form submits and the browser includes the button name in the POST parameters, however when adding an "ic-post-to" to the buttons the form submits as expected, but the button name that was clicked on is not included.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

1cg commented 8 years ago

Oh, I should mention, we do include the name and the id of the triggering element as ‘ic-trigger-name’ and ‘ic-trigger-id’ request variables, so you could use those in the meantime, if you want a work around.

Cheers, Carson

On September 12, 2016 at 3:46:26 PM, OvermindDL1 (notifications@github.com) wrote:

I have a couple of buttons in a form, they all have "name" attributes, and when one is clicked the form submits and the browser includes the button name in the POST parameters, however when adding an "ic-post-to" to the buttons the form submits as expected, but the button name that was clicked on is not included.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

1cg commented 8 years ago

Thinking about it, if you put the ic-post-to on the form (which is a better approach generally) we don't have the context of what button is clicked because we are in the submit event. :/

Hmmmm.

OvermindDL1 commented 8 years ago

Thinking about it, if you put the ic-post-to on the form (which is a better approach generally) we don't have the context of what button is clicked because we are in the submit event. :/

Yep, that was the first thing I tried, then I tried it on the button and still did not get the information of which button was clicked.

Oh, I should mention, we do include the name and the id of the triggering element as ‘ic-trigger-name’ and ‘ic-trigger-id’ request variables, so you could use those in the meantime, if you want a work around.

I saw that, but that would involve changing the server-side to be less generic than how it is now, was thinking about it though.

But yeah, it is currently breaking forms with multiple buttons as the spec states that a 'name'd button that is clicked should be sent along with the rest of the form data. See https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/button at the 'name' description.

pwFoo commented 8 years ago

Would be great if the button name / id would also be submitted, because the used form class check the button name if form was sent or not.

Workaround is to set my needed post values if ic-trigger-name is set, but would be nice without the need of that workaround ;)

OvermindDL1 commented 8 years ago

Current development branch should be fixed I 'think'. :-)

pwFoo commented 8 years ago

Tested dev and 1.0.0, but both I need my workaround to set the "name" POST value of my button.

1cg commented 8 years ago

It’s in the current dev branch, here’s the change:

  https://github.com/LeadDyno/intercooler-js/commit/c4cc5ab493b11e83c9850b3d2929d9ae28f566d2

Will be out in 1.0.1, probably next week.  (Sorry, this week is slammed for me.)

Cheers, Carson

On September 19, 2016 at 11:23:14 AM, pwFoo (notifications@github.com) wrote:

Tested dev and 1.0.0, but both I need my workaround to set the "name" POST value of my button.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

pwFoo commented 8 years ago

Hi @carsongross strange. Tested master and dev and it doesn't work without workaround. But the linked version works fine. Tested dev again and now it works too... Maybe a local cache problem...

Just good to know it will work out of the box in the future! Thanks!!!

pwFoo commented 8 years ago

I'm confused... Today it stopped working again (without a change). Tested it again with stable, dev and https://github.com/LeadDyno/intercooler-js/commit/c4cc5ab493b11e83c9850b3d2929d9ae28f566d2.

ic-request:true
username:<USERNAME>
password:<PASSWORD>
TOKEN859533586X1474710336:i0rJ5QwkIuJAtGkAYnKcd8D4sasrZpM2
ic-element-id:fhSubmit1
ic-element-name:fhSubmit1
ic-id:1
ic-target-id:pageContent
ic-trigger-id:fhSubmit1
ic-trigger-name:fhSubmit1
ic-current-url:/pwic/frontenduser/
_method:POST

button name is missing here... Workaround is needed again... ?!

// set button name to $_POST
if ($input->post['ic-trigger-name']) {
    $input->post[$fu->form->fhSubmitBtn->name] = $input->post['ic-trigger-name'];
}

Non ajax form submit:

username:<USERNAME>
password:<PASSWORD>
fhSubmit1:Login              // <-- button name
TOKEN2054235705X1474711122:p/8Pl0TVUwl1SydqWhbh6wVZW0PJeKnT
1cg commented 8 years ago

That doesn’t make any sense.  It was working and now it isn’t, with no change?

pwFoo commented 8 years ago

Maybe the working file was cached first...? Don't know. Tested, verified loaded source and post values with each version of file (1.0.0, dev, linked commit version) without success. I have no idea what local issue could cause it...

One version should work, because I tested it successful in the past... But I don't know which one... Maybe I should wait for 1.0.1 and verify with a clean environment...

1cg commented 8 years ago

Mmm.  I’m hoping to have a 1.0.1 release on Friday.  If you can wait, that’d be good to test with.

Cheers, Carson

On September 26, 2016 at 11:06:17 AM, pwFoo (notifications@github.com) wrote:

Maybe the working file was cached first...? Don't know. Tested, verified loaded source and post values with each version of file (1.0.0, dev, linked commit version) without success. I have no idea what local issue could cause it...

One version should work, because I tested it successful in the past... But I don't know which one... Maybe I should wait for 1.0.1 and verify with a clean environment...

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

pwFoo commented 8 years ago

Yes, fine for me. No stress. I can wait ;) Thanks

kezabelle commented 7 years ago

Apologies for dredging this back up, but I'm seeing an inconsistency in how this works, though this is anecdotal data. In Chrome, closestForm.find('button:focus') yields the button which triggered the IC event. But in Firefox, the selector returns 0 items. I can't find a current bug against Firefox for it, but reports do suggest that focus may not always be given.

This might mean that a different way of discovering the triggerer may be necessary if button.length is 0 but there were buttons in the closestForm. As I'm not entirely sure elt will represent the clicked element at all times, I'm not sure of the best solution.

1cg commented 7 years ago

Hi There,

Do you have the ic-post-to on the button, or on the form? I have a test for this and it passes in the version of Firefox I use for testing.

Cheers, Carson

On February 21, 2017 at 7:09:10 AM, Keryn Knight (notifications@github.com) wrote:

Apologies for dredging this back up, but I'm seeing an inconsistency in how this works, though this is anecdotal data. In Chrome, closestForm.find('button:focus') yields the button which triggered the IC event. But in Firefox, the selector returns 0 items. I can't find a current bug against Firefox for it, but reports https://gist.github.com/cvrebert/68659d0333a578d75372 do suggest https://developer.mozilla.org/en-US/docs/Web/HTML/Element/button#Clicking_and_focus that focus may not always be given.

This might mean that a different way of discovering the triggerer may be necessary if button.length is 0 but there were buttons in the closestForm. As I'm not entirely sure elt will represent the clicked element at all times, I'm not sure of the best solution.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/LeadDyno/intercooler-js/issues/115#issuecomment-281370961, or mute the thread https://github.com/notifications/unsubscribe-auth/AAcovzFey4rs9mHOSFIDtZHKqFhQF2bXks5rev4IgaJpZM4J7HO_ .

kezabelle commented 7 years ago

The form was decorated with ic-post-to, though I think I also tried shoving it on the individual buttons. It is entirely possible that I've either done something wrong, or other code is interfering, so I do need to try and pare it down to a small test case to confirm it.

The version of Firefox being used is 51.0.1 (64 bit) on OSX 10.11.6.

kezabelle commented 7 years ago

Minimal case from which I can reproduce the issue, using ic-post-to on the form (so triggered on submit) https://gist.github.com/kezabelle/91dfc75820005321b7b95f2282640ba0

As you'd expect, if I remove the ic-post-to, Firefox submits the clicked button's value (if it has a name), which makes the ic-* options not fully a progressive-enhancement :(

1cg commented 7 years ago

Can you run this unit test with your browser and let me know if it passes:

http://intercoolerjs.org/release/unit-tests-1.1.0.html?testId=26d7d883

(if it passes... REEEEEEEEEEEEEEE)

kezabelle commented 7 years ago

Well, it makes matters frustratingly less clear, because ... sometimes it passes.

REEEEEEEEEEEEEEE

I'm not sure I know what that means? :)

screen shot 2017-02-23 at 08 45 01 screen shot 2017-02-23 at 08 48 06

1cg commented 7 years ago

Hi Keryn,

Can you try this code out:

https://github.com/LeadDyno/intercooler-js/blob/aggressive_button_submit_fix/src/intercooler.js

I'm aggressively getting the clicked button value.

Cheers, Carson

On February 23, 2017 at 12:51:13 AM, Keryn Knight (notifications@github.com) wrote:

Well, it makes matters frustratingly less clear, because ... sometimes it passes.

REEEEEEEEEEEEEEE

I'm not sure I know what that means? :)

[image: screen shot 2017-02-23 at 08 45 01] https://cloud.githubusercontent.com/assets/118377/23251409/15762cac-f9a5-11e6-9b81-a4277d14faa0.png [image: screen shot 2017-02-23 at 08 48 06] https://cloud.githubusercontent.com/assets/118377/23251410/15775e60-f9a5-11e6-8dab-7fca42076c1e.png

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/LeadDyno/intercooler-js/issues/115#issuecomment-281931963, or mute the thread https://github.com/notifications/unsubscribe-auth/AAcov9tjDi26LmJ2vP-Ls56c2T3jFxxUks5rfUiBgaJpZM4J7HO_ .

kezabelle commented 7 years ago

Perhaps I'm being dense, but that tree appears to be 2 commits behind master, and hasn't got any difference of note and looks to be the same point-in-time as 1.1.0 ... perhaps you forgot to push the changes upstream? :)

1cg commented 7 years ago

Looks like it was on me, try it now:

https://github.com/LeadDyno/intercooler-js/blob/aggressive_button_submit_fix/src/intercooler.js

Cheers, Carson

On February 25, 2017 at 5:12:01 AM, Keryn Knight (notifications@github.com) wrote:

Perhaps I'm being dense, but that tree appears to be 2 commits behind master, and hasn't got any difference of note https://github.com/LeadDyno/intercooler-js/compare/aggressive_button_submit_fix...master and looks to be the same point-in-time as 1.1.0 https://github.com/LeadDyno/intercooler-js/compare/aggressive_button_submit_fix...v1.1.0 ... perhaps you forgot to push the changes upstream? :)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/LeadDyno/intercooler-js/issues/115#issuecomment-282483156, or mute the thread https://github.com/notifications/unsubscribe-auth/AAcovwsOFLZpMjfwmaLPYG3Rkv_GjN7Rks5rgCiggaJpZM4J7HO_ .

kezabelle commented 7 years ago

I've only tested it (using my previous gist) in FirefoxDeveloperEdition (53.0a2) on OSX 10.10.5 but that seems to work perfectly. FWIW the only other useful reference about the issue I can readily find is on document.activeElement which has a throwaway remark about focus not necessarily being given on OSX.

I'll re-test it again when I'm back in front of the other machine from which I originally found the problem ( Firefox 51.0.1, OSX 10.11.6) for a second anecdotal data point, but its looking promising.

kezabelle commented 7 years ago

Re-tested against Firefox 51.0.1, OSX 10.11.6 and the changes you've made in agressive_button_submit_fix seem to work there, too. So that's 2 datapoints that suggest its an improvement, hopefully without regression for other browsers :)

styxxx commented 6 years ago

Hello,

I just found this issue while looking for a solution for my problem that sounds like it was the same: I've an element:

< div id="content-block"> < form ic-post-to="http:..." ic-target="#content-block"> < input type="submit" name="somename" value="somevalue"> ...

And the name/values pairs aren't sumbitted to the form by intercooler when the button is clicked (all other form data is). I figured out that no values from submit type inputs are included ever. Unfortunately I can't change the html code since it is generated by some CMS.

I tried with intercooler 1.1.2 and 1.2.1.

I tried the provided unit tests and noticed: "Form submission with submit button works (1, 0, 1)" fails. But works when being repeated. "Button value is included in request (2, 4, 6)" also partly fails, but "Submit input value is included in request" always passes. But it doesn't seem to work "in the real world" for some reason.

Using google chrome stable: User-Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/66.0.3359.170 Safari/537.36

1cg commented 6 years ago

passes in tests, breaks in real world

I love being a developer.

styxxx commented 6 years ago

:D

btw, I came up with a workaround: I just added a small JS code so when the button is clicked a hidden input element is being created before the form is sent.

ThisNoName commented 6 years ago

@styxxx Probably nobody is watching this. I'm having the same issue, but even stranger.

I have ic-post-to on the form and multiple buttons to differentiate actions. Everything worked perfectly until I try to make some change. Now the submit button is no longer included on localhost. But when deploy to test server, everything works fine.

1cg commented 6 years ago

Well, I'm watching it, but unfortunately it seems like a heisenbug so I'm not sure what to do about it. :/

MutatedGamer commented 5 years ago

I'm having this issue (https://stackoverflow.com/questions/53339930/intercooler-js-not-sending-form-data-correctly-on-safari?noredirect=1#comment93559143_53339930) and wondering if there was ever a fix found?

1cg commented 5 years ago

Can you try putting a value on the buttons and see if that addresses the issue? I do have tests for this that pass in safari.

MutatedGamer commented 5 years ago

Tried that already, to no avail.

1cg commented 5 years ago

Ugh, really sorry about that. I have tests! :(

OK, what about a different approach? Can you put an ic-post-to="/some/other/action" on the button that isn't the main submission button? The form will still be included when the user clicks that button, but it will be submitted to a different URL making it easy to disambiguate.

Again, sorry for this nagging issue.

MutatedGamer commented 5 years ago

I'm away from my workstation right now so I can't test this, but I think I see a difference in my setup versus your tests. Both of my buttons are "main submission buttons," and both have type = "submit".

I have a work-around where I put the intercooler attributes on each button, and then determine which button was used to submit the request by checking the ic-tigger-name. Would you like me to continue debugging the root issue with you, given that this is a valid work-around?

1cg commented 5 years ago

We should probably get a test in for this situation. You mind copying the test and switching it to submits?

kezabelle commented 5 years ago

As conversation about this problem has continued, I'm also continuing it ... if this should be expanded into a separate/new ticket, sorry :)

It's still possible to "lose" the value of a selected button.

Consider:

<form method="post" action="...">
<button name="testing"  value="1" type="submit"  ic-post-to="..." ic-trigger-on="default">test</button>
</form>

However, the bit of code (handleTriggerOn(elt)) which attaches event handlers for .data('ic-last-clicked-button') will receive an elt = <button> but does not check for a closestForm to set up .data(...) binding, only a <form> - see handleTriggerOn here. Thus the testing=1 pair may not be included in the payload.