389016 - our 1.1 insert code can't handle empty target nodesest

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[aaronr]

Attachment: testcase

Found by a user on the newsgroup and SteveS, our 1.1 insert doesn't correctly handle the case where the nodeset that we are inserting into is empty.

Testcase I'm attaching comes from John Boyer's blog.

Comment 1

[aaronr]

Attachment: testcase2

This testcase is similar to the first testcase but with attributes.  Works on Orbeon, but not formsPlayer/Sidewinder.  But I don't think that there is anything wrong with it.

Comment 2

[aaronr]

Attachment: testcase3

testcase from John on NG.

Comment 3

[aaronr]

Attachment: testcase2

fixed the 'delete' trigger in testcase 2

Comment 4

[Merle Sterling]

Attachment: patch

- No longer restricting insert of an element to another element. Now elements can be inserted to any non-attribute node. Fixes testcase 1, 2 and 3.
- If the insert location node is empty, the inserted node will become the first child of the location node. Fixes the first test of testcase 3.
- We were unable to delete an attribute node but now check if the node to be deleted is an attribute node. If so, gets the owner element and removes the attribute node. Fixes the delete in testcase 2.

Comment 5

[aaronr]

Comment on attachment 273209 [details] [diff] [review]
patch

>Index: nsXFormsInsertDeleteElement.cpp
>===================================================================

>@@ -652,52 +684,54 @@ nsXFormsInsertDeleteElement::InsertNode(

>+    // New node will be inserted either before or after targetNode.
>+    nsCOMPtr<nsIDOMNode> targetNode;
>+    targetNode = aTargetNode;

nit: assign targetNode to aTargetNode when you declare it on the previous line.

with that, r=me

Comment 6

[Merle Sterling]

Attachment: patch2

Comment 7

[Merle Sterling]

Attachment: final patch

Delete attribute case was not incrementing the deleteIndex for the while loop so we loop more than necessary and get ns_ensure_state warnings when the second pass thru the loop tried to get the owner element for the attribute node that was deleted in the first iteration of the loop.

Comment 9

[aaronr]

checked into trunk and 1.8 branch for msterlin

Comment 10

[Boris Zbarsky [:bzbarsky]]

>+  enum Location {
>+    eLocation_After,
>+    eLocation_Before,
>+    eLocation_FirstChild,
>+  };

The trailing comma there is a compile error with gcc, for what it's worth.  Please fix?

Comment 11

[aaronr]

(In reply to comment #10)
> >+  enum Location {
> >+    eLocation_After,
> >+    eLocation_Before,
> >+    eLocation_FirstChild,
> >+  };
> 
> The trailing comma there is a compile error with gcc, for what it's worth. 
> Please fix?
> 

Thanks for catching it.  Will fix.