our 1.1 insert code can't handle empty target nodesest

RESOLVED FIXED

Status

defect
RESOLVED FIXED
12 years ago
3 years ago

People

(Reporter: aaronr, Assigned: msterlin)

Tracking

({fixed1.8.1.8})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 3 obsolete attachments)

1.92 KB, application/xhtml+xml
Details
2.88 KB, application/xhtml+xml
Details
2.25 KB, application/xhtml+xml
Details
10.12 KB, patch
aaronr
: review+
smaug
: review+
Details | Diff | Splinter Review
(Reporter)

Description

12 years ago
Posted file 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.
(Assignee)

Updated

12 years ago
Status: NEW → ASSIGNED
(Reporter)

Comment 1

12 years ago
Posted file testcase2 (obsolete) —
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.
(Reporter)

Comment 2

12 years ago
Posted file testcase3
testcase from John on NG.
(Reporter)

Comment 3

12 years ago
Posted file testcase2
fixed the 'delete' trigger in testcase 2
Attachment #273187 - Attachment is obsolete: true
(Assignee)

Comment 4

12 years ago
Posted patch patch (obsolete) — Splinter Review
- 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.
Attachment #273209 - Flags: review?(aaronr)
(Reporter)

Comment 5

12 years ago
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
Attachment #273209 - Flags: review?(aaronr) → review+
(Assignee)

Comment 6

12 years ago
Posted patch patch2 (obsolete) — Splinter Review
Attachment #273209 - Attachment is obsolete: true
Attachment #273437 - Flags: review?(Olli.Pettay)

Updated

12 years ago
Attachment #273437 - Flags: review?(Olli.Pettay) → review+
(Assignee)

Comment 7

12 years ago
Posted patch final patchSplinter Review
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.
Attachment #273437 - Attachment is obsolete: true
(Reporter)

Updated

12 years ago
Attachment #273863 - Flags: review+
(Assignee)

Updated

12 years ago
Attachment #273863 - Flags: review?(Olli.Pettay)
(Reporter)

Updated

12 years ago
Duplicate of this bug: 389868

Updated

12 years ago
Attachment #273863 - Flags: review?(Olli.Pettay) → review+
(Reporter)

Comment 9

12 years ago
checked into trunk and 1.8 branch for msterlin
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Keywords: fixed1.8.1.7
Resolution: --- → FIXED
>+  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?
(Reporter)

Comment 11

12 years ago
(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.
(Reporter)

Updated

11 years ago
Duplicate of this bug: 413354
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.