Ruby on Rails | Screencasts | Download | Documentation | Weblog | Community | Source

Ticket #9609 (reopened enhancement)

Opened 1 year ago

Last modified 5 months ago

[PATCH] [TEST] Form.serializeElements() fails when form contains "toString"/"valueOf" - named elements

Reported by: gryn Assigned to: sam
Priority: normal Milestone:
Component: Prototype Version: edge
Severity: normal Keywords:
Cc:

Description

When calling Form.serialize, IE and Opera fail if the form contains certain elements.

Specifically, if the form contains a form sub-element (such as 'input') of name or id equal to 'length', it fails. Additionally, if the form contains any element with an id equal to 'length', it also fails. As is, the example below works in all browsers -- clicking on 'Click' produces 'me=' in an alert box.

However, if any lines commented with '<!--!!!' are uncommented, IE and Opera will fail to produce anything in the alert box. Lines commented with '<!--...' cause no errors on any browser.

IE 6SP2, 7 and Opera 9.21 were tested and failed. Firefox 2.0.7 and Safari for Windows 3.0.2 were tested and passed. Other browsers were not tested. Prototype 1.6.0_rc0 used for testing, the doctype was set to html 4.01 strict, mime-type 'text/html', quirks mode was not enabled for rendering.

I do not know if there is a spec that defines the correct behaviour in this test case. And I am not sure if it is feasible to fix. I primarily wanted to document the behaviour somewhere. If it cannot be fixed in the code, then the next best thing is some documentation somewhere prominent, for instance on prototype's API about Forms.

<script type="text/javascript">
Event.observe(window,'load',function() {
  Event.observe($$('legend')[0],'click',function() {
    alert( $('test').serialize() );
  });
});
</script>
<form id="test">
  <fieldset>
    <legend>Click</legend>
    <input name="me" />
<!--!!!    <input name="length" /> -->
<!--!!!    <input name="funny" id="length" /> -->
<!--!!!    <p id="length"></p> -->
<!--...    <p name="length"></p> -->
  </fieldset>
</form>
<!--... <p name="length"></p> -->
<!--... <p id="length"></p> -->
<!--... <form><div><input name="length" /></div></form> -->

Attachments

form_serialize_test_9609.diff (1.0 kB) - added by kangax on 10/22/07 14:36:42.
form_serialize_hackish_patch_9609.diff (0.6 kB) - added by kangax on 10/22/07 17:31:39.
toString_valueOf_patch.diff (0.7 kB) - added by kangax on 01/22/08 18:22:05.
serialize_elements_patch_test.diff (1.7 kB) - added by kangax on 01/22/08 23:13:42.
unified patch/test

Change History

09/23/07 04:15:04 changed by kangax

Oh boy...
Guess what, name="toString" fails too (and even in FF)
Empty name="length" indeed fails in IE (not sure if it is because of empty value or a general issue)
Here's a failing test of "name=toString" http://yura.thinkweb2.com/testing/form-serialize/ (tested against latest rev - 7360 )

10/22/07 14:36:13 changed by kangax

I'm removing the above mentioned link, instead, attaching a normal diff.

P.S. look like valueOf fails with the same pattern - returning

function toString(){ [native code] }

instead of actual value

10/22/07 14:36:42 changed by kangax

  • attachment form_serialize_test_9609.diff added.

10/22/07 17:31:16 changed by kangax

  • version set to edge.
  • summary changed from Prototype 1.6.0_rc0 fails Form.serialize() when form contains "length" id or name elements, IE and Opera only. to [PATCH] Form.serializeElements() fails when form contains "toString" or "valueOf" -named elements.

Ok, thinking about Safari 2 and IE 5, I really doubt hasOwnProperty is the "right" way to go, but this simple check makes IE6/7 and FF pass. I don't have Safari 2 to test this out, but am guessing that it will still be failing.

Any ideas?

10/22/07 17:31:39 changed by kangax

  • attachment form_serialize_hackish_patch_9609.diff added.

01/22/08 14:17:10 changed by kangax

  • summary changed from [PATCH] Form.serializeElements() fails when form contains "toString" or "valueOf" -named elements to [PATCH] [TEST] Form.serializeElements() fails when form contains "toString"/"valueOf" - named elements.

01/22/08 18:22:05 changed by kangax

  • attachment toString_valueOf_patch.diff added.

01/22/08 18:22:56 changed by kangax

Wow, I can't believe I have nailed this one down. The solution is (as always) trivial.

01/22/08 23:13:42 changed by kangax

  • attachment serialize_elements_patch_test.diff added.

unified patch/test

01/24/08 02:24:40 changed by andrew

  • status changed from new to closed.
  • resolution set to fixed.

(In [8713]) Properly serialize form fields with names that collide with built-in JS properties (like length or toString). Closes #9609. [gryn, kangax]

01/25/08 18:55:35 changed by savetheclocktower

  • status changed from closed to reopened.
  • resolution deleted.

01/25/08 18:56:27 changed by savetheclocktower

  • status changed from reopened to closed.
  • resolution set to wontfix.

Changing this to WONTFIX. The fix for this does not work in Opera. Instead, we'll have to rely on developers not to use "valueOf" or "toString" as field names in forms.

02/07/08 19:50:27 changed by gryn

  • status changed from closed to reopened.
  • resolution deleted.

I guess the original intention of the bug was lost somewhere along the way. Certainly valueOf and toString also fail, but don't forget about length, which was the initial bug report.

Additionally, I had wagered that it could not be fixed. What I suggested was a prominent 'gotcha' warning in the docs for Form.serialize. So, instead of WONTFIX, why can't this be "fixed" via documentation?

Pardon me for reopening the ticket, since I'm not familiar if you (the powers that be) will notice my comment unless I do.

02/07/08 23:36:31 changed by Tobie

  • status changed from reopened to closed.
  • resolution set to wontfix.

Well, this isn't a Prototype issue but a JavaScript one and falls under the same category, imho, than the shared namespace issues that exists in IE between the id and name attributes. I however agree that this could be documented. The documentation is about to be seriously revamped, so as to make user contribution a lot easier. I'm closing this ticket until then, as it will be a lot easier to correct once the new documentation is in place.

04/10/08 02:32:18 changed by kangax

This could actually be fixed with a simple check:

...
if (Object.prototype.hasOwnProperty.call(result, key))
...

The failing values are any of those which return "true" by "in" operator (i.e. all of the properties of Object.prototype):

length
toString
valueOf
toLocaleString
hasOwnProperty
isPrototypeOf

I think those are quite a lot of names to "ignore". Maybe we should fix this after all?

04/10/08 02:39:13 changed by jdalton

related to Object.prototype.hasOwnProperty: #11043

This effects more than just the Form.serializeElements (it also effects Hashes and other objects)

04/14/08 18:39:40 changed by jdalton

  • status changed from closed to reopened.
  • type changed from defect to enhancement.
  • resolution deleted.