Last Comment Bug 363516 - inline message regression
: inline message regression
Status: RESOLVED FIXED
: fixed1.8.0.12, fixed1.8.1.4
Product: Core
Classification: Components
Component: XForms (show other bugs)
: 1.8 Branch
: x86 All
: -- normal (vote)
: ---
Assigned To: aaronr
: Stephen Pride
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-12-11 16:50 PST by aaronr
Modified: 2007-04-23 16:01 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (1.09 KB, application/xhtml+xml)
2006-12-11 16:52 PST, aaronr
no flags Details
patch1 (1.32 KB, patch)
2007-01-24 16:47 PST, aaronr
bugs: review-
Details | Diff | Review
patch2 (4.17 KB, patch)
2007-01-25 10:36 PST, aaronr
bugs: review+
Details | Diff | Review
patch3 (4.18 KB, patch)
2007-01-25 17:46 PST, aaronr
surkov.alexander: review+
Details | Diff | Review

Description aaronr 2006-12-11 16:50:37 PST
xf:messages with inline text (instead of binding to a node or using an external resource) no longer work.  The dialog displays, but no message is shown.

I've debugged this down.  Inline xforms messages stopped working on 11/23/06 due to the checkin for bug 47903.  We currently clone the child nodes of the xf:message and append them into the message document.  We can't do that.  We need to import the nodes into the document first.
Comment 1 aaronr 2006-12-11 16:52:39 PST
Created attachment 248321 [details]
testcase
Comment 2 aaronr 2006-12-11 17:00:19 PST
ugh, no time now.  I'll get to it after the holidays.  If someone else wants to whip up a fix, please feel free to assign this bug to yourself.
Comment 3 aaronr 2007-01-24 16:47:14 PST
Created attachment 252705 [details] [diff] [review]
patch1

easy fix, just importing the result of nsXFormsMessageElement::CloneNode into the destination document before appending it.
Comment 4 alexander :surkov 2007-01-24 19:08:43 PST
Comment on attachment 252705 [details] [diff] [review]
patch1

>           CloneNode(child, getter_AddRefs(clone));
>-          if (clone)
>-            bodyEl->AppendChild(clone, getter_AddRefs(tmp));
>+          if (clone) {
>+            // since clone is generated from the original document, we need
>+            // to import it into the destination document before appending
>+            // it.
>+            nsCOMPtr<nsIDOMNode> importedNode;
>+            ddoc->ImportNode(clone, PR_TRUE, getter_AddRefs(importedNode));
>+            bodyEl->AppendChild(importedNode, getter_AddRefs(tmp));

I'd prefer to pass document into nsXFormsMessagee::CloneNode() instead of follow up import node.
Comment 5 Olli Pettay [:smaug] 2007-01-25 00:11:23 PST
Comment on attachment 252705 [details] [diff] [review]
patch1


>           CloneNode(child, getter_AddRefs(clone));
>-          if (clone)
>-            bodyEl->AppendChild(clone, getter_AddRefs(tmp));
>+          if (clone) {
>+            // since clone is generated from the original document, we need
>+            // to import it into the destination document before appending
>+            // it.
>+            nsCOMPtr<nsIDOMNode> importedNode;
>+            ddoc->ImportNode(clone, PR_TRUE, getter_AddRefs(importedNode));

This is quite heavy because first CloneNode creates new nodes which are then
again cloned for the new document when ImportNode is called.
http://www.w3.org/TR/2004/REC-DOM-Level-3-Core-20040407/core.html#Core-Document-importNode
"Imports a node from another document to this document, without altering or removing the source node from the original document; this method creates a new copy of the source node."

I guess nsXFormsMessageElement::CloneNode could be modified to use ImportNode, not
CloneNode (and ofc then it should be called nsXFormsMessageElement::ImportNode :))
Comment 6 aaronr 2007-01-25 10:03:29 PST
(In reply to comment #5)
> (From update of attachment 252705 [details] [diff] [review])
> 
> >           CloneNode(child, getter_AddRefs(clone));
> >-          if (clone)
> >-            bodyEl->AppendChild(clone, getter_AddRefs(tmp));
> >+          if (clone) {
> >+            // since clone is generated from the original document, we need
> >+            // to import it into the destination document before appending
> >+            // it.
> >+            nsCOMPtr<nsIDOMNode> importedNode;
> >+            ddoc->ImportNode(clone, PR_TRUE, getter_AddRefs(importedNode));
> 
> This is quite heavy because first CloneNode creates new nodes which are then
> again cloned for the new document when ImportNode is called.
> http://www.w3.org/TR/2004/REC-DOM-Level-3-Core-20040407/core.html#Core-Document-importNode
> "Imports a node from another document to this document, without altering or
> removing the source node from the original document; this method creates a new
> copy of the source node."
> 
> I guess nsXFormsMessageElement::CloneNode could be modified to use ImportNode,
> not
> CloneNode (and ofc then it should be called nsXFormsMessageElement::ImportNode
> :))
> 


I agree it is heavy, but it is a convenient, 3 line change.  I guess I'll go the long way around.  *sigh*.  :-)
Comment 7 aaronr 2007-01-25 10:36:22 PST
Created attachment 252792 [details] [diff] [review]
patch2

using smaug and surkov's approach
Comment 8 Olli Pettay [:smaug] 2007-01-25 10:50:29 PST
Comment on attachment 252792 [details] [diff] [review]
patch2


>+++ nsXFormsMessageElement.cpp	25 Jan 2007 18:35:22 -0000
...
>-  void CloneNode(nsIDOMNode* aSrc, nsIDOMNode** aTarget);
>+  void CloneNode(nsIDOMNode* aSrc, nsIDOMDocument* aDestDoc,
>+                 nsIDOMNode** aTarget);

rename this to ImportNode, r=me
Comment 9 aaronr 2007-01-25 17:46:46 PST
Created attachment 252854 [details] [diff] [review]
patch3

renamed to ImportNode as per smaug's comment
Comment 10 aaronr 2007-01-26 12:17:22 PST
checked into trunk by smaug.
Comment 11 aaronr 2007-04-23 16:01:04 PDT
checked into 1.8 branch on 2007-04-12
checked into 1.8.0 branch on 2007-04-16

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