Closed Bug 204784 Opened 20 years ago Closed 18 years ago

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

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: collins_p, Assigned: pkwarren)

References

()

Details

(Keywords: helpwanted, Whiteboard: [good first bug])

Attachments

(5 files)

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.
Attached file Example of bug
Whiteboard: DUPEME
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?
no longer seeing this as of 1.4 on win2k. They appear in order now.
ack...nevermind, it is still happening, got my wires crossed. Sorry for the spam.
on WinNT 4.0 SP6, Mozilla 2003080204 the bug occurs. 
*** Bug 219901 has been marked as a duplicate of this bug. ***
This is really a nasty bug... actually I would say it's critical.

Still exists in 20030921 nightlies
*** Bug 219664 has been marked as a duplicate of this 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.
Attached file
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.
Attachment #141357 - Attachment description: More example of the bug →
Attachment #141357 - Attachment filename: bugpage1.htm
*** Bug 239229 has been marked as a duplicate of this bug. ***
*** Bug 203886 has been marked as a duplicate of this bug. ***
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?
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Linux → All
Hardware: PC → All
Whiteboard: DUPEME
nsHTMLFormElement::AddElement is what needs fixing.
Keywords: helpwanted
Whiteboard: [good first bug]
Attached file Yet another test case
Notice this bug breaks the form submission - the order of the field data being
submitted.
Blocks: 244694
No longer blocks: 244694
*** Bug 244694 has been marked as a duplicate of this bug. ***
Summary: form.elements[] not order perserving when using javascript DOM interface (example included) → form.elements[] not order preserving when using javascript DOM interface (example included)
Blocks: 182457
*** Bug 248176 has been marked as a duplicate of this bug. ***
Assignee: general → gerlofs
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.
Assignee: gerlofs → general
QA Contact: desale → ian
Attached patch Patch v1Splinter Review
Assignee: general → pkwarren
Status: NEW → ASSIGNED
Comment on attachment 163091 [details] [diff] [review]
Patch v1

Boris: Do you mean something like this?
Attachment #163091 - Flags: review?(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 on attachment 163091 [details] [diff] [review]
Patch v1

r+sr=bzbarsky.	Looks good!  I assume it fixes all the testcases in this bug?
Attachment #163091 - Flags: superreview+
Attachment #163091 - Flags: review?(bzbarsky)
Attachment #163091 - Flags: review+
Yes - all of the testcases listed in this bug are fixed by this patch.
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
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
+  // This should not occur - we should be able to QI to nsIContent

add a NS_NOTREACHED here?
Oh, yeah.  I was going to put that in my review comments and forgot...
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.
Oops, that doesn't actually help.  The assertion is from AddElement(), not
RemoveElement().
So the simple thing to do would be to simply append when the new nsIFormControl
has no parent...
Bug 267511 filed for the problem described in comment 27.
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
> 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.
Flags: blocking1.8a6?
Flags: blocking1.7.6?
Flags: blocking1.8a6?
*** Bug 273835 has been marked as a duplicate of this bug. ***
closing down 1.7.6 to try it shipped soon -minus
Flags: blocking1.7.6? → blocking1.7.6-
*** Bug 281044 has been marked as a duplicate of this bug. ***
*** Bug 298912 has been marked as a duplicate of this bug. ***
*** Bug 299316 has been marked as a duplicate of this bug. ***
Component: DOM: HTML → DOM: Core & HTML
QA Contact: ian → general
You need to log in before you can comment on or make changes to this bug.