363516 - inline message regression

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[aaronr]

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]

Attachment: testcase

Comment 2

[aaronr]

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]

Attachment: patch1

easy fix, just importing the result of nsXFormsMessageElement::CloneNode into the destination document before appending it.

Comment 4

[alexander :surkov (:asurkov)]

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][[e-mail]]]

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]

(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]

Attachment: patch2

using smaug and surkov's approach

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

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]

Attachment: patch3

renamed to ImportNode as per smaug's comment

Comment 10

[aaronr]

checked into trunk by smaug.

Comment 11

[aaronr]

checked into 1.8 branch on 2007-04-12
checked into 1.8.0 branch on 2007-04-16