332893 - <form> .elements["x"] order is not updated after reordering element using DOM

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[ctambrin]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.1) Gecko/20060111 Firefox/1.5.0.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.1) Gecko/20060111 Firefox/1.5.0.1

I was testing with DOM manipulation of form elements, namely try reordering them. But it seems like the form.elements didn't pick up the changes done by DOM manipulation (see steps to reproduce). I tried this on IE and it works.

Reproducible: Always

Steps to Reproduce:
1. Browse the sample test.html
2. Click [up] on the second row
3. Click [show],  
Actual Results:  
it shows 132

Expected Results:  
it shoud show 213 (as screen order)

The result is a bit random, I can't figured out any pattern in it.

Comment 1

[ctambrin]

Attachment: testing html to reproduce error

Comment 2

[Boris Zbarsky [:bzbarsky]]

Not a parser issue.  And form.elements itself is fine.  But you have multiple inputs all with the same name and the list for that name is not stored in document order.  It's stored in "last element placed in the DOM last" order.

If someone really cares, it could be stored in DOM sorted order; the function nsFormControlList::AddElementToTable in nsHTMLFormElement.cpp would need to be modified accordingly.  I'm not sure how much we care, however.  What do other UAs (other than IE) do?

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

IMHO Document order is always nice since it's predictable. Why do we cache this stuff at all rather then grabbing it dynamically from the elements array?

Comment 4

[Boris Zbarsky [:bzbarsky]]

You mean why do we have this name/id hashtable to start with instead of walking the array every single time?  Because it's a lot faster....

Wanna add some sorting code (of the sort that AddElement already has) here?

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

We could make walking the array very fast. If we atomize the requested name and then call AttrValueIs it'll end up being an atom compare plus a few functioncalls.

Though I guess if you have a lot of elements that might be too much. Though maybe lazily caching it a'la GetElementById might be enough. Granted, that might be more complex then what we do now, and just a small win in footprint.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Right.  Given how much this stuff is used, I don't think it's worth it to optimize for cases when it's not used....

Comment 7

[ctambrin]

Attachment: test2.html

Hi, I've just tested with Opera, it works as expected. 

Then I tested a bit more, that is to add append row (see test2.html). This time, none of the browser picked up the dynamically added element. Seems like if I want to use this technique, I need to find out the value of elements through DOM as well.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> none of the browser picked up the dynamically added element.

Your displayInput thing only shows three elements.

Comment 9

[ctambrin]

(In reply to comment #8)
> > none of the browser picked up the dynamically added element.
> 
> Your displayInput thing only shows three elements.

Click on the [add] button few times (for test2.html), so more elements added, then click on the [show]

Comment 10

[Boris Zbarsky [:bzbarsky]]

Yes.  The function that runs when I click "show" only shows three elements. By design.  It loops from one 1 to 3.

Comment 11

[ctambrin]

Attachment: test3.html

My sincere apology, in the rush I've forgotten that I hardcoded the loop. I corrected it in test3.html. The [add] works fine using firefox and IE. Only when moving [up], the sequence in firefox becomes wrong, while in IE is still correct. Opera also show correct sequence.

Comment 12

[jpl24]

Attachment: form_element_sort_v1

This adds sorting between form controls with the same name/id.

Comment 13

[jpl24]

Attachment: testcases

A few tests cases using onload. These should make it into the tests eventually.

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 237324 [details] [diff] [review]
form_element_sort_v1

>? content/svg/content/src/nsSVGGenericStringValue.cpp
>Index: content/base/src/nsContentList.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/base/src/nsContentList.cpp,v
>retrieving revision 3.95
>diff -u -p -8 -r3.95 nsContentList.cpp
>--- content/base/src/nsContentList.cpp	25 Aug 2006 17:45:25 -0000	3.95
>+++ content/base/src/nsContentList.cpp	8 Sep 2006 12:29:47 -0000
>@@ -116,16 +116,29 @@ nsBaseContentList::Item(PRUint32 aIndex,
> 
> void
> nsBaseContentList::AppendElement(nsIContent *aContent)
> {
>   mElements.AppendObject(aContent);
> }
> 
> void
>+nsBaseContentList::InsertElementAt(nsIContent* aContent, PRInt32 aIndex)
>+{
>+  mElements.InsertObjectAt(aContent, aIndex);
>+}
>+
>+nsIContent*
>+nsBaseContentList::ElementAt(PRInt32 aIndex) const
>+{
>+  NS_ASSERTION(0 <= aIndex && aIndex < mElements.Count(), "index out of range");
>+  return mElements.ObjectAt(aIndex);
>+}
>+
>+void
> nsBaseContentList::RemoveElement(nsIContent *aContent)
> {
>   mElements.RemoveObject(aContent);
> }
> 
> PRInt32
> nsBaseContentList::IndexOf(nsIContent *aContent, PRBool aDoFlush)
> {
>Index: content/base/src/nsContentList.h
>===================================================================
>RCS file: /cvsroot/mozilla/content/base/src/nsContentList.h,v
>retrieving revision 3.65
>diff -u -p -8 -r3.65 nsContentList.h
>--- content/base/src/nsContentList.h	25 Aug 2006 17:45:25 -0000	3.65
>+++ content/base/src/nsContentList.h	8 Sep 2006 12:29:48 -0000
>@@ -77,17 +77,35 @@ public:
>   virtual ~nsBaseContentList();
> 
>   NS_DECL_ISUPPORTS
> 
>   // nsIDOMNodeList
>   NS_DECL_NSIDOMNODELIST
> 
>   virtual void AppendElement(nsIContent *aContent);
>+
>+  /**
>+   * Insert the element at a given index, shifting the objects at
>+   * the given index and later to make space.
>+   * @param aContent Element to insert.
>+   * @param aIndex Index to insert the element at.
>+   */
>+  virtual void InsertElementAt(nsIContent* aContent, PRInt32 aIndex);
>+
>   virtual void RemoveElement(nsIContent *aContent);
>+
>+  /**
>+   * Get the element at a given index. This method does not
>+   * refcount for speed.
>+   * @param aIndex Index.
>+   * @return Element at a given index.
>+   */
>+  virtual nsIContent* ElementAt(PRInt32 aIndex) const;
>+
>   virtual PRInt32 IndexOf(nsIContent *aContent, PRBool aDoFlush);
>   virtual void Reset();
> 
>   static void Shutdown();
> 
> protected:
>   nsCOMArray<nsIContent> mElements;
> };
>Index: content/html/content/src/nsHTMLFormElement.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/html/content/src/nsHTMLFormElement.cpp,v
>retrieving revision 1.208
>diff -u -p -8 -r1.208 nsHTMLFormElement.cpp
>--- content/html/content/src/nsHTMLFormElement.cpp	5 Sep 2006 10:22:53 -0000	1.208
>+++ content/html/content/src/nsHTMLFormElement.cpp	8 Sep 2006 12:29:50 -0000
>@@ -2086,41 +2086,85 @@ nsFormControlList::AddElementToTable(nsI
>         return NS_OK;
>       }
> 
>       // Found an element, create a list, add the element to the list and put
>       // the list in the hash
>       nsBaseContentList *list = new nsBaseContentList();
>       NS_ENSURE_TRUE(list, NS_ERROR_OUT_OF_MEMORY);
> 
>-      list->AppendElement(content);
>+      // Determine the ordering between the new and old element.
>+      PRBool newFirst = PR_FALSE;
> 
>-      // Add the new child too
>-      list->AppendElement(newChild);
>+      NS_ASSERTION(content->GetParent(), "Item in list without parent");
>+
>+      // Can only perform the comparison if the new child has a parent.
>+      if (newChild->GetParent()) {
>+        newFirst = (nsLayoutUtils::CompareTreePosition(newChild, content) < 0);

Use nsContentUtils::PositionIsBefore

>+      // If the new child does not have a parent then it isn't in the form
>+      // yet and we can't determine where it should be ordered in the list.
>+      // Append the element to the end of the list if it does not exist
>+      // in the list already.
>+      if (!newChild->GetParent()) {
>+        if (list->IndexOf(newChild, PR_FALSE) < 0) {
>+          list->AppendElement(newChild);
>+        }
>+        return NS_OK;
>       }

How could this happen?

>+
>+      // Search backwards from the end of the list to determine the position
>+      // at which to insert the new element. Optimize for the case where the
>+      // new element is last by searching backwards.
>+      PRUint32 length;
>+      list->GetLength(&length);
>+      NS_ASSERTION(length > 1,
>+                   "List should have been converted back to a single element");
>+
>+      for (PRInt32 i = length - 1; i >= 0; --i) {

You should do a binary search here. Ideally break out the code from nsHTMLFormElement::AddElement into some utility function.

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Oh, but before you do the binary search it's probably worth checking if the new element should be positioned after the last element.

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Actually, you should be able to take advantage of the list in mElements. That already contains a sorted list so you should be able to do a search in there to figure out where to insert the new element.

Though I'm admittedly not 100% sure which will be fastest

Comment 17

[jpl24]

(In reply to comment #14)

Thanks for the review! Comments follow:

> 
> Use nsContentUtils::PositionIsBefore
Will do.
 
> >+      // If the new child does not have a parent then it isn't in the form
> >+      // yet and we can't determine where it should be ordered in the list.
> >+      // Append the element to the end of the list if it does not exist
> >+      // in the list already.
> >+      if (!newChild->GetParent()) {
> >+        if (list->IndexOf(newChild, PR_FALSE) < 0) {
> >+          list->AppendElement(newChild);
> >+        }
> >+        return NS_OK;
> >       }
> 
> How could this happen?

This is related Bug 353415, form controls don't have parents early enough. I'll wait to update this patch until that is resolved.

> You should do a binary search here. Ideally break out the code from
> nsHTMLFormElement::AddElement into some utility function.

I'll test a couple approaches out and see what is fastest.

Comment 18

[Johnny Stenback (:jst)]

Not a blocker, but we'd be happy to take a fix if one comes up in time.

Comment 19

[Brad]

This just split my head in two for a couple hours. Couldn't figure out why Firefox was killing my form values when all the other browsers including Chrome worked.

So this destroyed my app I have to rewrite a bunch of it now. I find this bug to be a big issue now that more and more DOM manipulation is becoming common.

What TRUELY happens:
Elements that are in the ORIGINAL SOURCE before dom manipulation appear in the list first. Then all DOM elements added there after show up.

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Bz: I thought that we had fixed this bug?

Comment 21

[Brad]

I just ran into it. FF2 and FF3 newest releases.

I thought it was the same bug. The only difference that there may be is that I am using .cloneNode(true)

<table>
<tr id=tr1>
<td>
<input name=input1 id=input1>
</td>
</tr>
</table>

document.form.elements[input1][0].id = input1

var newtr = tr1.cloneNode(true)
table.insertbefore(newtr, input1)

document.form.elements[input1][0].id = input1

Comment 22

[Boris Zbarsky [:bzbarsky]]

Jonas, what we fixed was storing form.elements in document order.  This one still isn't fixed; one of us should probably just update that patch and check it in (and the other review).  Got any preference for which you'd rather do?

Comment 23

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I can do it, but not until in a couple of weeks due to vacation.

Comment 24

[Cory Dodt]

I have confirmed this, see e.g. http://code.google.com/p/google-jstemplate/issues/detail?id=1 which uses google-jstemplate to clone an input several times.  Document order and form.name order do not match.

Comment 26

[David Zbarsky (:dzbarsky)]

Attachment: patch with tests

Comment 27

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 390270 [details] [diff] [review]
patch with tests

>+++ b/content/base/src/nsContentList.cpp
>+nsBaseContentList::Length()

Please just inline this in the header.

>+nsIContent*
>+nsBaseContentList::ElementAt(PRInt32 aIndex) const

Just use GetNodeAt() in the caller instead?

>+++ b/content/base/src/nsContentList.h

>+   * Insert the element at a given index, shifting the objects at
>+   * the given index and later to make space.
>+   * @param aContent Element to insert.

Document that aContent must not be null?  And assert it in the implementation?

>+  void InsertElementAt(nsIContent* aContent, PRInt32 aIndex);

Again, I'd just inline this here instead of putting it in the C++ file.

>+++ b/content/html/content/src/nsHTMLFormElement.cpp
>+      // Can only perform the comparison if the new child has a parent.
>+      if (newChild->GetParent()) {

I don't think you need that check.  See comment 17.

>+      if(nsContentUtils::PositionIsBefore(list->ElementAt(list->Length() - 1), newChild)) {
>+          list->AppendElement(newChild);
>+          return NS_OK;
>+      }
>+
>+      NS_ASSERTION(list->Length() > 1,
>+                   "List should have been converted back to a single element");

I'd assert that before you do the PositionIsBefore check above.

>+      //First is the first possible index, last is the last possible index

s/First/first/ and put a space before it.  And maybe s/index/insertion index/?

>+      while (last != first) 
>+        {

Please put the '{' on the same line as the |while|, like so:

  while (condition) {

>     }
>+    
>   }

Please don't add the random blank line.

The tests look good.

Comment 28

[David Zbarsky (:dzbarsky)]

Attachment: path adressing comments

Comment 29

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/e915a6e0b9e7