Closed
Bug 98487
Opened 23 years ago
Closed 23 years ago
removeChild of form's child does not clear form pointer of descendant controls
Categories
(Core :: DOM: Core & HTML, defect, P2)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla0.9.7
People
(Reporter: wimroffel, Assigned: bzbarsky)
References
Details
Attachments
(2 files, 4 obsolete files)
|
(deleted),
text/html
|
Details | |
|
(deleted),
patch
|
jst
:
review+
jst
:
superreview+
|
Details | Diff | Splinter Review |
If you add and remove inputs to a form and these inputs are packed in a <div>
then it will leave a phantom input field. This field is not displayed but it is
part of the forms elements array[]. You can assign values to it but if you try
to retrieve them it will return an empty string.
The following code demonstrates the bug. It contains a "run demo" button to get
a fast impression.
Some notes:
- this happens only when the inputs are part of a div
- if you have more then one field in the div one may remember its input
- if you have more then one field in the div you will have the same number of
ghostfields
- it does not make a difference whether you remove one or several children
right after each other. You get only one set of ghostfields.
- this code is surrounded by a lot of instability. During testing I was
regularly thrown back to my homepage. Sometimes inserting an alert repaired this.
<Head>
<SCRIPT Language=JAVASCRIPT>
var fieldcount=0;
function createfield()
{
var field1Elt = document.createElement("input");
field1Elt.size = 20;
var divElt = document.createElement("div");
divElt.appendChild(field1Elt);
divElt.appendChild(document.createElement("br"));
document.MainForm.appendChild(divElt);
fieldcount++;
}
function removefield()
{ if(fieldcount==0) return;
MainFormElt = document.getElementById("MainFormId");
endchild = MainFormElt.lastChild;
MainFormElt.removeChild(endchild);
fieldcount--;
}
function fillfields()
{ for(j=1; j<=fieldcount; j++)
{ temp = "field"+j;
document.MainForm.elements[j+3].value = temp;
}
}
function rundemo()
{ for(i=1; i<=3; i++)
createfield();
fillfields();
alert("This is how it should look: each field filled with its name");
removefield();
createfield();
createfield();
fillfields();
alert("Note that 'field3' is now missing. It is assigned to a ghostelement
that remained after we removed one field and added two others.");
}
</SCRIPT>
</head>
<body>
<form name=MainForm id=MainFormId>
<input type=button onclick="rundemo()" value="Run Demo"><br>
<input type=button onclick="createfield()" value="Create Field"><br>
<input type=button onclick="removefield()" value="Remove Field"><br>
<input type=button onclick="fillfields()" value="Fill Fields"><br>
</form>
</body>
|
||
Comment 1•23 years ago
|
||
|
Assignee | |
Comment 2•23 years ago
|
||
alright... here is the problem. removeChild() ends up calling
nsGenericHTMLContainerElement::RemoveChildAt()
This calls oldKid->SetParent(nsnull);
Here things get messy. If the child being removed is a form element, all is
good. The SetParent that gets called then is
nsGenericHTMLLeafFormElement::SetParent() which removes the form control from
the form's list of controls.
In the div case, however, we call nsGenericElement::SetParent() which just sets
the parent for the div and does not look at the contents of the div for form
controls...
So, what needs to happen:
We need something like nsGenericHTMLContainerElement::ClearForm() which will
get called and do the right thing. That is, it needs to pass on down to all
it's children that are containers calls to ClearForm() and all the children that
are form controls should get SetForm(nsnull) called on them.
Oh, and I'm seeing this with current linux debug builds, of course (where else
would I get all this juicy data?)
Status: UNCONFIRMED → NEW
Component: DOM Other → DOM Core
Ever confirmed: true
QA Contact: gerardok → stummala
|
|
Assignee | |
Comment 3•23 years ago
|
||
Alright. Throught the wonders of nsIContentIterator, this has a fairly simple
solution. Attaching patch and taking bug.
Assignee: jst → bzbarsky
OS: Windows 98 → All
Hardware: PC → All
|
|
Assignee | |
Comment 4•23 years ago
|
||
|
|
Assignee | |
Comment 5•23 years ago
|
||
reviews?
|
|
Assignee | |
Comment 6•23 years ago
|
||
jst says we call SetParent(nsnull) on every element on tear-down. That would
mean that this patch absolutely _kills_ performance. Thoughts:
1) Only iterate if we have a form as an ancestor.
2) Have a bool arg to SetParent() that's true only in teardown and is false
normally (this would be combined with #1 anyway).
3) Come up with something clever and more complicated to solve this problem
without iterating over all the descendants.
|
|
Assignee | |
Comment 7•23 years ago
|
||
Comment on attachment 48493 [details] [diff] [review]
Patch
We need to do #1 anyway for correctness reasons. Consider removing a <div> that contains a <form> and all its children....
Attachment #48493 -
Flags: needs-work+
|
|
Assignee | |
Comment 8•23 years ago
|
||
|
|
Assignee | |
Updated•23 years ago
|
Attachment #48493 -
Attachment is obsolete: true
Attachment #48493 -
Flags: needs-work+
|
|
Assignee | |
Comment 9•23 years ago
|
||
|
|
Assignee | |
Updated•23 years ago
|
Attachment #48544 -
Attachment is obsolete: true
|
|
Assignee | |
Comment 10•23 years ago
|
||
Comment on attachment 48550 [details] [diff] [review]
Last patch still broke if a parent of the form was removed... This one should be better.
Oh, fun. With that last patch I can't submit comments to bugzilla. "Product" is not defined. I added a breakpoint in nsGenericHTMLContainerFormElement::SetDocument() and got this stack:
#0 nsGenericHTMLContainerFormElement::SetDocument (this=0x887fbf8, aDocument=0x0,
aDeep=1, aCompileEventHandlers=1) at nsGenericHTMLElement.cpp:4001
#1 0x4133755b in nsGenericElement::SetDocumentInChildrenOf (aContent=0x88852e0,
aDocument=0x0, aCompileEventHandlers=1) at nsGenericElement.cpp:1477
#2 0x413377d6 in nsGenericElement::SetDocument (this=0x88852e0, aDocument=0x0, aDeep=1,
aCompileEventHandlers=1) at nsGenericElement.cpp:1515
#3 0x411223c4 in nsGenericHTMLElement::SetDocument (this=0x88852e0, aDocument=0x0,
aDeep=1, aCompileEventHandlers=1) at nsGenericHTMLElement.cpp:1158
#4 0x4133755b in nsGenericElement::SetDocumentInChildrenOf (aContent=0x87d3088,
aDocument=0x0, aCompileEventHandlers=1) at nsGenericElement.cpp:1477
#5 0x413377d6 in nsGenericElement::SetDocument (this=0x87d3088, aDocument=0x0, aDeep=1,
aCompileEventHandlers=1) at nsGenericElement.cpp:1515
#6 0x411223c4 in nsGenericHTMLElement::SetDocument (this=0x87d3088, aDocument=0x0,
aDeep=1, aCompileEventHandlers=1) at nsGenericHTMLElement.cpp:1158
#7 0x4133755b in nsGenericElement::SetDocumentInChildrenOf (aContent=0x8900680,
aDocument=0x0, aCompileEventHandlers=1) at nsGenericElement.cpp:1477
#8 0x413377d6 in nsGenericElement::SetDocument (this=0x8900680, aDocument=0x0, aDeep=1,
aCompileEventHandlers=1) at nsGenericElement.cpp:1515
#9 0x411223c4 in nsGenericHTMLElement::SetDocument (this=0x8900680, aDocument=0x0,
aDeep=1, aCompileEventHandlers=1) at nsGenericHTMLElement.cpp:1158
#10 0x4133755b in nsGenericElement::SetDocumentInChildrenOf (aContent=0x87fafa0,
aDocument=0x0, aCompileEventHandlers=1) at nsGenericElement.cpp:1477
#11 0x413377d6 in nsGenericElement::SetDocument (this=0x87fafa0, aDocument=0x0, aDeep=1,
aCompileEventHandlers=1) at nsGenericElement.cpp:1515
#12 0x411223c4 in nsGenericHTMLElement::SetDocument (this=0x87fafa0, aDocument=0x0,
aDeep=1, aCompileEventHandlers=1) at nsGenericHTMLElement.cpp:1158
#13 0x4112b2b8 in nsGenericHTMLContainerElement::RemoveChildAt (this=0x889eb38,
aIndex=0, aNotify=1) at nsGenericHTMLElement.cpp:3861
#14 0x4119fb66 in SinkContext::DemoteContainer (this=0x87074f8, aNode=@0xbfffef38)
at nsHTMLContentSink.cpp:1800
#15 0x411a52ad in HTMLContentSink::CloseForm (this=0x872ce00, aNode=@0xbfffef38)
at nsHTMLContentSink.cpp:3259
#16 0x40c0e3a7 in CNavDTD::CloseForm (this=0x88eb338, aNode=0xbfffef38)
at CNavDTD.cpp:3197
#17 0x40c0ec3e in CNavDTD::CloseContainer (this=0x88eb338, aNode=0xbfffef38,
aTarget=eHTMLTag_form, aClosedByStartTag=0) at CNavDTD.cpp:3491
#18 0x40c0bbb2 in CNavDTD::HandleEndToken (this=0x88eb338, aToken=0x8979708)
at CNavDTD.cpp:1896
Worse yet, when the element is readded to the document it's not added as a child of the form, so it can't find the form... As a result it does not get submitted properly.
Basically, DemoteContainer() removes a parent of the <select> from the doc and reinserts it outside the form. In the process, it keeps the form element (if any) of the element it's demoting but not of that element's children.
This looks like it may take some work....
Attachment #48550 -
Flags: needs-work+
|
|
Assignee | |
Comment 11•23 years ago
|
||
Great. From comments before DemoteContainer:
// This method is called when a container is determined to be
// non well-formed in the source content. Currently this can only
// happen for forms, since the parser doesn't do fixup of forms.
// The method makes the container a leaf and moves all the container's
// children up a level to the container's parent.
So this method will take a malformed form and move all its children to its
parent. It'll restore mForm for children of the form that are form controls but
any form controls which are indirect descendants of the form get no special
treatment. Needless to say with my patch this breaks the form controls. In
fact, any approach to fixing this should involve modifications to
DemoteContainer().
I'm getting tempted to add a aDemoting arg to SetDocument and have it be false
by default. If it's true (which would only happen for calls from
DemoteContainer) we don't forget our form.
|
|
Assignee | |
Comment 12•23 years ago
|
||
|
|
Assignee | |
Comment 13•23 years ago
|
||
while that last patch works, it seems very kludgey. A better method is probably
needed...
|
|
Assignee | |
Comment 14•23 years ago
|
||
resummarizing to reflect what's really going on
Summary: appendChild/removeChild of form inputs with <div> leaves ghostfield → removeChild of form's child does not clear form pointer of descendant controls
|
|
Assignee | |
Updated•23 years ago
|
Priority: -- → P3
Target Milestone: --- → mozilla1.0
|
|
Assignee | |
Comment 15•23 years ago
|
||
*** Bug 111502 has been marked as a duplicate of this bug. ***
|
|
Assignee | |
Comment 16•23 years ago
|
||
*** Bug 111502 has been marked as a duplicate of this bug. ***
|
|
Assignee | |
Comment 17•23 years ago
|
||
Attachment #48550 -
Attachment is obsolete: true
Attachment #48588 -
Attachment is obsolete: true
|
|
Assignee | |
Comment 18•23 years ago
|
||
jst, jkeiser, would you review? That last patch makes us pass the testcase in
this bug, I can submit to bugzilla fine, and it does not regress bug 109854. :)
Priority: P3 → P2
Target Milestone: mozilla1.0 → mozilla0.9.7
|
||
Comment 19•23 years ago
|
||
Comment on attachment 59582 [details] [diff] [review]
Patch to really fix this
I'm a little queasy about using NS_ENSURE_SUCCESS() instead of if
(formContent). Both NS_ENSURE_SUCCESS() checks seem redundant to me, if you
add if (formContent) ... functionally it looks good though.
|
|
Assignee | |
Comment 20•23 years ago
|
||
good point. The first NS_ENSURE_SUCCESS should just be an |if (formContent)|.
Changed.
The second one is _not_ equivalent to the check for |doc|. It's rather a check
that the doc value is null because the form is not in the document as opposed to
being null because some error occured.
|
|
||
Comment 21•23 years ago
|
||
Sounds good. r=jkeiser for such a beast then :)
|
||
Comment 22•23 years ago
|
||
Comment on attachment 59582 [details] [diff] [review]
Patch to really fix this
sr=jst
Attachment #59582 -
Flags: superreview+
Attachment #59582 -
Flags: review+
|
|
Assignee | |
Comment 23•23 years ago
|
||
checked in
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Component: DOM: Core → DOM: Core & HTML
QA Contact: stummala → general
You need to log in
before you can comment on or make changes to this bug.
Description
•