204784 - form.elements[] not order preserving when using javascript DOM interface (example included)

Status: RESOLVED FIXED

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

Description

[Philip Collins]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.3) Gecko/20030312
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.3) Gecko/20030312

The test page should show 'xyz' not 'xzy'. Basically the javascript code on this
page removes the input element named 'y' and replaces it in the same spot with
another named 'y' and the 'y' gets pushed to the end of the array when it still
is in the middle of the DOM tree.

Reproducible: Always

Steps to Reproduce:
1.View the test page
2.Notice that the input fields say 'xyz'
3.Notice that mozilla says the order is 'xzy'

Actual Results:  
You notice that the form.elements array is out of order.

Expected Results:  
It should have printed 'xyz'.

This works in Internet Explorer.

Comment 1

[Philip Collins]

Attachment: Example of bug

Comment 2

[Philip Collins]

When I say "This works in Internet Explorer" I am not implying that IE is better
or something. It would be nice if the elements array were consistent across
browsers but the specification may not say that the elements of the array
maintain its order when mucking with the DOM. I can't find evidence either way.
What do other people think?

Comment 3

[Jon Hall]

no longer seeing this as of 1.4 on win2k. They appear in order now.

Comment 4

[Jon Hall]

ack...nevermind, it is still happening, got my wires crossed. Sorry for the spam.

Comment 5

[Kenan]

on WinNT 4.0 SP6, Mozilla 2003080204 the bug occurs. 

Comment 6

[Hixie (not reading bugmail)]

*** Bug 219901 has been marked as a duplicate of this bug. ***

Comment 7

[Donny]

This is really a nasty bug... actually I would say it's critical.

Still exists in 20030921 nightlies

Comment 8

[Boris Zbarsky [:bzbarsky]]

*** Bug 219664 has been marked as a duplicate of this bug. ***

Comment 9

[Ed Lai]

Attachment: More example of the bug

I do not understand why the status of this bug is "unconfirmed". So I am
posting another example to illustrate the bug. In the example, the order of the
elements are first shown. Then DOM is used to add another element in the
middle, and then we show the order of elements again. The newly added element
appears at the end of the elements array even though its position in the
browser is in the middle.

Comment 10

[Ed Lai]

I do not understand why the status of this bug is "unconfirmed". So I am
posting another example to illustrate the bug. In the example, the order of the
elements are first shown. Then DOM is used to add another element in the
middle, and then we show the order of elements again. The newly added element
appears at the end of the elements array even though its position in the
browser is in the middle.

Comment 11

[Boris Zbarsky [:bzbarsky]]

*** Bug 239229 has been marked as a duplicate of this bug. ***

Comment 12

[Boris Zbarsky [:bzbarsky]]

*** Bug 203886 has been marked as a duplicate of this bug. ***

Comment 13

[Boris Zbarsky [:bzbarsky]]

The status of this bug is unconfirmed because I'm pretty sure it's a duplicate,
but since I can't find the original....

Adding to form.elements should probably do a binary search to find the location
instead of just appending.... perhaps using comparetreeposition?

Comment 14

[Boris Zbarsky [:bzbarsky]]

nsHTMLFormElement::AddElement is what needs fixing.

Comment 15

[Stanimir Stamenkov]

Attachment: Yet another test case

Notice this bug breaks the form submission - the order of the field data being
submitted.

Comment 16

[David D. Kilzer (ddk)]

*** Bug 244694 has been marked as a duplicate of this bug. ***

Comment 17

[PikeUK]

*** Bug 248176 has been marked as a duplicate of this bug. ***

Comment 18

[Maureen Gerlofs (IBM)]

nsHTMLFormElement::AddElement method in file
\mozilla\content\html\content\src\nsHTMLFormElement.cpp is set up to append to
window.document.forms[i].elements.  I have been pull off the mozilla team, so I
am reassining this back to the pool.

Comment 19

[Philip K. Warren]

Attachment: Patch v1

Comment 20

[Philip K. Warren]

Comment on attachment 163091 [details] [diff] [review]
Patch v1

Boris: Do you mean something like this?

Comment 21

[Boris Zbarsky [:bzbarsky]]

That looks like the right general approach yes.  It'll take me a few days to get
to doing a real review, though.... I'm pretty swamped right now.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 163091 [details] [diff] [review]
Patch v1

r+sr=bzbarsky.	Looks good!  I assume it fixes all the testcases in this bug?

Comment 23

[Philip K. Warren]

Yes - all of the testcases listed in this bug are fixed by this patch.

Comment 24

[Philip K. Warren]

Fix checked in.

Checking in nsHTMLFormElement.cpp;
/cvsroot/mozilla/content/html/content/src/nsHTMLFormElement.cpp,v  <-- 
nsHTMLFormElement.cpp
new revision: 1.175; previous revision: 1.174
done

Comment 25

[Christian :Biesinger (don't email me, ping me on IRC)]

+  // This should not occur - we should be able to QI to nsIContent

add a NS_NOTREACHED here?

Comment 26

[Boris Zbarsky [:bzbarsky]]

Oh, yeah.  I was going to put that in my review comments and forgot...

Comment 27

[Brian Ryner (not reading)]

This checkin causes a constant stream of assertions for me when loading pages
with form controls.  The assertion is "no common ancestor at all???" in
nsLayoutUtils::CompareTreePosition.

The reason we hit this seems to be that nsHTMLContentSink's MakeContentObject
calls SetForm() on the newly-created control (with aRemoveFromForm == true). 
nsGenericHTMLFormElement::SetForm() then calls
nsHTMLFormElement::RemoveElement() for the newly-created element, which triggers
this assertion because the form control isn't in its parent form element's DOM
tree yet.

Seems like an easy fix/optimization would be to pass in aRemoveFromForm =
PR_FALSE when calling it from this location in the content sink, since there
will never be an existing form to remove the element from.

Comment 28

[Brian Ryner (not reading)]

Oops, that doesn't actually help.  The assertion is from AddElement(), not
RemoveElement().

Comment 29

[Boris Zbarsky [:bzbarsky]]

So the simple thing to do would be to simply append when the new nsIFormControl
has no parent...

Comment 30

[Philip K. Warren]

Bug 267511 filed for the problem described in comment 27.

Comment 31

[Hungerburg]

the patch is in todays firefox nightly trunc build. I experience sudden crashes,
though they might not be related (firefox crashed also while idling). the patch
is not in todays firefox RC2 - does that mean, firefox 1.0 will ship with the
not order-preserving dom manipulations?

thanks, peter

Comment 32

[Boris Zbarsky [:bzbarsky]]

> does that mean, firefox 1.0 will ship with the not order-preserving dom
> manipulations?

That's correct.  It's months too late for changes of this sort to go on the
branch, in my opinion.

Comment 33

[Boris Zbarsky [:bzbarsky]]

*** Bug 273835 has been marked as a duplicate of this bug. ***

Comment 34

[chris hofmann]

closing down 1.7.6 to try it shipped soon -minus

Comment 35

[Boris Zbarsky [:bzbarsky]]

*** Bug 281044 has been marked as a duplicate of this bug. ***

Comment 36

[Erik Fabert]

*** Bug 298912 has been marked as a duplicate of this bug. ***

Comment 37

[Keith]

*** Bug 299316 has been marked as a duplicate of this bug. ***