356190 - Controls should not be allowed to bind to complexContent

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[Steve Speicher]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1) Gecko/20061003 Firefox/2.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1) Gecko/20061003 Firefox/2.0

This fails W3C test suite case 8.1.4.b

namely the spec says that controls like input/secret/textarea/output/... should be bound to "simple content".  There is currently some discussion in the XForms WG about support for "mixed content".  This bug is only to address the binding to complex content (elements) that are not allowed to have text associated with them.

Note: appears formsPlayer and XSmiles both allow controls to be associated with complexContent


Reproducible: Always

Comment 1

[Steve Speicher]

Attachment: test case with input, secret, textarea and output

Comment 2

[Steve Speicher]

failing test suite case, so linking to meta bug 322255

Comment 3

[aaronr]

I got feedback from the working group that defining this behavior is a workitem that is currently under way.  We can take a stab at this now.  We just need to keep in mind that the W3C may eventually define this in a way that will cause us to reimplement this functionality.

Comment 4

[Merle Sterling]

Attachment: patch 1

Patch 1: check for allowed content type during nsXFormsDelegateStub::Refresh.
Controls override IsContentAllowed to determine if the content type is acceptable; if not, an error is written to the console and BindingException event is dispatched.

Comment 5

[aaronr]

Ok, I debated with sspeiche.  Either way we are screwed.  If we follow the spec too closely, we are going to break a lot of forms by firing the fatal xforms-binding-exception when a control with simpleContent data binding restrictions is bound to a node that has a child element node.  And most processors make some attempt at supporting such a scenario (basically ignoring all nodes under the bound node but the leading CDATA/text nodes).  But if we don't fix this bug we'll technically not be spec compliant and fail a normative testcase from the testsuite.

So our decision, for now, will be to wait until the W3C hashes all of this out and issues a proper erratum for it.  In the meantime, we are going to ask the W3C to declare the testcase 'invalid' pending resolution of the matter.  And if they do choose to enforce some simpleContent rule, then we'll probably just have to tweak one of these patches a tiny bit and check it in.

Comment 6

[Merle Sterling]

Attachment: patch 1

Patch 1: Adding support for select/select1 which should only bind to complex content if an itemset is present.

Comment 7

[Merle Sterling]

Attachment: patch 2

Patch 2: IsContentAllowed checking is done in nsXFormsControlStub::resetBoundNode instead of nsXFormsDelegateStub::refresh as in patch 1. Submitting both patches so we can decide which approach is more efficient.

Comment 8

[aaronr]

(In reply to comment #7)
> Created an attachment (id=245288) [edit]
> patch 2
> 
> Patch 2: IsContentAllowed checking is done in
> nsXFormsControlStub::resetBoundNode instead of nsXFormsDelegateStub::refresh as
> in patch 1. Submitting both patches so we can decide which approach is more
> efficient.
> 

Note that these patches will also set the disabled intrinsic state for the control if the simpleContent rule is violated.  It is possible that the future errata will just have us generate the binding exception and not also hide the controls.

Comment 9

[Merle Sterling]

Awaiting clarification from the working group per comment #3 above.

Comment 10

[aaronr]

(In reply to comment #9)
> Awaiting clarification from the working group per comment #3 above.
> 

And comment #5, too.  WG proposal may really harm current schemas/instance data that customers want to use.

Comment 11

[Steve Speicher]

Here's feedback from the WG on this:
  http://lists.w3.org/Archives/Public/www-forms/2006Dec/0028.html

So looks like patch will need to be reworked to match this.  May want to post something to the NG to warn of this change when it is made, as it may affect some existing forms.

Comment 12

[Merle Sterling]

Comment on attachment 245288 [details] [diff] [review]
patch 2

Based on the WG erratum it appears that both patches correctly throw binding exceptions if a node has element children (complex content).  Patch2 that does the check during resetBoundNode is the better approach.

Comment 13

[aaronr]

Comment on attachment 245288 [details] [diff] [review]
patch 2

>Index: nsXFormsControlStub.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsControlStub.cpp,v
>retrieving revision 1.54
>diff -u -8 -p -r1.54 nsXFormsControlStub.cpp
>--- nsXFormsControlStub.cpp	8 Oct 2006 14:15:01 -0000	1.54
>+++ nsXFormsControlStub.cpp	11 Nov 2006 00:34:33 -0000
>@@ -178,22 +178,41 @@ nsXFormsControlStub::ResetBoundNode(cons
>     // When bound via @bind, we'll get a snapshot back
>     result->SnapshotItem(0, getter_AddRefs(mBoundNode));
>   } else {
>     result->GetSingleNodeValue(getter_AddRefs(mBoundNode));
>   }
> 
>   *aContextChanged = (oldBoundNode != mBoundNode);
> 
>-  if (!mBoundNode) {
>+  // Some controls may not be bound to certain types of content. If the content
>+  // is a disallowed type, report the error and dispatch a binding exception
>+  // event.
>+  PRBool isAllowed = PR_TRUE;
>+  IsContentAllowed(&isAllowed);
>+
>+  if (!mBoundNode || !isAllowed) {
>     // If there's no result (ie, no instance node) returned by the above, it
>     // means that the binding is not pointing to an instance data node, so we
>     // should disable the control.
>     mAppearDisabled = PR_TRUE;
> 
>+    if (!isAllowed) {
>+      // build the error string that we want output to the ErrorConsole
>+      nsAutoString localName;
>+      mElement->GetLocalName(localName);
>+      const PRUnichar *strings[] = { localName.get() };
>+
>+      nsXFormsUtils::ReportError(
>+        NS_LITERAL_STRING("boundTypeErrorComplexContent"),
>+        strings, 1, mElement, mElement);
>+
>+      nsXFormsUtils::DispatchEvent(mElement, eEvent_BindingException);
>+    }
>+
>     nsCOMPtr<nsIXTFElementWrapper> wrapper(do_QueryInterface(mElement));
>     NS_ENSURE_STATE(wrapper);
> 
>     PRInt32 iState;
>     GetDisabledIntrinsicState(&iState);
>     return wrapper->SetIntrinsicState(iState);
>   }
> 
>@@ -817,15 +836,48 @@ nsXFormsControlStub::GetBoundBuiltinType
>   // get type of bound instance data
>   nsCOMPtr<nsISchemaType> schemaType;
>   nsresult rv = mModel->GetTypeForControl(this, getter_AddRefs(schemaType));
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   return mModel->GetRootBuiltinType(schemaType, aBuiltinType);
> }
> 
>+NS_IMETHODIMP
>+nsXFormsControlStub::IsContentAllowed(PRBool *aIsAllowed)
>+{
>+  NS_ENSURE_ARG_POINTER(aIsAllowed);
>+  *aIsAllowed = PR_TRUE;
>+  return NS_OK;
>+}
>+
>+NS_IMETHODIMP
>+nsXFormsControlStub::IsContentComplex(PRBool *aIsComplex)
>+{
>+  NS_ENSURE_ARG_POINTER(aIsComplex);
>+  *aIsComplex = PR_FALSE;
>+
>+  if (mBoundNode) {
>+    // If the bound node has any Element children, then it has complexContent.
>+    nsCOMPtr<nsIDOMNode> currentNode;
>+    mBoundNode->GetFirstChild(getter_AddRefs(currentNode));
>+    while (currentNode) {
>+      PRUint16 nodeType;
>+      currentNode->GetNodeType(&nodeType);
>+      if (nodeType == nsIDOMNode::ELEMENT_NODE) {
>+          *aIsComplex = PR_TRUE;
>+          break;

nit: this section indented 4 columns instead of 2

>Index: nsXFormsControlStub.h
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsControlStub.h,v
>retrieving revision 1.37
>diff -u -8 -p -r1.37 nsXFormsControlStub.h
>--- nsXFormsControlStub.h	8 Oct 2006 14:15:01 -0000	1.37
>+++ nsXFormsControlStub.h	11 Nov 2006 00:34:33 -0000
>@@ -124,16 +124,34 @@ public:
>    *            different binding attrs, we do NO validation on the attr.  We
>    *            assume that the caller is smart enough to only send us a
>    *            binding attr atom.
>    *   aValue - The string value that the SNB attribute is being set to.
>    *
>    */
>   void AddRemoveSNBAttr(nsIAtom *aName, const nsAString &aValue);
> 
>+  /**
>+   * This function is overridden by controls that are restricted in the
>+   * type of content that may be bound to the control.
>+   * only if aIsAllowed is found to be PR_FALSE.
>+   *

The last sentence in this comment doesn't make any sense.  Please fix.  Also maybe add an example.  Like how xf:input can only be bound to simpleContent.

>Index: nsXFormsOutputElement.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsOutputElement.cpp,v
>retrieving revision 1.25
>diff -u -8 -p -r1.25 nsXFormsOutputElement.cpp
>--- nsXFormsOutputElement.cpp	1 Nov 2006 23:02:13 -0000	1.25
>+++ nsXFormsOutputElement.cpp	11 Nov 2006 00:34:34 -0000

>@@ -192,18 +194,34 @@ nsXFormsOutputElement::GetValue(nsAStrin
> 
> NS_IMETHODIMP
> nsXFormsOutputElement::SetValue(const nsAString& aValue)
> {
>   // Setting the value on an output controls seems wrong semantically.
>   return NS_ERROR_NOT_AVAILABLE;
> }
> 
>+NS_IMETHODIMP
>+nsXFormsOutputElement::IsContentAllowed(PRBool *aIsAllowed)
>+{
>+  NS_ENSURE_ARG_POINTER(aIsAllowed);
>+  *aIsAllowed = PR_TRUE;
>+
>+  // Output may not be bound to complexContent.
>+  PRBool isComplex = PR_FALSE;
>+  IsContentComplex(&isComplex);
>+  if (isComplex) {
>+    *aIsAllowed = PR_FALSE;
>+  }
>+  return NS_OK;
>+}
>+
> NS_HIDDEN_(nsresult)
> NS_NewXFormsOutputElement(nsIXTFElement **aResult)
> {
>   *aResult = new nsXFormsOutputElement();
>   if (!*aResult)
>     return NS_ERROR_OUT_OF_MEMORY;
> 
>   NS_ADDREF(*aResult);
>   return NS_OK;
> }
>+

nit: no need to add blank line at end of file.

>Index: nsXFormsSelect1Element.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsSelect1Element.cpp,v
>retrieving revision 1.5
>diff -u -8 -p -r1.5 nsXFormsSelect1Element.cpp
>--- nsXFormsSelect1Element.cpp	8 Oct 2006 14:15:01 -0000	1.5
>+++ nsXFormsSelect1Element.cpp	11 Nov 2006 00:34:34 -0000

>@@ -188,16 +190,76 @@ nsXFormsSelect1Element::GetXFormsAccesso
>     if (!mAccessor) {
>       return NS_ERROR_OUT_OF_MEMORY;
>     }
>   }
>   NS_ADDREF(*aAccessor = mAccessor);
>   return NS_OK;
> }
> 
>+NS_IMETHODIMP
>+nsXFormsSelect1Element::IsContentAllowed(PRBool *aIsAllowed)
>+{
>+  NS_ENSURE_ARG_POINTER(aIsAllowed);
>+  *aIsAllowed = PR_TRUE;
>+
>+  // For select1 elements, non-simpleContent is only allowed if the select
>+  // element contains an itemset.
>+  PRBool isComplex = PR_FALSE;
>+  IsContentComplex(&isComplex);
>+  if (isComplex) {
>+    PRBool containsItemset = PR_FALSE;
>+    nsCOMPtr<nsIDOMNodeList> selectChildren;
>+    mElement->GetChildNodes(getter_AddRefs(selectChildren));
>+
>+    PRUint32 childCount = 0;
>+    if (selectChildren) {
>+      selectChildren->GetLength(&childCount);
>+    }
>+
>+    nsAutoString localName;
>+    for (PRUint32 i = 0; i < childCount; ++i) {
>+      nsCOMPtr<nsIDOMNode> selectChild;
>+      selectChildren->Item(i, getter_AddRefs(selectChild));
>+      selectChild->GetLocalName(localName);
>+      if (localName.EqualsLiteral("itemset")) {
>+        // Found an itemset. complexContent is ok.
>+        containsItemset = PR_TRUE;
>+        break;
>+      } else if (localName.EqualsLiteral("choices")) {
>+         // The choices element may have an itemset.
>+         nsCOMPtr<nsIDOMNodeList> choicesChildren;
>+         selectChild->GetChildNodes(getter_AddRefs(choicesChildren));
>+         if (choicesChildren) {
>+           PRUint32 childCount = 0;
>+           choicesChildren->GetLength(&childCount);
>+
>+           nsAutoString localName;
>+           for (PRUint32 j = 0; j < childCount; ++j) {
>+             nsCOMPtr<nsIDOMNode> choicesChild;
>+             choicesChildren->Item(j, getter_AddRefs(choicesChild));
>+             choicesChild->GetLocalName(localName);
>+             if (localName.EqualsLiteral("itemset")) {
>+               containsItemset = PR_TRUE;
>+               break;
>+             }
>+           }
>+         }
>+      }
>+    }
>+

To be perfectly accurate, you could probably check for namespaces, too.  You should probably use: nsXFormsUtils::IsXFormsElement.  Also according to the xforms schema, a choices element can contain another choices element which could then contain an itemset.  Looks to me that maybe you ought to make this itemset search function a utility function or a pair of functions that could recurse.  Put it in nsXFormsUtils.cpp.

>+    if (!containsItemset) {
>+      // The content is complex but there are no itemset elements so
>+      // the content is not allowed.
>+      *aIsAllowed = PR_FALSE;
>+    }
>+  }
>+  return NS_OK;
>+}
>+
> NS_HIDDEN_(nsresult)
> NS_NewXFormsSelect1Element(nsIXTFElement **aResult)
> {
>   *aResult = new nsXFormsSelect1Element(NS_LITERAL_STRING("select1"));
>   if (!*aResult)
>     return NS_ERROR_OUT_OF_MEMORY;
> 
>   NS_ADDREF(*aResult);
>Index: nsXFormsSelectElement.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsSelectElement.cpp,v
>retrieving revision 1.16
>diff -u -8 -p -r1.16 nsXFormsSelectElement.cpp
>--- nsXFormsSelectElement.cpp	8 Oct 2006 14:15:01 -0000	1.16
>+++ nsXFormsSelectElement.cpp	11 Nov 2006 00:34:34 -0000
>@@ -70,16 +70,18 @@ public:
>   // nsIXFormsControl
>   NS_IMETHOD Refresh();
>   NS_IMETHOD GetDefaultIntrinsicState(PRInt32 *aState);
>   NS_IMETHOD GetDisabledIntrinsicState(PRInt32 *aState);
> 
>   // nsIXFormsDelegate overrides
>   NS_IMETHOD GetXFormsAccessors(nsIXFormsAccessors **aAccessor);
> 
>+  NS_IMETHOD IsContentAllowed(PRBool *aIsAllowed);
>+
> #ifdef DEBUG_smaug
>   virtual const char* Name() { return "select"; }
> #endif
> };
> 
> NS_IMPL_ISUPPORTS_INHERITED0(nsXFormsSelectElement,
>                              nsXFormsDelegateStub)
> 
>@@ -160,16 +162,76 @@ nsXFormsSelectElement::GetXFormsAccessor
>     if (!mAccessor) {
>       return NS_ERROR_OUT_OF_MEMORY;
>     }
>   }
>   NS_ADDREF(*aAccessor = mAccessor);
>   return NS_OK;
> }
> 
>+NS_IMETHODIMP
>+nsXFormsSelectElement::IsContentAllowed(PRBool *aIsAllowed)
>+{
>+  NS_ENSURE_ARG_POINTER(aIsAllowed);
>+  *aIsAllowed = PR_TRUE;
>+
>+  // For select elements, non-simpleContent is only allowed if the select
>+  // element contains an itemset.
>+  PRBool isComplex = PR_FALSE;
>+  IsContentComplex(&isComplex);
>+  if (isComplex) {
>+    PRBool containsItemset = PR_FALSE;
>+    nsCOMPtr<nsIDOMNodeList> selectChildren;
>+    mElement->GetChildNodes(getter_AddRefs(selectChildren));
>+
>+    PRUint32 childCount = 0;
>+    if (selectChildren) {
>+      selectChildren->GetLength(&childCount);
>+    }
>+
>+    nsAutoString localName;
>+    for (PRUint32 i = 0; i < childCount; ++i) {
>+      nsCOMPtr<nsIDOMNode> selectChild;
>+      selectChildren->Item(i, getter_AddRefs(selectChild));
>+      selectChild->GetLocalName(localName);
>+      if (localName.EqualsLiteral("itemset")) {
>+        // Found an itemset. complexContent is ok.
>+        containsItemset = PR_TRUE;
>+        break;
>+      } else if (localName.EqualsLiteral("choices")) {
>+         // The choices element may have an itemset.
>+         nsCOMPtr<nsIDOMNodeList> choicesChildren;
>+         selectChild->GetChildNodes(getter_AddRefs(choicesChildren));
>+         if (choicesChildren) {
>+           PRUint32 childCount = 0;
>+           choicesChildren->GetLength(&childCount);
>+
>+           nsAutoString localName;
>+           for (PRUint32 j = 0; j < childCount; ++j) {
>+             nsCOMPtr<nsIDOMNode> choicesChild;
>+             choicesChildren->Item(j, getter_AddRefs(choicesChild));
>+             choicesChild->GetLocalName(localName);
>+             if (localName.EqualsLiteral("itemset")) {
>+               containsItemset = PR_TRUE;
>+               break;
>+             }
>+           }
>+         }
>+      }
>+    }
>+
>+    if (!containsItemset) {
>+      // The content is complex but there are no itemset elements so
>+      // the content is not allowed.
>+      *aIsAllowed = PR_FALSE;
>+    }
>+  }
>+  return NS_OK;
>+}
>+
> NS_HIDDEN_(nsresult)
> NS_NewXFormsSelectElement(nsIXTFElement **aResult)
> {
>   *aResult = new nsXFormsSelectElement();
>   if (!*aResult)
>     return NS_ERROR_OUT_OF_MEMORY;
> 
>   NS_ADDREF(*aResult);
>Index: resources/locale/en-US/xforms.properties
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/resources/locale/en-US/xforms.properties,v
>retrieving revision 1.32
>diff -u -8 -p -r1.32 xforms.properties
>--- resources/locale/en-US/xforms.properties	21 Sep 2006 16:49:13 -0000	1.32
>+++ resources/locale/en-US/xforms.properties	11 Nov 2006 00:34:34 -0000
>@@ -72,16 +72,17 @@ defInstanceNotFound  = XForms Error (33)
> MDGLoopError         = XForms Error (34): There are loops in the bindings of the model!
> invalidExtFunction   = XForms Error (35): Non-existant extension functions listed in this model's function attribute
> inlineInstanceNoChildError = XForms Error (36): Inline instance has no child elements. This is illegal.
> inlineInstanceMultipleElementsError = XForms Error (37): Inline instance has multiple child elements. This is illegal.
> modelLoopError       = XForms Error (38): There seems to be an infinite loop in the event handling for the form. The maximum loop value can be set via xforms.modelLoopMax.
> duplicateSchema      = XForms Error (39): Ignoring duplicate schema with target namespace: %S
> boundTypeErrorInclusive = XForms Error (40): %S element not bound to valid datatype.  Must be bound to one of these datatypes or a type derived from one of these datatypes: %S.
> boundTypeErrorExclusive = XForms Error (41): %S element not bound to valid datatype.  Must not be bound to one of these datatypes or a type derived from one of these datatypes: %S.
>+boundTypeErrorComplexContent = XForms Error (41): %S element may not be bound to complex content.

change number to 42.  41 already used.

Comment 14

[Merle Sterling]

Attachment: patch

Fix typo and spacing nits; recursive function to find itemset in select/1 in case there are nested choice elements.

Comment 15

[aaronr]

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

>Index: nsXFormsControlStub.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsControlStub.cpp,v
>retrieving revision 1.55
>diff -u -8 -p -r1.55 nsXFormsControlStub.cpp
>--- nsXFormsControlStub.cpp	11 Dec 2006 10:54:09 -0000	1.55
>+++ nsXFormsControlStub.cpp	10 Jan 2007 20:10:10 -0000

>@@ -358,40 +377,45 @@ nsXFormsControlStub::ProcessNodeBinding(
>   NS_ENSURE_SUCCESS(rv, rv);
>   NS_ENSURE_STATE(mModel);
> 
>   rv = MaybeAddToModel(oldModel, parentControl);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   if (aModel)
>     NS_ADDREF(*aModel = mModel);
>-
>   mUsesModelBinding = usesModelBinding;
> 
>-  if (indexesUsed.Count()) {
>+  nsCOMPtr<nsIContent> content(do_QueryInterface(mElement));
>+  NS_ENSURE_STATE(content);
>+  nsCOMPtr<nsIDocument> doc = content->GetCurrentDoc();
>+  nsCOMPtr<nsIDOMDocument> domDoc(do_QueryInterface(doc));
>+  NS_ENSURE_STATE(domDoc);
>+
>+  if (NS_SUCCEEDED(rv) && indexesUsed.Count()) {

I don't see why rv wouldn't already be successful.  It was checked after the last time it was used (return from MaybeAddToModel)

>     // add index listeners on repeat elements
> 
>     for (PRInt32 i = 0; i < indexesUsed.Count(); ++i) {
>       // Find the repeat element and add |this| as a listener
>       nsCOMPtr<nsIDOMElement> repElem;
>-      nsXFormsUtils::GetElementByContextId(mElement, *(indexesUsed[i]),
>-                                           getter_AddRefs(repElem));
>+      domDoc->GetElementById(*(indexesUsed[i]), getter_AddRefs(repElem));

Why this change?

>       nsCOMPtr<nsIXFormsRepeatElement> rep(do_QueryInterface(repElem));
>       if (!rep)
>         continue;
> 
>       rv = rep->AddIndexUser(this);
>-      NS_ENSURE_SUCCESS(rv, rv);
>-
>+      if (NS_FAILED(rv)) {
>+        return rv;
>+      }

Why this change?

>Index: nsXFormsSelect1Element.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsSelect1Element.cpp,v
>retrieving revision 1.5
>diff -u -8 -p -r1.5 nsXFormsSelect1Element.cpp
>--- nsXFormsSelect1Element.cpp	8 Oct 2006 14:15:01 -0000	1.5
>+++ nsXFormsSelect1Element.cpp	10 Jan 2007 20:10:11 -0000

>@@ -188,16 +190,33 @@ nsXFormsSelect1Element::GetXFormsAccesso
>     if (!mAccessor) {
>       return NS_ERROR_OUT_OF_MEMORY;
>     }
>   }
>   NS_ADDREF(*aAccessor = mAccessor);
>   return NS_OK;
> }
> 
>+NS_IMETHODIMP
>+nsXFormsSelect1Element::IsContentAllowed(PRBool *aIsAllowed)
>+{
>+  NS_ENSURE_ARG_POINTER(aIsAllowed);
>+  *aIsAllowed = PR_FALSE;
>+
>+  // For select1 elements, non-simpleContent is only allowed if the select
>+  // element contains an itemset.
>+  PRBool isComplex = PR_FALSE;
>+  IsContentComplex(&isComplex);
>+  if (isComplex && nsXFormsUtils::NodeHasItemset(mElement)) {
>+    *aIsAllowed = PR_TRUE;
>+  }
>+
>+  return NS_OK;
>+}
>+

Ummm, what about if the content ISN'T complex?  Shouldn't aIsAllowed be PR_TRUE for that case, too?

>Index: nsXFormsSelectElement.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsSelectElement.cpp,v
>retrieving revision 1.16
>diff -u -8 -p -r1.16 nsXFormsSelectElement.cpp
>--- nsXFormsSelectElement.cpp	8 Oct 2006 14:15:01 -0000	1.16
>+++ nsXFormsSelectElement.cpp	10 Jan 2007 20:10:11 -0000

>@@ -160,16 +162,33 @@ nsXFormsSelectElement::GetXFormsAccessor
>     if (!mAccessor) {
>       return NS_ERROR_OUT_OF_MEMORY;
>     }
>   }
>   NS_ADDREF(*aAccessor = mAccessor);
>   return NS_OK;
> }
> 
>+NS_IMETHODIMP
>+nsXFormsSelectElement::IsContentAllowed(PRBool *aIsAllowed)
>+{
>+  NS_ENSURE_ARG_POINTER(aIsAllowed);
>+  *aIsAllowed = PR_FALSE;
>+
>+  // For select elements, non-simpleContent is only allowed if the select
>+  // element contains an itemset.
>+  PRBool isComplex = PR_FALSE;
>+  IsContentComplex(&isComplex);
>+  if (isComplex && nsXFormsUtils::NodeHasItemset(mElement)) {
>+    *aIsAllowed = PR_TRUE;
>+  }
>+
>+  return NS_OK;
>+}
>+

ditto fot this function.

>Index: nsXFormsUtils.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsUtils.cpp,v
>retrieving revision 1.93
>diff -u -8 -p -r1.93 nsXFormsUtils.cpp
>--- nsXFormsUtils.cpp	11 Dec 2006 10:54:09 -0000	1.93
>+++ nsXFormsUtils.cpp	10 Jan 2007 20:10:13 -0000
>@@ -2714,8 +2714,40 @@ nsXFormsUtils::SetSingleNodeBindingValue
>   {
>     nsresult rv = model->SetNodeValue(node, aValue, PR_FALSE, aChanged);
>     if (NS_SUCCEEDED(rv))
>       return PR_TRUE;
>   }
>   return PR_FALSE;
> }
> 
>+/* static */ PRBool
>+nsXFormsUtils::NodeHasItemset(nsIDOMNode *aNode)
>+{
>+  PRBool hasItemset = PR_FALSE;
>+
>+  nsCOMPtr<nsIDOMNodeList> children;
>+  aNode->GetChildNodes(getter_AddRefs(children));
>+
>+  PRUint32 childCount = 0;
>+  if (children) {
>+    children->GetLength(&childCount);
>+  }
>+
>+  for (PRUint32 i = 0; i < childCount; ++i) {
>+    nsCOMPtr<nsIDOMNode> child;
>+    children->Item(i, getter_AddRefs(child));
>+    if (nsXFormsUtils::IsXFormsElement(child, NS_LITERAL_STRING("itemset"))) {
>+      hasItemset = PR_TRUE;
>+      break;
>+    } else if (nsXFormsUtils::IsXFormsElement(child,
>+                                              NS_LITERAL_STRING("choices"))) {
>+      // The choices element may have an itemset.
>+      hasItemset = nsXFormsUtils::NodeHasItemset(child);
>+      if (hasItemset) {
>+        // No need to look at any other children.
>+        break;
>+      }
>+    }
>+  }
>+  return hasItemset;
>+}
>+

>Index: nsXFormsUtils.h
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsUtils.h,v
>retrieving revision 1.58
>diff -u -8 -p -r1.58 nsXFormsUtils.h
>--- nsXFormsUtils.h	11 Dec 2006 10:54:09 -0000	1.58
>+++ nsXFormsUtils.h	10 Jan 2007 20:10:14 -0000
>@@ -699,16 +699,23 @@ public:
>                                          double * aSeconds);
> 
>   static NS_HIDDEN_(nsresult) GetSecondsFromDateTime(const nsAString & aValue,
>                                                      double * aSeconds);
> 
>   static NS_HIDDEN_(nsresult) GetDaysFromDateTime(const nsAString & aValue,
>                                                   PRInt32 * aDays);
> 
>+  /**
>+   * Determine whether or not a select/select1 or choices node has an itemset
>+   * node as a child. Non-simpleContent is only allowed for Select/1 if it
>+   * contains an itemset.
>+   */
>+  static NS_HIDDEN_(PRBool) NodeHasItemset(nsIDOMNode *aNode);
>+

You don't prevent any other node from being tested and your comment kinda lead me to believe that this test would only work for select/1 or choices.  Also the non-simpleContent comment seems out of context here.  Maybe qualify the statement a little.  How about:

"Determine whether the given node contains a xf:itemset as a child.  In valid XForms documents this should only be possible if aNode is a xf:select/1 or a xf:choices element.  This function is used primarily as a worker function for select/1's IsContentAllowed override."

Comment 16

[Merle Sterling]

Regarding the comments 'Why this change?' - I didn't actually make those changes.  I must have copied over my older version of nsXFormsControlStub.

I'll fix the other issue with return true if the content is not complex.

Comment 17

[Merle Sterling]

Attachment: patch checked in #1

Comment 18

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

Comment on attachment 251134 [details] [diff] [review]
patch checked in #1

>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsControlStub.h,v
...
> 
>+  /**
>+   * This function is overridden by controls that are restricted in the
>+   * type of content that may be bound to the control. For example,
>+   * xf:input may only be bound to simpleContent.
>+   *
>+   * @param aIsAllowed     Indicates whether the control is allowed to bind
>+   *                       to the content type.
>+   */
>+  NS_IMETHOD IsContentAllowed(PRBool *aIsAllowed);
>+
>+  /**
>+   * This function determines if the content to which the control is being
>+   * bound is complex; ie contains Element children.
>+   *
>+   * @param aIsComplex     Indicates whether the content is complex.
>+   */
>+  NS_IMETHOD IsContentComplex(PRBool *aIsComplex);
>+


The new methods should be |virtual PRBool IsContent...()|.
No need for NS_IMETHOD... because the methods aren't in any interface.

With that, r=me

Comment 19

[alexander :surkov (:asurkov)]

(In reply to comment #18)

> The new methods should be |virtual PRBool IsContent...()|.
> No need for NS_IMETHOD... because the methods aren't in any interface.
> 
> With that, r=me
> 

Right, and then you can do
nsXFormsControlStub::IsContentAllowed()
{
  return !IsContentComplex();
}

That will work for all elements excepting select/select1.

Comment 20

[alexander :surkov (:asurkov)]

It's good to follow to specs note though this can broke some xforms pages but the main reason I don't like the patch is we are loosing flexibility of custom controls. If earlier I was free to create custom control like control that is proposed in bug 316817 then now I can't do it. I can see two ways:
1) provide an ability to create new xforms tags or add xforms specifity for any element (like http://www.w3.org/TR/xforms/slice7.html#ui-binding-foreign)
2) move IsContentAllowed() to nsIXFormsUIWidget, this provides ability for custom control authors to control it.

What do you think?

Comment 21

[aaronr]

Boy, I dunno.  Can you give an example?  If someone is overriding a widget to allow the handling of complex content, then they'll have to also modify how the bound data is gathered and rendered since for our restricted controls we won't look any farther than the first text node.  Which seems to me that the user would be better off creating their own widget rather than extending one of our restricted ones.

Comment 22

[Steve Speicher]

I think that making the change now to be "compliant" makes sense now, before more custom widgets are made on these.  So I agree that it will break some existing apps, I know I have a few, I believe now is the best time to do this.  The W3C test suite has been out for a while and has a test case that verifies this.

Comment 23

[alexander :surkov (:asurkov)]

(In reply to comment #21)
> Boy, I dunno.  Can you give an example?

Like widget described in bug bug 316817, output that shows tree of node that is bound to.

> If someone is overriding a widget to
> allow the handling of complex content, then they'll have to also modify how the
> bound data is gathered and rendered since for our restricted controls we won't
> look any farther than the first text node.

Absolutely right. nsIXFormsAccessors provides getBoundNode()/setContent() methods and I can manipulate with bound node as I wish.

> Which seems to me that the user
> would be better off creating their own widget rather than extending one of our
> restricted ones.

What does the creating their own widget mean, ie what tag should he use, xforms or any namespace tag?

I guess we can add attribute 'isComlexContentAllowed' to nsIXFormsUIWidget that may take tree constants: DEFAUL_CONTENT_ALLOWED, SIMPLE_CONTENT_ALLOWED, COMPLEX_CONTENT_ALLOWED. Then we provide abibility to custom controls author define own behavior for his control.

(In reply to comment #22)
> I think that making the change now to be "compliant" makes sense now, before
> more custom widgets are made on these.  So I agree that it will break some
> existing apps, I know I have a few, I believe now is the best time to do this. 
> The W3C test suite has been out for a while and has a test case that verifies
> this.
> 

That is fine. Controls that are described by specs should behave like specs assumes for them. But why the idea to extend behaviour of xforms controls is bad? I think we should provide an ability to manipulate it.

Comment 24

[aaronr]

(In reply to comment #23)
> (In reply to comment #21)
> > Boy, I dunno.  Can you give an example?
> 
> Like widget described in bug bug 316817, output that shows tree of node that is
> bound to.

But why does this custom control need to be a xf:output instead of a foo:bar control that the user defines and has extend our nsXFormsUIWidget?

> (In reply to comment #22)
> > I think that making the change now to be "compliant" makes sense now, before
> > more custom widgets are made on these.  So I agree that it will break some
> > existing apps, I know I have a few, I believe now is the best time to do this. 
> > The W3C test suite has been out for a while and has a test case that verifies
> > this.
> > 
> 
> That is fine. Controls that are described by specs should behave like specs
> assumes for them. But why the idea to extend behaviour of xforms controls is
> bad? I think we should provide an ability to manipulate it.
> 

My only problem with this is that so far our custom control work mostly allows users to change the widgets used to represent an xforms control.  Which is completely within the spirit of the XForms spec.  But with the capability that you propose, we'd be giving the user the ability to behave in direct contradiction to the spec.  This one change isn't probably that big of a deal, but it is a bad precedent.

Leigh, as a person with a vested interest in spec compliance and a person who will also likely use custom controls to write forms, what do you think?  Am I making too big a deal about this?

Comment 25

[alexander :surkov (:asurkov)]

(In reply to comment #24)
> (In reply to comment #23)
> > (In reply to comment #21)
> > > Boy, I dunno.  Can you give an example?
> > 

> > That is fine. Controls that are described by specs should behave like specs
> > assumes for them. But why the idea to extend behaviour of xforms controls is
> > bad? I think we should provide an ability to manipulate it.
> > 
> 
> My only problem with this is that so far our custom control work mostly allows
> users to change the widgets used to represent an xforms control.  Which is
> completely within the spirit of the XForms spec.  But with the capability that
> you propose, we'd be giving the user the ability to behave in direct
> contradiction to the spec.  This one change isn't probably that big of a deal,
> but it is a bad precedent.
> 

I can see your point of view. You are saying that http://www.w3.org/TR/xforms/slice7.html#ui-binding-foreign is all we want to have here. Right? But now we broke an ability and offer in exchange nothing.

Comment 26

[aaronr]

(In reply to comment #25)
> (In reply to comment #24)
> > (In reply to comment #23)
> > > (In reply to comment #21)
> > > > Boy, I dunno.  Can you give an example?
> > > 
> 
> > > That is fine. Controls that are described by specs should behave like specs
> > > assumes for them. But why the idea to extend behaviour of xforms controls is
> > > bad? I think we should provide an ability to manipulate it.
> > > 
> > 
> > My only problem with this is that so far our custom control work mostly allows
> > users to change the widgets used to represent an xforms control.  Which is
> > completely within the spirit of the XForms spec.  But with the capability that
> > you propose, we'd be giving the user the ability to behave in direct
> > contradiction to the spec.  This one change isn't probably that big of a deal,
> > but it is a bad precedent.
> > 
> 
> I can see your point of view. You are saying that
> http://www.w3.org/TR/xforms/slice7.html#ui-binding-foreign is all we want to
> have here. Right? But now we broke an ability and offer in exchange nothing.
> 

Right, that is what I'm saying.  And I don't think of it as breaking a capability, but rather conforming to the spec where we didn't before.

I suggest that we check in this bug and open a new bug that does what you propose (allow for custom controls to ignore the simpleContent restriction).  And then wait on that new bug until we see what the other processors decide to do or see if users write about wanting that capability on the newsgroup.

Comment 27

[alexander :surkov (:asurkov)]

It's fine with me to open another bug that will adress custom controls issue.

Comment 28

[aaronr]

checked into trunk for msterlin

Comment 29

[alexander :surkov (:asurkov)]

(In reply to comment #28)
> checked into trunk for msterlin
> 

I'm sorry probably I missed but where is planned to address smaug's comment #18?

Comment 30

[aaronr]

(In reply to comment #29)
> (In reply to comment #28)
> > checked into trunk for msterlin
> > 
> 
> I'm sorry probably I missed but where is planned to address smaug's comment
> #18?
> 

I didn't realize that the patch wasn't ready.  Merle, please attach a new patch to address.

Comment 31

[alexander :surkov (:asurkov)]

(In reply to comment #27)
> It's fine with me to open another bug that will adress custom controls issue.
> 

I filed new bug 367733 for this.

Comment 32

[Merle Sterling]

Attachment: revised patch

Changing signature of IsContentAllowed and IsContentComplex member functions per comments 18 and 19.

Comment 33

[alexander :surkov (:asurkov)]

Comment on attachment 252539 [details] [diff] [review]
revised patch


>+  PRBool IsContentAllowed();

Is virtual needed here if sometime we will have the class inherited from this class?

Comment 34

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

(In reply to comment #33)
> (From update of attachment 252539 [details] [diff] [review])
> 
> >+  PRBool IsContentAllowed();
> 
> Is virtual needed here if sometime we will have the class inherited from this
> class?
> 

Ah, although not needed (at least not now), it might be better to have the |virtual|

Comment 35

[aaronr]

Comment on attachment 252539 [details] [diff] [review]
revised patch

I don't like the new change that made all controls bind to simpleContent only by default.

1) trigger and submit don't have that limitation but you have no override in the patch for them.

2) if we ever allow non-xforms elements to bind to xforms data models (like here: http://www.w3.org/TR/xforms/index-all.html#ui-binding-foreign) then it is quite likely we'll have to hook them into nsXFormsControlStub somehow (though I admit now I don't know how we'd do it, except maybe using some kind of shadow control) and I don't think we want to limit them this way.

So since I don't know if #2 will ever come to pass in a way that will be affected by this patch, I guess I won't veto the default being simpleContent only.  But we still need trigger and submit to not have that limitation.

Other than that, nothing else wrong with the patch.

Comment 36

[Merle Sterling]

Attachment: patch

Going back to the original approach where nsXFormsControlStub::IsContentAllowed returns true (allow any content) and every class that needs to deny certain types of content must override IsContentAllowed. Trigger and Submit will now work properly as the default inherited from controlstub is correct for those elements.

Adding the virtual keyword to the IsContentAllowed overrides too just in case we ever need to subclass any of those classes.

Comment 37

[aaronr]

Comment on attachment 251134 [details] [diff] [review]
patch checked in #1

changing this patch back from obsolete since patch to fix signatures, etc. is built on the fact that this patch was checked in already.  So when we build the patches for 1.8.x, we'll need to include both patches from here.

Comment 38

[Merle Sterling]

Attachment: patch

Forgot to add virtual to select/1

Comment 39

[aaronr]

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

>Index: nsXFormsInputElement.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsInputElement.cpp,v
>retrieving revision 1.36
>diff -u -8 -p -r1.36 nsXFormsInputElement.cpp
>--- nsXFormsInputElement.cpp	19 Jan 2007 22:22:07 -0000	1.36
>+++ nsXFormsInputElement.cpp	24 Jan 2007 23:40:18 -0000

>@@ -130,31 +127,28 @@ nsXFormsTextareaElement::IsTypeAllowed(P
>     return NS_OK;
>   }
> 
>   // build the string of types that textareas can bind to
>   aAllowedTypes.AssignLiteral("xsd:string");
>   return NS_OK;
> }
> 
>-NS_IMETHODIMP
>-nsXFormsTextareaElement::IsContentAllowed(PRBool *aIsAllowed)
>+PRBool
>+nsXFormsTextareaElement::IsContentAllowed()
> {
>-  NS_ENSURE_ARG_POINTER(aIsAllowed);
>-  *aIsAllowed = PR_TRUE;
>+  PRBool isAllowed = PR_TRUE;
> 
>-  // Textareas may not be bound to complexContent.
>-  PRBool isComplex = PR_FALSE;
>-  IsContentComplex(&isComplex);
>+  // Textarea elements may not be bound to complexContent.
>+  PRBool isComplex = IsContentComplex();
>   if (isComplex) {
>-    *aIsAllowed = PR_FALSE;
>+    isAllowed = PR_FALSE;
>   }
>-  return NS_OK;
>+  return isAllowed;
> }
>-


nit: you are removing an extraneous line after the function.  I like having that line of seperation before the comment, so please correct.

> // Creators
> 
> NS_HIDDEN_(nsresult)
> NS_NewXFormsInputElement(nsIXTFElement **aResult)
> {
>   *aResult = new nsXFormsInputElement(NS_LITERAL_STRING("input"));
>   if (!*aResult)
>     return NS_ERROR_OUT_OF_MEMORY;

With that, r=me

Comment 40

[Merle Sterling]

Attachment: patch checked in #2

Ok, added back the blank line. :)

Comment 41

[aaronr]

checked in second patch to trunk for msterlin to fix signatures oversight.

Comment 42

[aaronr]

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