Opened 11 years ago

Closed 11 years ago

Last modified 10 years ago

#7387 closed defect (wontfix)

dojo.query reorders form elements -> dojo.xhrPost mangles order of submission

Reported by: Matt Sgarlata Owned by: James Burke
Priority: high Milestone: 1.2
Component: Query Version: 1.1.1
Keywords: Cc: development-staff@…
Blocked By: Blocking:

Description (last modified by bill)

First let me say in most cases query/xhrPost is fine. Here is a case where it is not:

<input type="hidden" name="variableName" value="selectVariable">
<select name="variableValue">
<option selected value="This should be first">
</select>

<input type="hidden" name="variableName" value="inputVariable">
<input type="text" name="variableValue" value="This should be second">

The variable names come through in the correct order: selectVariable, inputVariable

The variable values come through with all input elements first, followed by select elements like this: This should be second, This should be first.

Of course in this example I only have one select and one text box, but in my production app I have lots of both and all the input box values come through first in the same order they appear on the page, then all the select box values come through second in the same order they appear on the page. To be consistent with how a form submission would work outside dojo, the order should not be determined by the type of form element, but rather simply the order the elements appear on the page.

I did extensive testing I'm pretty sure the root source of the problem is dojo.query returns the elements in the wrong order. dojo.xhrPost and dojo.formToObject are just working with the elements as returned by dojo.query.

This is not an issue in dojo 0.4 (we are working on migrating from 0.4 to 1.1 now).

Attachments (2)

dojoBug7387Test.html (428 bytes) - added by Matt Sgarlata 11 years ago.
Test case
7387.patch (1.6 KB) - added by James Burke 11 years ago.
Possible patch, needs more testing.

Download all attachments as: .zip

Change History (26)

comment:1 Changed 11 years ago by James Burke

I am not aware of any guarantee of precise name=value pair order when using an HTTP POST with form encoding. In fact, I thought the goal of having name=value pairs was so that order was not important when the data reached the server.

While it may have worked out in the past that the order worked out, it seems like from a robustness point of view, the server should not assume a specific order when processing name/value pairs? Most server frameworks from what I recall actually do not care about the order -- they normally provide a hashmap or dictionary approach to getting the values for the named parameters. Or did I misunderstand the issue?

comment:2 Changed 11 years ago by Matt Sgarlata

The problem is not that name=value pairs are submitted in the wrong order (I'm not sure what order they're submitted in to be honest). The problem is with a variable that has been overloaded so that it appears multiple times in the form. In this case the order is crucial because there is no way (like variable name) to distinguish values submitted to the server.

All major browsers work fine with this approach (Safari, FF, IE6, IE7) and so does dojo 0.4. As a matter of fact, dojo 1.1 also works, but only for the case when all of the form elements are of the same type. In the example, the parameter with the name "variableName" is submitted to the server with the correct order. The parameter with the name "variableValue" is submitted in the incorrect order.

comment:3 Changed 11 years ago by bill

Description: modified (diff)

Which browsers does this problem occur on? I presume safari's dojo.query() is using xpath whereas IE's is not, so the results may be different. Also, you should attach a full testcase using the attach file button just to make it crystal clear how to reproduce.

comment:4 Changed 11 years ago by Matt Sgarlata

I did my testing on FF 3.01 on Mac OS X Leopard. I will prepare a test case and perform testing on additional browsers/platforms later today.

Changed 11 years ago by Matt Sgarlata

Attachment: dojoBug7387Test.html added

Test case

comment:5 Changed 11 years ago by Matt Sgarlata

I just uploaded a test case. I refer to dojo with the URL dojo/dojo.js, so be sure to place the test in an appropriate directory. Next, click on the Test button and the application will report "This should be second,This should be first". Of course the correct output would be "This should be first,This should be second"

This test case failed for the following platforms and browsers: FF 3.0.1 on Mac OS X 10.5.4 Safari 3.1.2 on Mac OS X 10.5.4 FF 3.0.1 on Windows Media Center (basically Windows XP) IE6 on Windows Media Center (basically Windows XP)

comment:6 Changed 11 years ago by bill

Oh I see, yah, the dojo.query is

input:not([type=file]):not([type=submit]):not([type=image]):not([type=reset]):not([type=button]), select, textarea

I bet it's first scanning for <input> nodes and then scanning for <select> nodes (and finally scanning for <textarea> nodes) as per the comma separated list.

It's a pretty unusual case though to have a <select> and an <input> w/the same name.

comment:7 Changed 11 years ago by Matt Sgarlata

I agree it is an unusual case, but it is supported by all the major web browsers when doing a normal form submission (I've tested FF, IE6, IE7 and Safari).

comment:8 Changed 11 years ago by dylan

Component: GeneralCore
Owner: changed from anonymous to alex

comment:9 Changed 11 years ago by dylan

Alex, Peter: it this something we should fix? Presumably it works the way it does to optimize performance over an edge case.

comment:10 Changed 11 years ago by James Burke

I have not looked at the code yet, but I wonder if we can address this after the query step, once all the parameters are collected? That may help performance if the query step might slow things down.

comment:11 Changed 11 years ago by bill

Personally I would just close this as "wontfix" since it's such an edge case. I can't think of anyway to "reorder" the items to their natural order after the internal queries on "input" and "select" have finished. Perhaps there's a simple update to the code to support this but I don't think so (and I don't think it's worth a lot of new lines of code)

Changed 11 years ago by James Burke

Attachment: 7387.patch added

Possible patch, needs more testing.

comment:12 Changed 11 years ago by James Burke

Milestone: tbd1.2
Owner: changed from alex to James Burke

Attached a patch that I think is shorter and probably performs better than before, and fixes the issue. But I still need to do more testing. Using formNode.elements to cycle through form elements. Not sure yet if this is robust.

comment:13 Changed 11 years ago by bill

Oh good job; I wish I had thought of that approach myself. Yah, no reason to use dojo.query at all.

comment:14 Changed 11 years ago by Matt Sgarlata

This is working great for me. Thanks so much!

comment:15 Changed 11 years ago by James Burke

Resolution: fixed
Status: newclosed

Fixed in [14881].

comment:16 Changed 11 years ago by Pete Smith

Resolution: fixed
Status: closedreopened

This breaks something that worked in 1.1 - and a feature I relied on.I would hand this formToObject a "node" (in my case a fieldset) and it would magically deliciously work. Now, it does not. It was not a formNode - it just was a domnode and it would give me an object with the values. Now it doesnt.

comment:17 Changed 11 years ago by James Burke

Resolution: wontfix
Status: reopenedclosed

httpete: sorry for the inconvenience. However, I feel the fix to conform to how forms post via HTTP is more important, and correct to do. Also, the naming of the APIs (formToObject) imply that a form is needed for the API call. I think the fieldset behavior you describe above is a casualty of fixing a real bug in the code.

You could make your own formToObject function based on the old 1.1 code, and use that if you wanted to upgrade to 1.2.

comment:18 Changed 11 years ago by jlevy

I've been watching this issue closely since it appeared. dojo/dojo/_base/xhr.js r14789 being the last useful incarnation of this file; passing a domNode to a form : attr in a xhrPost query was incredibly useful. This arbitrary change "on principle" has seriously hobbled xhr.js. I have been forced to hold back on this file during upgrades, and it's seriously affecting my applications.

comment:19 Changed 11 years ago by James Burke

jlevy: are you saying that dojo.xhrPost { form: formNode} does not work now, where formNode is a handle to a form, or a string with the ID of a form? I tried a quick test, and that seems to still work for me. If you have a test case I can try, I can investigate more.

If formNode does not map to a form node and you want the old behavior, then you can take the dojo.formToObject function from Dojo 1.1.1 and have that one override the one that is in the Dojo 1.2 distribution by including it in one of your modules.

comment:20 Changed 11 years ago by jlevy

jburke: I'm sure the resolution you indicate is adequate. I have not yet had the opportunity to try it. I'll work up a test-case against the latest dojo code, and update my comment, if necessary. I do believe, though, that simply passing a containing widget id will not work as it used to. Granted, I believe the method I used for this xhrPost technique was a convenience, and honestly, it may be better to do things 'your way'. I've embedded a bit of code to this comment to give some idea as to what I've been doing in the past (for clarification's sakes)

Pardon any glaring errors in markup.

<html>
<div id="myContainer" dojoType="dijit.layout.TabContainer">
  
  <!-- 1st container -->
  <div id="myContained_01" dojotType="dijit.layout.ContentPane" title="Contained, 01">
    <form id="contained_form_01" method="post">
      <input id="my_input_contained_01" dojotType="dijit.form.textBox" />
    </form>
  </div>

  <!-- 2nd container -->
  <div id="myContained_02" dojotType="dijit.layout.ContentPane" title="Contained, 02">
    <form id="contained_form_02" method="post">
      <input id="my_input_contained_02" dojotType="dijit.form.textBox" />
    </form>
  </div>

</div>
</html>

Then, the javascript:

(sorry for lack of pretty-print, but this trac has no javascript macro)

dojo.xhrPost({
  url: '/my/controller',
  handleAs: 'json',
  timeout: 15000,

  form: 'myContainer',
  content: { supplementary : 'foobar' },

  load: function(r, args) {
    console.dir(r);
  },

  error: function(r, args) {
    console.dir(r);
  }
});

In my example, not that form: is not a form node, rather, it's my tabContainer. Prior to r14789, xhr.js did it's magick. Both forms would be transmitted to the receiving controller, my/controller. Post-14789, nada. Whilst the form is sent, there are no values.

comment:21 Changed 11 years ago by dante

@jlevy - dijit widgets don't all support having native input nodes so running formToObject (which used to just query from a node for inputs) now simply serializes a real form. Either dijit needs to implement a dijit.formToObject that will detect .getValues() existence or needs to implement a policy of having a stateful input with the current value always available to formToObject. Currently, mixing the two are not supported, though it worked in your case as a side effect. The problem is, Dojo cannot assume the presence of Dijit (by design) so making dojo.formToObject recognize dijit form widgets isn't a possibility. While unfortunate, it simply exposes a larger problem when differentiating normal dom activity versus dijits. What is further unfortunate is that dijit.form.Form doesn't recognize native inputs in its submission, otherwise the solution would be a lot more clear.

comment:22 Changed 11 years ago by James Burke

jlevy: Assuming you don't want the dijit-specific data, you might be able to do something like this instead:

var content = {
    supplementary: 'foobar'
};
dojo.query("form", dojo.byId("myContainer").forEach(function(form){
    content = dojo.mixin(content, dojo.formToObject(form);
});

dojo.xhrPost({
  url: '/my/controller',
  handleAs: 'json',
  timeout: 15000,

  content: content,

  load: function(r, args) {
    console.dir(r);
  },

  error: function(r, args) {
    console.dir(r);
  }
});

comment:23 Changed 11 years ago by bill

Guys, dijit widgets *do* all support having native input nodes. The issue above (under "Pardon any glaring errors in markup.") is unrelated to dijit; it's about passing in a <div> instead of a <form>.

comment:24 Changed 10 years ago by bill

Component: CoreQuery
Note: See TracTickets for help on using tickets.