Last Comment Bug 389016 - our 1.1 insert code can't handle empty target nodesest
: our 1.1 insert code can't handle empty target nodesest
Status: RESOLVED FIXED
: fixed1.8.1.8
Product: Core Graveyard
Classification: Graveyard
Component: XForms (show other bugs)
: Trunk
: x86 All
: -- normal with 1 vote (vote)
: ---
Assigned To: Merle Sterling
:
:
Mentors:
: 389868 413354 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-07-20 15:58 PDT by aaronr
Modified: 2016-07-15 14:46 PDT (History)
4 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
testcase (1.92 KB, application/xhtml+xml)
2007-07-20 15:58 PDT, aaronr
no flags Details
testcase2 (2.27 KB, application/xhtml+xml)
2007-07-20 16:49 PDT, aaronr
no flags Details
testcase3 (2.88 KB, application/xhtml+xml)
2007-07-20 16:57 PDT, aaronr
no flags Details
testcase2 (2.25 KB, application/xhtml+xml)
2007-07-20 17:08 PDT, aaronr
no flags Details
patch (10.12 KB, patch)
2007-07-20 20:21 PDT, Merle Sterling
aaronr: review+
Details | Diff | Splinter Review
patch2 (10.09 KB, patch)
2007-07-23 13:05 PDT, Merle Sterling
bugs: review+
Details | Diff | Splinter Review
final patch (10.12 KB, patch)
2007-07-25 15:42 PDT, Merle Sterling
aaronr: review+
bugs: review+
Details | Diff | Splinter Review

Description aaronr 2007-07-20 15:58:36 PDT
Created attachment 273173 [details]
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 2007-07-20 16:49:14 PDT
Created attachment 273187 [details]
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 2007-07-20 16:57:00 PDT
Created attachment 273189 [details]
testcase3

testcase from John on NG.
Comment 3 aaronr 2007-07-20 17:08:05 PDT
Created attachment 273194 [details]
testcase2

fixed the 'delete' trigger in testcase 2
Comment 4 Merle Sterling 2007-07-20 20:21:53 PDT
Created attachment 273209 [details] [diff] [review]
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 2007-07-23 10:49:10 PDT
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 2007-07-23 13:05:22 PDT
Created attachment 273437 [details] [diff] [review]
patch2
Comment 7 Merle Sterling 2007-07-25 15:42:55 PDT
Created attachment 273863 [details] [diff] [review]
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 8 aaronr 2007-07-27 11:51:37 PDT
*** Bug 389868 has been marked as a duplicate of this bug. ***
Comment 9 aaronr 2007-08-03 17:32:01 PDT
checked into trunk and 1.8 branch for msterlin
Comment 10 Boris Zbarsky [:bz] (still a bit busy) 2007-08-08 18:20:01 PDT
>+  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 2007-08-09 12:31:20 PDT
(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.
Comment 12 aaronr 2008-01-21 10:05:03 PST
*** Bug 413354 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.