Last Comment Bug 357652 - xf:textarea appears to have a 4096 characters size restriction
: xf:textarea appears to have a 4096 characters size restriction
Status: RESOLVED FIXED
: fixed1.8.0.12, fixed1.8.1.4
Product: Core Graveyard
Classification: Graveyard
Component: XForms (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: alexander :surkov
: Stephen Pride
:
Mentors:
: 372324 (view as bug list)
Depends on:
Blocks: 353738
  Show dependency treegraph
 
Reported: 2006-10-23 03:14 PDT by jan-wijbrand kolman
Modified: 2016-07-15 14:46 PDT (History)
3 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
A simple text case demonstrating the described problem. (16.66 KB, application/xhtml+xml)
2006-10-23 03:16 PDT, jan-wijbrand kolman
no flags Details
patch (4.47 KB, patch)
2006-10-23 16:21 PDT, alexander :surkov
aaronr: review+
Details | Diff | Splinter Review
patch2 (5.21 KB, patch)
2006-10-25 09:14 PDT, alexander :surkov
doronr: review+
Details | Diff | Splinter Review

Description jan-wijbrand kolman 2006-10-23 03:14:20 PDT
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 2006-10-23 03:16:03 PDT
Created attachment 243171 [details]
A simple text case demonstrating the described problem.

This attachement is a simple text case demonstrating the described problem.
Comment 2 jan-wijbrand kolman 2006-10-23 03:17:12 PDT
If you need additional information or details, I'd be more than happy to provide it!
Comment 3 Alex Vincent [:WeirdAl] 2006-10-23 08:34:05 PDT
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 2006-10-23 13:51:58 PDT
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 2006-10-23 14:24:39 PDT
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 2006-10-23 14:28:20 PDT
(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 2006-10-23 14:37:25 PDT
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. 2006-10-23 15:03:46 PDT
I recently noticed xf:output has the same limitation, but presumably you already know that.
Comment 9 aaronr 2006-10-23 15:15:09 PDT
(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 2006-10-23 16:21:43 PDT
Created attachment 243266 [details] [diff] [review]
patch
Comment 11 aaronr 2006-10-23 16:53:01 PDT
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 2006-10-23 16:58:54 PDT
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 2006-10-23 17:58:49 PDT
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 2006-10-24 14:19:50 PDT
(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 2006-10-24 15:24:16 PDT
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 2006-10-24 15:40:26 PDT
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 2006-10-25 09:14:26 PDT
Created attachment 243482 [details] [diff] [review]
patch2
Comment 18 aaronr 2006-10-25 13:42:53 PDT
checked into trunk for surkov
Comment 19 aaronr 2007-03-04 15:45:48 PST
*** Bug 372324 has been marked as a duplicate of this bug. ***
Comment 20 aaronr 2007-04-23 15:43:53 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.