The default bug view has changed. See this FAQ.

inline message regression

RESOLVED FIXED

Status

Core Graveyard
XForms
RESOLVED FIXED
10 years ago
8 months ago

People

(Reporter: aaronr, Assigned: aaronr)

Tracking

({fixed1.8.0.12, fixed1.8.1.4})

1.8 Branch
x86
All
fixed1.8.0.12, fixed1.8.1.4

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

1.09 KB, application/xhtml+xml
Details
4.18 KB, patch
surkov
: review+
Details | Diff | Splinter Review
(Assignee)

Description

10 years ago
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.
(Assignee)

Comment 1

10 years ago
Created attachment 248321 [details]
testcase
(Assignee)

Comment 2

10 years ago
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.
(Assignee)

Comment 3

10 years ago
Created attachment 252705 [details] [diff] [review]
patch1

easy fix, just importing the result of nsXFormsMessageElement::CloneNode into the destination document before appending it.
Attachment #252705 - Flags: review?(Olli.Pettay)
(Assignee)

Updated

10 years ago
Attachment #252705 - Flags: review?(surkov.alexander)

Comment 4

10 years ago
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

10 years ago
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 :))
Attachment #252705 - Flags: review?(Olli.Pettay) → review-
(Assignee)

Comment 6

10 years ago
(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*.  :-)
(Assignee)

Comment 7

10 years ago
Created attachment 252792 [details] [diff] [review]
patch2

using smaug and surkov's approach
Attachment #252705 - Attachment is obsolete: true
Attachment #252705 - Flags: review?(surkov.alexander)
(Assignee)

Updated

10 years ago
Attachment #252792 - Flags: review?(Olli.Pettay)
(Assignee)

Updated

10 years ago
Attachment #252792 - Flags: review?(surkov.alexander)

Comment 8

10 years ago
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
Attachment #252792 - Flags: review?(Olli.Pettay) → review+
(Assignee)

Comment 9

10 years ago
Created attachment 252854 [details] [diff] [review]
patch3

renamed to ImportNode as per smaug's comment
Attachment #252792 - Attachment is obsolete: true
Attachment #252854 - Flags: review?(surkov.alexander)
Attachment #252792 - Flags: review?(surkov.alexander)

Updated

10 years ago
Attachment #252854 - Flags: review?(surkov.alexander) → review+

Updated

10 years ago
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Whiteboard: xf-to-branch
(Assignee)

Comment 10

10 years ago
checked into trunk by smaug.
(Assignee)

Comment 11

10 years ago
checked into 1.8 branch on 2007-04-12
checked into 1.8.0 branch on 2007-04-16
Keywords: fixed1.8.0.12, fixed1.8.1.4
Whiteboard: xf-to-branch
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.