357652 - xf:textarea appears to have a 4096 characters size restriction

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[jan-wijbrand kolman]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.7) Gecko/20060921 Ubuntu/dapper-security Firefox/1.5.0.7
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.7) Gecko/20060921 Ubuntu/dapper-security Firefox/1.5.0.7

An xf:textarea form element only displays the first 4096 characters of the referenced element in the instance data.

Reproducible: Always

Steps to Reproduce:
1. create a xf:instance, where one of the instance data elements contain over 4096 characters worth of data.
2. create a xf:textarea referencing this instance data element

Actual Results:  
The xf:textarea displays only the first 4096 characters

Expected Results:  
The xf:textarea should display all data contained in the referenced instance data element.

Comment 1

[jan-wijbrand kolman]

Attachment: A simple text case demonstrating the described problem.

This attachement is a simple text case demonstrating the described problem.

Comment 2

[jan-wijbrand kolman]

If you need additional information or details, I'd be more than happy to provide it!

Comment 3

[Alex Vincent [:WeirdAl]]

Hm.  There's an open bug against core DOM, where text nodes are only 4096 characters in size during parsing.  Might this be fallout from that?

Normalizing the document usually works around this.

Comment 4

[alexander :surkov (:asurkov)]

I talked with Alex, he gave me bug number, it's bug 194231. At least it's very similar the reason is there.

Comment 5

[aaronr]

Doron mentioned this possibility quite a while ago...that it is legitimately possible to have two textnodes under a bound element.  The fix might be as simple as changing nsXFormsControlStub::GetValue to handle this possibility.

Comment 6

[alexander :surkov (:asurkov)]

(In reply to comment #5)
> Doron mentioned this possibility quite a while ago...that it is legitimately
> possible to have two textnodes under a bound element.  The fix might be as
> simple as changing nsXFormsControlStub::GetValue to handle this possibility.
> 

How can I put create text nodes into one and the same element by markup?

Comment 7

[aaronr]

Doron's point is that under the covers mozilla might create multiple textnodes to support this extra large amount of data (since it seems to have a 4096 limit in mozilla).  If mozilla does this, then we need to be able to handle it.

Comment 8

[Leigh L. Klotz, Jr.]

I recently noticed xf:output has the same limitation, but presumably you already know that.

Comment 9

[aaronr]

(In reply to comment #8)
> I recently noticed xf:output has the same limitation, but presumably you
> already know that.
> 

I personally didn't until you let me know, but if these issues are all because of the limitation on DOM textnodes, then I'd say that all of our xforms controls will have this limitation.  We currently only look at the first textnode when getting the values from the instance document.

Comment 10

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 11

[aaronr]

Comment on attachment 243266 [details] [diff] [review]
patch

>Index: extensions/xforms/nsXFormsMDGEngine.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsMDGEngine.cpp,v
>retrieving revision 1.23
>diff -u -8 -p -r1.23 nsXFormsMDGEngine.cpp
>--- extensions/xforms/nsXFormsMDGEngine.cpp	13 Jul 2006 14:21:52 -0000	1.23
>+++ extensions/xforms/nsXFormsMDGEngine.cpp	23 Oct 2006 23:20:22 -0000
>@@ -669,39 +669,60 @@ nsXFormsMDGEngine::SetNodeValueInternal(
>   case nsIDOMNode::PROCESSING_INSTRUCTION_NODE:
>   case nsIDOMNode::COMMENT_NODE:
>     rv = aContextNode->SetNodeValue(aNodeValue);
>     NS_ENSURE_SUCCESS(rv, rv);
> 
>     break;
> 
>   case nsIDOMNode::ELEMENT_NODE:
>+
>     rv = aContextNode->GetFirstChild(getter_AddRefs(childNode));
>     NS_ENSURE_SUCCESS(rv, rv);
> 
>-    if (!childNode) {
>-      rv = CreateNewChild(aContextNode, aNodeValue);
>-      NS_ENSURE_SUCCESS(rv, rv);
>-    } else {
>+    if (childNode) {
>       PRUint16 childType;
>       rv = childNode->GetNodeType(&childType);
>       NS_ENSURE_SUCCESS(rv, rv);
>-      
>-      if (   childType == nsIDOMNode::TEXT_NODE
>-          || childType == nsIDOMNode::CDATA_SECTION_NODE) {
>-        rv = childNode->SetNodeValue(aNodeValue);
>-        NS_ENSURE_SUCCESS(rv, rv);
>+
>+      if (childType != nsIDOMNode::TEXT_NODE &&
>+          childType != nsIDOMNode::CDATA_SECTION_NODE) {
>+        childNode = nsnull;
>       } else {
>-        // Not a text child, create a new one
>-        rv = CreateNewChild(aContextNode, aNodeValue, childNode);
>+        rv = childNode->SetNodeValue(aNodeValue);
>         NS_ENSURE_SUCCESS(rv, rv);
>+
>+        // Remove all leading text child nodes expept first one.
>+        nsCOMPtr<nsIDOMNode> siblingNode;
>+        while (true) {
>+          rv = childNode->GetNextSibling(getter_AddRefs(siblingNode));
>+          NS_ENSURE_SUCCESS(rv, rv);
>+          if (!siblingNode)
>+            break;
>+
>+          rv = siblingNode->GetNodeType(&childType);
>+          NS_ENSURE_SUCCESS(rv, rv);
>+          if (childType != nsIDOMNode::TEXT_NODE &&
>+              childType != nsIDOMNode::CDATA_SECTION_NODE) {
>+            break;
>+          }
>+          rv = aContextNode->RemoveChild(siblingNode,
>+                                         getter_AddRefs(siblingNode));
>+          NS_ENSURE_SUCCESS(rv, rv);
>+        }

I would remove all of these extra text nodes nodes first, before you set the node value.  Just in case the SetNodeValue that you are doing above it inserts a textnode (due to the 4K limit) that you might now delete.

>       }
>     }
>+
>+    if (!childNode) {
>+      rv = CreateNewChild(aContextNode, aNodeValue, childNode);
>+      NS_ENSURE_SUCCESS(rv, rv);
>+    }
>+

If there is no childNode, you don't need the third parameter.

>     break;
>-          
>+
>   default:
>     /// Unsupported nodeType
>     /// @todo Should return more specific error? (XXX)
>     return NS_ERROR_ILLEGAL_VALUE;
>     break;
>   }
>   
>   // NB: Never reached for Readonly nodes.  
>Index: extensions/xforms/nsXFormsUtils.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsUtils.cpp,v
>retrieving revision 1.91
>diff -u -8 -p -r1.91 nsXFormsUtils.cpp
>--- extensions/xforms/nsXFormsUtils.cpp	21 Sep 2006 16:49:13 -0000	1.91
>+++ extensions/xforms/nsXFormsUtils.cpp	23 Oct 2006 23:20:23 -0000
>@@ -754,36 +754,39 @@ nsXFormsUtils::GetNodeValue(nsIDOMNode* 
>   case nsIDOMNode::TEXT_NODE:
>   case nsIDOMNode::CDATA_SECTION_NODE:
>     // "Returns the string-value of the node."
>     aDataNode->GetNodeValue(aNodeValue);
>     break;
> 
>   case nsIDOMNode::ELEMENT_NODE:
>     {
>-      // "If text child nodes are present, returns the string-value of the
>-      // first text child node.  Otherwise, returns "" (the empty string)".
>+      // If text child nodes are present, returns the combined string value of
>+      // leading text child nodes. Otherwise, returns "" (the empty string).
> 
>       // Find the first child text node.
>       nsCOMPtr<nsIDOMNodeList> childNodes;
>       aDataNode->GetChildNodes(getter_AddRefs(childNodes));
> 
>       if (childNodes) {
>         nsCOMPtr<nsIDOMNode> child;
>         PRUint32 childCount;
>         childNodes->GetLength(&childCount);
> 
>+        nsAutoString value;
>         for (PRUint32 i = 0; i < childCount; ++i) {
>           childNodes->Item(i, getter_AddRefs(child));
>           NS_ASSERTION(child, "DOMNodeList length is wrong!");
> 
>           child->GetNodeType(&nodeType);
>           if (nodeType == nsIDOMNode::TEXT_NODE ||
>               nodeType == nsIDOMNode::CDATA_SECTION_NODE) {
>-            child->GetNodeValue(aNodeValue);
>+            child->GetNodeValue(value);
>+            aNodeValue.Append(value);
>+          } else {
>             break;
>           }
>         }
>       }
> 
>       // No child text nodes.  Return an empty string.
>     }
>     break;

Along the lines of comments to bug 328524, we should only return the combined value if we think that it is the result of a single node that had to be split due to size.  So I guess testing the node's value's length and appending the next node only if it is 4K?

Comment 12

[aaronr]

along the lines of my last review comment, we should probably only remove contiguous textnodes if we detect it is because of overflow (test 4K node value length).  Also Alex, please put a XXX around the code with a comment saying that this is a workaround only until bug 194231 is fixed.

Comment 13

[alexander :surkov (:asurkov)]

Talked with Aaron on irc. We can't combine values of text child nodes per spec 1.8.1 (http://www.w3.org/TR/xforms/slice8.html#ui-processing):

All form controls that read simpleContent instance data must do so as follows:
* Element nodes: if text child nodes are present, returns the string-value of the first text child node. Otherwise, returns "" (the empty string)

Either this bug is dupe of bug 194231 or specs behavior should be changed.

Comment 14

[aaronr]

(In reply to comment #13)
> Talked with Aaron on irc. We can't combine values of text child nodes per spec
> 1.8.1 (http://www.w3.org/TR/xforms/slice8.html#ui-processing):
> 
> All form controls that read simpleContent instance data must do so as follows:
> * Element nodes: if text child nodes are present, returns the string-value of
> the first text child node. Otherwise, returns "" (the empty string)
> 
> Either this bug is dupe of bug 194231 or specs behavior should be changed.
> 

Got further clarification from a member of the W3C XForms WG.  The 'text child node' that they mention there is from the xforms instance document which is, according to spec, is formed by 'creating an XPath data model'.  So we need to treat 'text node' in this case as an XPath text node and not a DOM text node.  If you look in the XPath spec, you'll see that there is no such thing as contiguous text nodes (http://www.w3.org/TR/xpath#section-Text-Nodes) in the XPath data model.  It also states there how to figure out the 'string-value' of the text node.  And if you look in the DOM XPath spec, especially here: http://www.w3.org/TR/2002/WD-DOM-Level-3-XPath-20020328/xpath.html#TextNodes, you'll see that "Applications using XPath in an environment with fragmented text nodes must manually gather the text of a single logical text node possibly from multiple nodes beginning with the first Text node or CDATASection node returned by the implementation".

So, in short, it looks like we need to concatenate our contiguous CDATA/DOM text nodes in order to implement GetNodeValue correctly.

Comment 15

[alexander :surkov (:asurkov)]

So, should I fix in my patch only next?

>+    if (!childNode) {
>+      rv = CreateNewChild(aContextNode, aNodeValue, childNode);
>+      NS_ENSURE_SUCCESS(rv, rv);
>+    }
>+

If there is no childNode, you don't need the third parameter.

Comment 16

[aaronr]

Comment on attachment 243266 [details] [diff] [review]
patch

(In reply to comment #15)
> So, should I fix in my patch only next?
> 
> >+    if (!childNode) {
> >+      rv = CreateNewChild(aContextNode, aNodeValue, childNode);
> >+      NS_ENSURE_SUCCESS(rv, rv);
> >+    }
> >+
> 
> If there is no childNode, you don't need the third parameter.
> 

nit: "Remove all leading text child nodes expept first one".  Change expept to except

I'd also say that you need to comment why we are doing it this way in GetNodeValue and SetNodeValueInternal -> the concept of no consecutive/contiguous xpath text nodes in a XPath data model...if there are they are considered one text node.  And probably wouldn't hurt mentioning where in the xpath spec it says that (and also point to 8.1.1 in the xforms spec).  Since we thought this old way for almost 2 years, I can definitely forsee someone maybe changing it back to the DOM text node interpretation some time in the future if we don't comment the code well.

With that, r=me

Comment 17

[alexander :surkov (:asurkov)]

Attachment: patch2

Comment 18

[aaronr]

checked into trunk for surkov

Comment 20

[aaronr]

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