This project is archived and is in readonly mode.

#5356 ✓resolved
Andrew Kaspick

[PATCH] rails.js fix for submitting forms with multiple submit buttons

Reported by Andrew Kaspick | August 11th, 2010 @ 07:59 AM

Prototype contains a bug which doesn't serialize forms properly when submitting data for an ajax request. If a form contains multiple submit buttons, the value for the first button appears to always be serialized (and sent) to the server even if the user clicks a different button to submit the form.

Based on existing tickets for prototype (https://prototype.lighthouseapp.com/projects/8886/tickets/672-forms..., this doesn't look like it will be fixed anytime soon either, so instead we can try to fix it in rails.

<form ...>
  <input type="submit" name="update" value="Update"/>
  <input type="submit" name="delete" value="Delete"/>
</form>

For this form (if set up for ujs), it doesn't matter which button is pressed to submit the form; prototype will always send {:update => 'Update'} in the POST params.

My patch listens for the button click event and renames the "name" attribute before the values are sent to the server so that we can properly check for the correct value for the submit buttons.

So when the user clicks the "delete" button, the form changes to...

<form ...>
  <input type="submit" name="delete" data-original-name="update" value="Update"/>
  <input type="submit" name="delete" value="Delete"/>
</form>

Prototype will now serialize the first submit button as {:delete => "Update"}, which allows us to check for the "delete" action now. The value isn't updated because it's too fragile of a value to be checking on the server side anyway... only the name value should be checked.

After the data is posted in rails.js, we just reset the submit buttons back to their original values.

I'm not the most fluent with javascript, so please correct any issues with my code. Or provide feedback, comments on changes.

Comments and changes to this ticket

  • Mislav

    Mislav August 11th, 2010 @ 12:02 PM

    Hey Andrew,

    It's a good idea to try to solve this, but I don't agree this is a good approach.

    Prototype form serialization is done by the Form#serializeElements method, which—as you noticed—by default includes the first submit button because it can't know which one was pressed. However, it supports an optional argument submit with which you can specify the name of the submit button you want to include instead of the first button.

    So, in your case we would want the serialize method in rails.js to get invoked like this:

    params = element.serialize({ submit: 'delete' });
    

    Now the problem remains how do we know which submit button was pressed? There are 3 ways a user can trigger form submit that I can think of:

    1. hitting the Return key while in a text input element: unless it was a TEXTAREA, this submits the form as if the first submit button was pressed. because Prototype already serializes by this, we're OK.
    2. pressing the submit button with the mouse or finger: a mouseup or touch event on a button means it got pressed.
    3. activating the submit button with keyboard: a keypress of the Return (and possibly Spacebar?) key means the currently focused submit button got pressed.

    My suggestion is to observe all these events on submit buttons which can trigger for submit, attach a piece of metadata on the form with Element#store indicating the name of the button, then use that information when calling serialize:

    params = element.serialize({ submit: element.retrieve('rails-pressed-button') });
    
  • Andrew Kaspick

    Andrew Kaspick August 11th, 2010 @ 03:37 PM

    Great feedback. Some bits of prototype I wasn't aware of there. I'll rework this with your suggestions as soon as I can. Thanks.

  • Andrew Kaspick

    Andrew Kaspick August 13th, 2010 @ 07:45 PM

    I've attached a new version.

    In my testing a mouse click, space bar press and enter key press all register the 'click' event. A mouseup event doesn't mean a button was pressed though. I can click anywhere on the page, hold the button down and move it over a button and release it for a mouseup. Either way, the 'click' event appears to be the only reliable event that is triggered for the space bar, enter key and mouse. I've also changed the code to use the 'store'/'retrieve' method in prototype.

    Please let me know if any changes to method names, code style, logic, etc need to be changed.

  • Larry Sprock

    Larry Sprock August 13th, 2010 @ 09:16 PM

    Andrew,

    Using your latest patch I am getting nothing returned. Line 23 should be: return submit.name;

  • Larry Sprock

    Larry Sprock August 13th, 2010 @ 09:27 PM

    Oops 79 not 23, was looking at the patch not the file but just to be clear...

    function whichSubmitButton(form, reset) {
      var submit = form.select('input[type=submit]').detect(function(input){
        return input.retrieve('rails-submit-button') === true;
      });
      if (reset === true) {submit.store('rails-submit-button', null);}
      return submit.name; //added .name to submit
    }
    
  • Andrew Kaspick

    Andrew Kaspick August 14th, 2010 @ 01:12 PM

    Thanks Larry. It appeared to be working for me locally, but looking at the code for serializeElements in prototype, it does seem to do a comparison with the 'name' value.

    I've attached a diff with the newest commit. My change is slightly different and does a check for the submit element before returning the name attribute.

    Please review and let me know if things are working ok.

    Thanks

  • Mislav

    Mislav August 14th, 2010 @ 03:22 PM

    This wasn't exactly what I had in mind. If you store the name of the submit button on the form rather than on the button itself, you don't need the whichSubmitButton function.

    document.on('click', 'form input[type=submit]', function(event, button) {
      // register the pressed submit button
      event.findElement('form').store('rails:submit-button', button.name || false);
    });
    
    // and then:
    params = element.serialize({ submit: element.retrieve('rails:submit-button') });
    

    The complete solution is much less complex and handles the case when the submit button didn't have a "name" to start with (no submit button value should be serialized in that case).

  • Andrew Kaspick

    Andrew Kaspick August 14th, 2010 @ 07:38 PM

    Storing on the form makes much more sense. I'll get another patch together soon. Thanks.

  • Larry Sprock

    Larry Sprock August 14th, 2010 @ 08:01 PM

    Here's a patch. Put Andrews name on it since he's been doing the work.

  • Mislav

    Mislav August 14th, 2010 @ 08:40 PM

    • Assigned user set to “josh”

    Indenting looks off, apart from that it's fine. I'd like you guys to test this in browsers that Prototype supports (as well as touch devices if you have any) and report here so whoever commits this to core knows that it's a real solution.

  • Larry Sprock

    Larry Sprock August 14th, 2010 @ 09:54 PM

    Yeah, funky stuff with tabs in textmate. Format correction attached.

  • Larry Sprock

    Larry Sprock August 14th, 2010 @ 11:36 PM

    Created a little rails app for testing in different browsers. It just returns the name and value from the button pressed pulled from the params in the controller.

    http://ajax-button-test.heroku.com

    iphone iOS4: working
    safari 5.0.1: working
    firefox 3.6.8: working

    These are all the browsers I could test at the moment.

    Also I had to perform a little hakery in rails.js to get the responseText. Tried using observe ajax:success but was only getting [object Event] returned which was probably fire... what is the best way to get the response from ajax:success using the current implementation of rails.js?

  • Andrew Kaspick

    Andrew Kaspick August 15th, 2010 @ 06:18 AM

    I'm away traveling so am a bit staged with getting online. I noticed Larry attached a new patch. I have a new patch as well, but don't do the "|| false" part as that stops any of the "submit" values from being serialized, is that what we are going for if nothing is found then?

    I'll attach my patch as well just in case, but if we're leaving the || false part it then we can just use Larry's latest.

  • Mislav

    Mislav August 15th, 2010 @ 11:41 PM

    @Andrew: the || false fallback is what we want if the button doesn't have a name attribute. In this case, { submit: false } will be given to serialize method and no button value should be serialized. I'm pretty sure this is native browser behavior, too.

    @Larry: rails.js fires custom events at all stages of an Ajax request. To access the responseText property, observe the "ajax:success" event and access its memo property:

    document.on("ajax:success", function(e) {
      $('responseText').update(e.memo.responseText)
    })
    

    The memo property on a custom event is any object that the code which fired the event in the first place wished to pass on. In rails.js case, this is the Ajax request object.

  • Andrew Kaspick

    Andrew Kaspick August 16th, 2010 @ 09:02 AM

    Fair enough. I just wanted to make sure that's what the plan for the submit button was. Besides that, Larry's patch is what mine would be then too.

  • Mislav
  • Andrew Kaspick

    Andrew Kaspick September 28th, 2010 @ 03:19 AM

    I noticed that the rails.js file that comes with rails 3 is still out of date and doesn't include this fix...

    http://github.com/rails/rails/blob/master/railties/lib/rails/genera...

    Shouldn't the original patch be committed to rails master to reflect this update? Otherwise why would the current rails.js be left as is in master?

  • Andrew Kaspick

    Andrew Kaspick September 28th, 2010 @ 03:20 AM

    To avoid any confusion, I didn't mean to say "original" patch there, just meant to say "patch" by itself... the one that was pushed to prototype-ujs.

  • Mislav

    Mislav September 29th, 2010 @ 06:55 PM

    The prototype-ujs project should be the source of edge rails.js for people that wish to update, not Rails master. I expect prototype-ujs to be updated much more frequently than it would make sense to merge back into Rails.

  • Andrew Kaspick

    Andrew Kaspick September 29th, 2010 @ 08:00 PM

    So everything committed to prototype-ujs will be merged for the next rails point release? I just thought it would have been merged for the final rails 3 release as well.

    This ticket might as well be closed now.

    Thanks

  • Jeff Kreeftmeijer

    Jeff Kreeftmeijer October 10th, 2010 @ 08:23 PM

    • State changed from “new” to “resolved”
    • Importance changed from “” to “Low”

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

Pages