300801 - XBLize xforms:select

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[Doron Rosenberg (IBM)]

Comment 1

[Doron Rosenberg (IBM)]

Attachment: work in progress

Work in progress, patch contains some select1 leftovers.  It seems to work ok,
doesn't include appearance="full" since I am having issues getting it to work.

Comment 2

[Doron Rosenberg (IBM)]

Attachment: latest work

feel free to give review comments.

Comment 3

[Allan Beaufour]

Comment on attachment 191747 [details] [diff] [review]
latest work

> Index: extensions/xforms/nsXFormsChoicesElement.cpp
> ===================================================================

* Remember to kill the class comment, it talks about creating an optgroup.

* Shouldn't this inherit from nsXFormsDelegateStub? This goes for most of the
controls.

* Are the 2 TODO's in ChildRemoved() still valid?

> Index: extensions/xforms/nsXFormsItemSetElement.cpp
> ===================================================================
> @@ -321,113 +321,73 @@

Please do -p diffs.

>    // XXX Possibly cleanup this when XBLizing <select>.

Remove the comment, if you feel the job's done :)

> +++ extensions/xforms/resources/content/select.xml	2005-08-05 17:05:09.000000000 -0500

I would love to see some general comments for both controls.

Your patch does not seem to support the styling in attachment 190004 [details]?

Comment 4

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

(In reply to comment #3)
> * Shouldn't this inherit from nsXFormsDelegateStub? This goes for most of the
> controls.
> 

Why? <choices> is not exactly a form control. But see also Bug 303876.

Comment 5

[Doron Rosenberg (IBM)]

> Your patch does not seem to support the styling in attachment 190004 [details] [edit]?

I am using a regular html select, which does not support that kind of stuff.  If
we want to support that, I'd have to do a custom widget like smaug did for select1. 

Comment 6

[Doron Rosenberg (IBM)]

Attachment: more cleanup, removed some more useless code.

Right now, appearance full is a table, rather that like Novell, where each item
is inline.

Comment 7

[Andrew Douglas]

Wow - looks good.. okay, so all I've seen so far are three typo's in one comment.. 

+  (html:select for regulat select and checkboxes for appearanc="full" selects.

- no need for the (
- regula_r_
- appearanc_e_

I'll yell if I see anything else. Thanks!

-Andrew

Comment 8

[Allan Beaufour]

(In reply to comment #5)
> > Your patch does not seem to support the styling in attachment 190004 [details] [edit]
[edit]?
> 
> I am using a regular html select, which does not support that kind of stuff.  If
> we want to support that, I'd have to do a custom widget like smaug did for
select1. 

We support it in the current select, so we would be losing functionality
otherwise. Would also be weird to support it in select1 and not in select, or?

Comment 9

[Doron Rosenberg (IBM)]

(In reply to comment #8)
> (In reply to comment #5)
> > > Your patch does not seem to support the styling in attachment 190004 [details]
[edit] [edit]
> [edit]?
> > 
> > I am using a regular html select, which does not support that kind of stuff.  If
> > we want to support that, I'd have to do a custom widget like smaug did for
> select1. 
> 
> We support it in the current select, so we would be losing functionality
> otherwise. Would also be weird to support it in select1 and not in select, or?
> 
> 

It worked in the xtf select?  I'll take a look.  Note novell has the exact same
behavior - select1 works, not select, since they too use html:select for regular
select.

Comment 10

[Doron Rosenberg (IBM)]

(In reply to comment #8)
> (In reply to comment #5)
> > > Your patch does not seem to support the styling in attachment 190004 [details]
[edit] [edit]
> [edit]?
> > 
> > I am using a regular html select, which does not support that kind of stuff.  If
> > we want to support that, I'd have to do a custom widget like smaug did for
> select1. 
> 
> We support it in the current select, so we would be losing functionality
> otherwise. Would also be weird to support it in select1 and not in select, or?
> 
> 

Is this per spec? The schema for XForms says no.

Comment 11

[Allan Beaufour]

(In reply to comment #10)
> Is this per spec? The schema for XForms says no.

label is UI.Inline, which has:
<!--
 containing document language to add additional allowed content here 
-->

Comment 12

[Doron Rosenberg (IBM)]

Attachment: added support for html elements in label for select children

Comment 13

[Doron Rosenberg (IBM)]

Comment on attachment 192215 [details] [diff] [review]
added support for html elements in label for select children

review comments welcome

Comment 14

[aaronr]

Comment on attachment 192215 [details] [diff] [review]
added support for html elements in label for select children

>? extensions/xforms/attachment.cgi?id=191499
>? extensions/xforms/resources/content/select.xml
>Index: extensions/xforms/jar.mn
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/jar.mn,v
>retrieving revision 1.6
>diff -p -u -1 -0 -r1.6 jar.mn
>--- extensions/xforms/jar.mn	3 Aug 2005 18:43:43 -0000	1.6
>+++ extensions/xforms/jar.mn	10 Aug 2005 15:51:53 -0000
>@@ -2,14 +2,15 @@ xforms.jar:
> % overlay	chrome://browser/content/preferences/preferences.xul	chrome://xforms/content/xforms-prefs.xul
> % content	xforms	%content/xforms/
> % locale	xforms	en-US	%locale/en-US/xforms/
>   content/xforms/contents.rdf                  (resources/content/contents.rdf)
> * content/xforms/xforms.css                    (resources/content/xforms.css)
> * content/xforms/xforms-prefs.xul              (resources/content/xforms-prefs.xul)
> * content/xforms/xforms-prefs-ui.xul           (resources/content/xforms-prefs-ui.xul)
> * content/xforms/xforms-prefs.js               (resources/content/xforms-prefs.js)
>   content/xforms/xforms.xml                    (resources/content/xforms.xml)
>   content/xforms/select1.xml                   (resources/content/select1.xml)
>+  content/xforms/select.xml                   (resources/content/select.xml)

nit: resources/content/... not lined up with others


> 
> NS_IMETHODIMP
> nsXFormsChoicesElement::ChildRemoved(PRUint32 aIndex)
> {
>-  // TODO: should remove the anonymous content owned by the removed item.
>-

this TODO comment doesn't still apply?


> NS_IMETHODIMP
> nsXFormsChoicesElement::GetAnonymousNodes(nsIArray **aNodes)
> {
>-  nsCOMPtr<nsIMutableArray> array;
>-  nsresult rv = NS_NewArray(getter_AddRefs(array));
>-  NS_ENSURE_SUCCESS(rv, rv);
>-
>-  array->AppendElement(mOptGroup, PR_FALSE);
>-
>-  NS_ADDREF(*aNodes = array);
>   return NS_OK;
> }
> 

Could you look at the other GetAnonymousNodes in the XForms code?  As far as I
can tell, we don't need the method anymore.  Can probably get removed from the
interface, too.


> // internal methods
> 
> NS_IMETHODIMP
> nsXFormsChoicesElement::Refresh()
> {
>-  if (mInsideSelect1) {
>-    // <select1> is XBLized, so there is no need to recreate UI -
>-    // XBL will do it for us.
>-    return NS_OK;
>-  }
>-
>-  // Remove any existing first child that is not an option, to clear out
>-  // any existing label.
>-  nsCOMPtr<nsIDOMNode> firstChild, child2;
>-  nsresult rv = mOptGroup->GetFirstChild(getter_AddRefs(firstChild));
>-  NS_ENSURE_SUCCESS(rv, rv);
>-

I don't see mOptGroup being used anywhere anymore.  Can you get rid of it?

> NS_IMETHODIMP
> nsXFormsItemElement::OnCreated(nsIXTFBindableElementWrapper *aWrapper)
> {
>@@ -128,50 +122,32 @@ nsXFormsItemElement::OnCreated(nsIXTFBin
>   nsCOMPtr<nsIDOMElement> node;
>   aWrapper->GetElementNode(getter_AddRefs(node));
> 
>   // It's ok to keep a pointer to mElement.  mElement will have an
>   // owning reference to this object, so as long as we null out mElement in
>   // OnDestroyed, it will always be valid.
> 
>   mElement = node;
>   NS_ASSERTION(mElement, "Wrapper is not an nsIDOMElement, we'll crash soon");
> 
>-  // Our anonymous content structure in <select> will look like this:
>-  //
>-  // <option/>   (mOption, also insertion point)
>-  //
>-  //XXX Remove when XBLizing <select>
>-
>-  nsCOMPtr<nsIDOMDocument> domDoc;
>-  node->GetOwnerDocument(getter_AddRefs(domDoc));
>-
>-  nsCOMPtr<nsIDOMElement> element;
>-  domDoc->CreateElementNS(NS_LITERAL_STRING(NS_NAMESPACE_XHTML),
>-                          NS_LITERAL_STRING("option"),
>-                          getter_AddRefs(element));
>-
>-  mOption = do_QueryInterface(element);
>-  NS_ENSURE_TRUE(mOption, NS_ERROR_FAILURE);
>-

Doesn't look like you need mOption anymore...do you?

>   return NS_OK;
> }
> 
> NS_IMETHODIMP
> nsXFormsItemElement::ParentChanged(nsIDOMElement *aNewParent)
> {
>-  mInsideSelect1 = PR_FALSE;
>   if (aNewParent) {
>     nsCOMPtr<nsIDOMNode> parent, tmp;
>     mElement->GetParentNode(getter_AddRefs(parent));
>     while (parent) {
>-      if (nsXFormsUtils::IsXFormsElement(parent, NS_LITERAL_STRING("select1"))) {
>-        mInsideSelect1 = PR_TRUE;
>+      if (nsXFormsUtils::IsXFormsElement(parent, NS_LITERAL_STRING("select1")) ||
>+          nsXFormsUtils::IsXFormsElement(parent, NS_LITERAL_STRING("select"))) {
>         break;
>       }
>       tmp.swap(parent);
>       tmp->GetParentNode(getter_AddRefs(parent));
>     }
> 
>     Refresh();
>   }
> 
>   return NS_OK;

without mInsideSelect1, no point to this logic, right?	Just have Refresh()
inside test for aNewParent and then the return.


>@@ -248,27 +224,20 @@ nsXFormsItemElement::DoneAddingChildren(
>   // Assume that we've got something worth refreshing now.
>   Refresh();
>   return NS_OK;
> }
> 
> // nsIXFormsSelectChild
> 
> NS_IMETHODIMP
> nsXFormsItemElement::GetAnonymousNodes(nsIArray **aNodes)
> {
>-  nsCOMPtr<nsIMutableArray> array;
>-  nsresult rv = NS_NewArray(getter_AddRefs(array));
>-  NS_ENSURE_SUCCESS(rv, rv);
>-
>-  array->AppendElement(mOption, PR_FALSE);
>-
>-  NS_ADDREF(*aNodes = array);
>   return NS_OK;
> }
> 

Like I mentioned above, I don't see where GetAnonymousNodes is used anymore.


> NS_IMETHODIMP
> nsXFormsItemElement::SelectItemByValue(const nsAString &aValue, nsIDOMNode **aSelected)
> {
>   NS_ENSURE_ARG_POINTER(aSelected);
>   NS_ENSURE_STATE(mElement);
>   nsAutoString value;
>   GetValue(value);
>@@ -277,72 +246,40 @@ nsXFormsItemElement::SelectItemByValue(c
>   } else {
>     *aSelected = nsnull;
>   }
> 
>   return NS_OK;
> }
> 
> NS_IMETHODIMP
> nsXFormsItemElement::SelectItemsByValue(const nsStringArray &aValueList)
> {
>-  nsAutoString value;
>-  nsresult rv = GetValue(value);
>-  NS_ENSURE_SUCCESS(rv, rv);
>-
>-  PRInt32 count = aValueList.Count();
>-  for (PRInt32 i = 0; i < count; ++i) {
>-    if (aValueList[i]->Equals(value)) {
>-      mOption->SetSelected(PR_TRUE);
>-      break;
>-    }
>-  }
>-
>   return NS_OK;
> }

Doesn't look like this interface is used anymore by anyone.  Can it be removed
from here (and itemset and choices)?

> 
> NS_IMETHODIMP
> nsXFormsItemElement::SelectItemsByContent(nsIDOMNode *aNode)
> {
>-  // TODO: implement support for \<copy\> element.
>   return NS_OK;
> }
> 

Doesn't look like this interface is used anymore by anyone.  Can it be removed
from here (and itemset and choices)?

> NS_IMETHODIMP
> nsXFormsItemElement::WriteSelectedItems(nsIDOMNode *aContainer, 
>                                         nsAString  &aStringBuffer)
> {
>-  PRBool selected;
>-  mOption->GetSelected(&selected);
>-  if (!selected)
>-    return NS_OK;
>-
>-  nsAutoString value;
>-  nsresult rv = GetValue(value);
>-  NS_ENSURE_SUCCESS(rv, rv);
>-
>-  if(aStringBuffer.IsEmpty()){
>-    aStringBuffer.Append(value);
>-  }
>-  else{
>-    aStringBuffer.Append(NS_LITERAL_STRING(" ") + value);
>-  }
>-
>   return NS_OK;
> }
> 

Doesn't look like this interface is used anymore by anyone.  Can it be removed
from here (and itemset and choices)?

>Index: extensions/xforms/nsXFormsItemSetElement.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsItemSetElement.cpp,v
>retrieving revision 1.8
>diff -p -u -1 -0 -r1.8 nsXFormsItemSetElement.cpp
>--- extensions/xforms/nsXFormsItemSetElement.cpp	10 Aug 2005 07:58:29 -0000	1.8
>+++ extensions/xforms/nsXFormsItemSetElement.cpp	10 Aug 2005 15:51:54 -0000
>@@ -314,130 +314,89 @@ nsXFormsItemSetElement::Refresh()
>     templateNodes->GetLength(&templateNodeCount);
>   
>   nsCOMPtr<nsIDOMDocument> document;
>   mElement->GetOwnerDocument(getter_AddRefs(document));
>   if (!document)
>     return NS_OK;

Even though this isn't part of your change, could you change this, please? 
GetOwnerDocument earlier in ::Refresh so we should just use that value instead
of getting it again.

> 
>   PRUint32 nodeCount;
>   result->GetSnapshotLength(&nodeCount);
> 
>-  // XXX Possibly cleanup this when XBLizing <select>.
>   nsCOMPtr<nsIDOMNode> parent, tmp;
>   mElement->GetParentNode(getter_AddRefs(parent));
>-  PRBool parentIsSelect1 = PR_FALSE;
>-  while (parent &&
>-    !nsXFormsUtils::IsXFormsElement(parent, NS_LITERAL_STRING("select"))) {
>-    if (nsXFormsUtils::IsXFormsElement(parent, NS_LITERAL_STRING("select1"))) {
>-      parentIsSelect1 = PR_TRUE;
>+
>+  while (parent) {
>+    if (nsXFormsUtils::IsXFormsElement(parent, NS_LITERAL_STRING("select1")) ||
>+        nsXFormsUtils::IsXFormsElement(parent, NS_LITERAL_STRING("select"))) {
>       break;
>     }
>     tmp.swap(parent);
>     tmp->GetParentNode(getter_AddRefs(parent));
>   }

what happens if there isn't an ancestor that is a select or select1?  Should we
keep going anyhow?  Until schema is ready to do the form validation, this can't
be guaranteed.	So if it is necessary to have this to go on, should probably
check.

>Index: extensions/xforms/nsXFormsSelectElement.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsSelectElement.cpp,v
>retrieving revision 1.11
>diff -p -u -1 -0 -r1.11 nsXFormsSelectElement.cpp
>--- extensions/xforms/nsXFormsSelectElement.cpp	7 Jun 2005 07:14:31 -0000	1.11
>+++ extensions/xforms/nsXFormsSelectElement.cpp	10 Aug 2005 15:51:55 -0000
>@@ -14,503 +14,134 @@


>+  // nsIXFormsDelegate
>+  NS_IMETHOD SetValue(const nsAString& aValue);
> #ifdef DEBUG_smaug
>-  virtual const char* Name() { return "select"; }
>+  virtual const char* Name() { return "select1"; }
> #endif

shouldn't this return "select" and not "select1"?


>--- /dev/null	2005-07-06 06:24:31.632732624 -0500
>+++ extensions/xforms/resources/content/select.xml	2005-08-10 

>+<!--
>+  XBL select parses the childnodes of the xforms:select and then constructs
>+  the anonymous content programmatically.
>+
>+  The main _buildSelect() method assumes that each select implementation
>+  has three methods: _addSelectItem (for xf:item), _handleChoices (for xf:choice,
>+  which can be nested) and _addItemSetItem for itemsets.
>+
>+  _controlArray is used to store each generated selectable item in the UI
>+  (html:select for regulat select and checkboxes for appearanc="full" selects.
>+-->

nit: spelling.	s/regulat/regular s/appearnac/appearance



>+          // check if any default values where not found
>+          for (var index in defaultHash) {
>+            if (defaultHash[index].hits == 0) {
>+              // XXX: some of default values not found, we need to throw an event.
>+            }
>+          }

nit: spelling.	s/where/were.  In your XXX comment, you should probably mention
that the event that should be thrown is xforms-out-of-range, but that it only
applies to 'closed' select elements.  And that for 'open' select elements the
element should be added to the select element (and selected) as per 8.1.10 in
the spec.  That way no one is left wondering exactly what you are talking
about.


** I'll finish the rest of my review later tonight or tomorrow **

Comment 15

[Doron Rosenberg (IBM)]

> > NS_IMETHODIMP
> > nsXFormsChoicesElement::ChildRemoved(PRUint32 aIndex)
> > {
> >-  // TODO: should remove the anonymous content owned by the removed item.
> >-
>
> this TODO comment doesn't still apply?

I don't think that is needed anymore, perhaps smaug knows if we need to address
it?  xbl select doesn't, since refresh will cause a rebuild of the generated
content.
> 
> 
> > NS_IMETHODIMP
> > nsXFormsChoicesElement::GetAnonymousNodes(nsIArray **aNodes)
> > {
> >-  nsCOMPtr<nsIMutableArray> array;
> >-  nsresult rv = NS_NewArray(getter_AddRefs(array));
> >-  NS_ENSURE_SUCCESS(rv, rv);
> >-
> >-  array->AppendElement(mOptGroup, PR_FALSE);
> >-
> >-  NS_ADDREF(*aNodes = array);
> >   return NS_OK;
> > }
> > 
> 
> Could you look at the other GetAnonymousNodes in the XForms code?  As far as I
> can tell, we don't need the method anymore.  Can probably get removed from the
> interface, too.

smaug needs to have a say, I am not sure.

> > NS_IMETHODIMP
> > nsXFormsItemElement::SelectItemsByValue(const nsStringArray &aValueList)
> > {
> >-  nsAutoString value;
> >-  nsresult rv = GetValue(value);
> >-  NS_ENSURE_SUCCESS(rv, rv);
> >-
> >-  PRInt32 count = aValueList.Count();
> >-  for (PRInt32 i = 0; i < count; ++i) {
> >-    if (aValueList[i]->Equals(value)) {
> >-      mOption->SetSelected(PR_TRUE);
> >-      break;
> >-    }
> >-  }
> >-
> >   return NS_OK;
> > }
> 
> Doesn't look like this interface is used anymore by anyone.  Can it be removed
> from here (and itemset and choices)?

smaug: is that true?

> > 
> > NS_IMETHODIMP
> > nsXFormsItemElement::SelectItemsByContent(nsIDOMNode *aNode)
> > {
> >-  // TODO: implement support for \<copy\> element.
> >   return NS_OK;
> > }
> > 
> 
> Doesn't look like this interface is used anymore by anyone.  Can it be removed
> from here (and itemset and choices)?
> 

smaug: is that true?

> > NS_IMETHODIMP
> > nsXFormsItemElement::WriteSelectedItems(nsIDOMNode *aContainer, 
> >                                         nsAString  &aStringBuffer)
> > {
> >-  PRBool selected;
> >-  mOption->GetSelected(&selected);
> >-  if (!selected)
> >-    return NS_OK;
> >-
> >-  nsAutoString value;
> >-  nsresult rv = GetValue(value);
> >-  NS_ENSURE_SUCCESS(rv, rv);
> >-
> >-  if(aStringBuffer.IsEmpty()){
> >-    aStringBuffer.Append(value);
> >-  }
> >-  else{
> >-    aStringBuffer.Append(NS_LITERAL_STRING(" ") + value);
> >-  }
> >-
> >   return NS_OK;
> > }
> > 
> 
> Doesn't look like this interface is used anymore by anyone.  Can it be removed
> from here (and itemset and choices)?

smaug: is that true?

> >+  while (parent) {
> >+    if (nsXFormsUtils::IsXFormsElement(parent, NS_LITERAL_STRING("select1")) ||
> >+        nsXFormsUtils::IsXFormsElement(parent, NS_LITERAL_STRING("select"))) {
> >       break;
> >     }
> >     tmp.swap(parent);
> >     tmp->GetParentNode(getter_AddRefs(parent));
> >   }
> 
> what happens if there isn't an ancestor that is a select or select1?  Should we
> keep going anyhow?  Until schema is ready to do the form validation, this can't
> be guaranteed.	So if it is necessary to have this to go on, should probably
> check.

All we use parent now is for calling refresh, so if it isn't a select/select1,
nothing bad should happen.

Comment 16

[aaronr]

(In reply to comment #15)
> > >+  while (parent) {
> > >+    if (nsXFormsUtils::IsXFormsElement(parent,
NS_LITERAL_STRING("select1")) ||
> > >+        nsXFormsUtils::IsXFormsElement(parent, NS_LITERAL_STRING("select"))) {
> > >       break;
> > >     }
> > >     tmp.swap(parent);
> > >     tmp->GetParentNode(getter_AddRefs(parent));
> > >   }
> > 
> > what happens if there isn't an ancestor that is a select or select1?  Should we
> > keep going anyhow?  Until schema is ready to do the form validation, this can't
> > be guaranteed.	So if it is necessary to have this to go on, should probably
> > check.
> 
> All we use parent now is for calling refresh, so if it isn't a select/select1,
> nothing bad should happen.
> 
> 

You actually use parent to call ChildAppended() on the parent which will do who
knows what if it isn't a select or a select1.  At least make sure that parent is
a nsIXFormsControl, please.

Comment 17

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

Comment on attachment 192215 [details] [diff] [review]
added support for html elements in label for select children

> class nsXFormsChoicesElement : public nsXFormsBindableStub,
>                                public nsIXFormsSelectChild
> {
> public:
>   nsXFormsChoicesElement() : mElement(nsnull) {}
> 
>   NS_DECL_ISUPPORTS_INHERITED
> 
>   NS_IMETHOD OnCreated(nsIXTFBindableElementWrapper *aWrapper);
> 
>@@ -74,87 +68,55 @@ public:
> 
>   // nsIXFormsControlBase overrides
>   NS_IMETHOD Refresh();

Not needed? And does the class actually implement nsIXFormsControlBase?

>   nsCOMPtr<nsIDOMHTMLOptGroupElement> mOptGroup;

Remove.

> 
> NS_IMETHODIMP
> nsXFormsChoicesElement::ParentChanged(nsIDOMElement *aNewParent)
> {

Couldn't we remove this method. Or better, if parent is changed after
doneAddingChildren, 
we should call the containing select/select1's Refresh().

> 
> // nsIXFormsSelectChild
> 
> NS_IMETHODIMP
> nsXFormsChoicesElement::GetAnonymousNodes(nsIArray **aNodes)
> {
>-  nsCOMPtr<nsIMutableArray> array;
>-  nsresult rv = NS_NewArray(getter_AddRefs(array));
>-  NS_ENSURE_SUCCESS(rv, rv);
>-
>-  array->AppendElement(mOptGroup, PR_FALSE);
>-
>-  NS_ADDREF(*aNodes = array);
>   return NS_OK;
> }

Not needed.


> NS_IMETHODIMP
> nsXFormsChoicesElement::Refresh()
> {
>-  if (mInsideSelect1) {
>-    // <select1> is XBLized, so there is no need to recreate UI -
>-    // XBL will do it for us.
>-    return NS_OK;
>-  }
>-
>-  // Remove any existing first child that is not an option, to clear out
>-  // any existing label.
>-  nsCOMPtr<nsIDOMNode> firstChild, child2;
>-  nsresult rv = mOptGroup->GetFirstChild(getter_AddRefs(firstChild));
>-  NS_ENSURE_SUCCESS(rv, rv);
>-
>-  nsAutoString value;
>-
>-  if (firstChild) {
>-    firstChild->GetLocalName(value);
>-    if (!value.EqualsLiteral("option")) {
>-      mOptGroup->RemoveChild(firstChild, getter_AddRefs(child2));
>-      mOptGroup->GetFirstChild(getter_AddRefs(firstChild));
>-    }
>-  }
>-
>-  // We need to find our label element and clone it into the optgroup.
>-  // This content will appear before any of the optgroup's options.
>-
>-  nsCOMPtr<nsIDOMNodeList> children;
>-  rv = mElement->GetChildNodes(getter_AddRefs(children));
>-  NS_ENSURE_SUCCESS(rv, rv);
>-
>-  PRUint32 childCount;
>-  children->GetLength(&childCount);
>-
>-  nsCOMPtr<nsIDOMNode> child;
>-  nsAutoString labelValue;
>-
>-  for (PRUint32 i = 0; i < childCount; ++i) {
>-    children->Item(i, getter_AddRefs(child));
>-    if (nsXFormsUtils::IsLabelElement(child)) {
>-      // Clone our label into the optgroup
>-      child->CloneNode(PR_TRUE, getter_AddRefs(child2));
>-      mOptGroup->InsertBefore(child2, firstChild, getter_AddRefs(child));
>-    }
>-  }
>-
>   return NS_OK;
> }

So does this method do something?


> class nsXFormsItemElement : public nsXFormsBindableStub,
>                             public nsIXFormsSelectChild,
>                             public nsIXFormsItemElement
> {
...
>   nsCOMPtr<nsIDOMHTMLOptionElement> mOption;

Remove mOption.


> NS_IMETHODIMP
> nsXFormsItemElement::GetAnonymousNodes(nsIArray **aNodes)
> {
>-  nsCOMPtr<nsIMutableArray> array;
>-  nsresult rv = NS_NewArray(getter_AddRefs(array));
>-  NS_ENSURE_SUCCESS(rv, rv);
>-
>-  array->AppendElement(mOption, PR_FALSE);
>-
>-  NS_ADDREF(*aNodes = array);
>   return NS_OK;
> }

Not needed.


> NS_IMETHODIMP
> nsXFormsItemElement::SelectItemsByValue(const nsStringArray &aValueList)
> {
>-  nsAutoString value;
>-  nsresult rv = GetValue(value);
>-  NS_ENSURE_SUCCESS(rv, rv);
>-
>-  PRInt32 count = aValueList.Count();
>-  for (PRInt32 i = 0; i < count; ++i) {
>-    if (aValueList[i]->Equals(value)) {
>-      mOption->SetSelected(PR_TRUE);
>-      break;
>-    }
>-  }
>-
>   return NS_OK;
> }

select1 doesn't need this, and if it is not needed by <select> then remove the
whole method.

> 
> NS_IMETHODIMP
> nsXFormsItemElement::SelectItemsByContent(nsIDOMNode *aNode)
> {
>-  // TODO: implement support for \<copy\> element.
>   return NS_OK;
> }
> 
> NS_IMETHODIMP
> nsXFormsItemElement::WriteSelectedItems(nsIDOMNode *aContainer, 
>                                         nsAString  &aStringBuffer)
> {
>-  PRBool selected;
>-  mOption->GetSelected(&selected);
>-  if (!selected)
>-    return NS_OK;
>-
>-  nsAutoString value;
>-  nsresult rv = GetValue(value);
>-  NS_ENSURE_SUCCESS(rv, rv);
>-
>-  if(aStringBuffer.IsEmpty()){
>-    aStringBuffer.Append(value);
>-  }
>-  else{
>-    aStringBuffer.Append(NS_LITERAL_STRING(" ") + value);
>-  }
>-
>   return NS_OK;
> }

If this is not needed by <select> then remove it.

> 
> NS_IMETHODIMP
> nsXFormsItemElement::SetContext(nsIDOMElement *aContextNode,
>                                 PRInt32 aContextPosition, PRInt32 aContextSize)
> {
>-  mContextNode = aContextNode;
>-  mContextPosition = aContextPosition;
>-  mContextSize = aContextSize;
>   return NS_OK;
> }

Is SetContext needed anymore?

> 
> NS_IMETHODIMP
> nsXFormsItemElement::Refresh()
> {
>-  if (mInsideSelect1) {
>-    // <select1> is XBLized, so there is no need to recreate UI -
>-    // XBL will do it for us.
>-    return NS_OK;
>-  }
>-
>-  // Clear out all of the existing option text
>-  nsCOMPtr<nsIDOMNode> child, child2;
>-  while (NS_SUCCEEDED(mOption->GetFirstChild(getter_AddRefs(child))) && child)
>-    mOption->RemoveChild(child, getter_AddRefs(child2));
>-    
>-  // We need to find our label child, serialize its contents
>-  // (which can be xtf-generated), and set that as our label attribute.
>-
>-  nsCOMPtr<nsIDOMNodeList> children;
>-  nsresult rv = mElement->GetChildNodes(getter_AddRefs(children));
>-  NS_ENSURE_SUCCESS(rv, rv);
>-
>-  PRUint32 childCount;
>-  children->GetLength(&childCount);
>-
>-  nsAutoString value;
>-
>-  nsCOMPtr<nsIDOMDocument> doc;
>-  mElement->GetOwnerDocument(getter_AddRefs(doc));
>-
>-  nsCOMPtr<nsIDOMElement> container;
>-  doc->CreateElementNS(NS_LITERAL_STRING(NS_NAMESPACE_XFORMS),
>-                       NS_LITERAL_STRING("contextcontainer-inline"),
>-                       getter_AddRefs(container));
>-
>-  NS_NAMED_LITERAL_STRING(modelStr, "model");
>-
>-  nsAutoString modelID;
>-  mElement->GetAttribute(modelStr, modelID);
>-  container->SetAttribute(modelStr, modelID);
>-
>-  nsCOMPtr<nsIXFormsContextControl> context = do_QueryInterface(container);
>-  context->SetContext(mContextNode, 1, 1);
>-
>-  for (PRUint32 i = 0; i < childCount; ++i) {
>-    children->Item(i, getter_AddRefs(child));
>-    child->GetLocalName(value);
>-    if (value.EqualsLiteral("label")) {
>-      child->GetNamespaceURI(value);
>-      if (value.EqualsLiteral(NS_NAMESPACE_XFORMS)) {
>-        child->CloneNode(PR_TRUE, getter_AddRefs(child2));
>-        container->AppendChild(child2, getter_AddRefs(child));
>-      }
>-    }
>-  }
>-
>-  nsCOMPtr<nsIDOMNode> childReturn;
>-  mOption->AppendChild(container, getter_AddRefs(childReturn));
>-
>   return NS_OK;
> }

So why do you need Refresh anymore?

>@@ -507,21 +389,22 @@ nsXFormsItemElement::LabelRefreshed()
>   // refreshes.
>   if (doc && doc->GetProperty(nsXFormsAtoms::deferredBindListProperty)) {
>     return NS_OK;
>   }
> 
>   NS_ENSURE_STATE(mElement);
>   nsCOMPtr<nsIDOMNode> parent, current;
>   current = mElement;
>   do {
>     current->GetParentNode(getter_AddRefs(parent));
>-    if (nsXFormsUtils::IsXFormsElement(parent, NS_LITERAL_STRING("select1"))) {
>+    if (nsXFormsUtils::IsXFormsElement(parent, NS_LITERAL_STRING("select1")) ||
>+        nsXFormsUtils::IsXFormsElement(parent, NS_LITERAL_STRING("select"))) {
>       nsCOMPtr<nsIXFormsControl> select1(do_QueryInterface(parent));
>       if (select1) {
>         select1->Refresh();
>       }
>       return NS_OK;
>     }
>     current = parent;
>   } while(current);
>   return NS_OK;
> }

Change the name of the variable select1.



>Index: extensions/xforms/nsXFormsItemSetElement.cpp

Not yet reviewed.


> #ifdef DEBUG_smaug
>-  virtual const char* Name() { return "select"; }
>+  virtual const char* Name() { return "select1"; }

"select"

>-select1 itemset {
>+select1 itemset,
>+select itemset {
>   -moz-binding: url('chrome://xforms/content/select1.xml#xformswidget-select1-itemset');
> }

If you use the same binding also for <select>, could you rename the binding.
Perhaps xformswidget-itemset


>+/* select */
>+select {
>+  -moz-binding: url('chrome://xforms/content/select.xml#xformswidget-select');
>+}
>+
>+select[appearance="full"] {
>+  -moz-binding: url('chrome://xforms/content/select.xml#xformswidget-select-full');
>+}
>+
>+select html|div.select-choice-label {
>+  font: -moz-list;
>+  font-style: italic;
>+  font-weight: bold;

Should this have display:inherit?

>+}
>+
>+select html|div.select-choice-content {
>+  padding-left: 10px;

Should this have display:inherit?



>+          var selectedArray = this.delegate.value.split(" ");

Is " " enough, should there be also \n and \t?

>+          // itemsets needs to use aLabel and not the label element contents
>+          if (aUseLabelValue) {
>+            option.textContent = aLabel;
>+          } else {
>+            // label can contain other elements such as html, so we import it
>+            var label = aControl.getElementsByTagName("label")[0];
>+            var newLabel = document.importNode(label, true);

What is this? "*importNode* Imports a node from another document to this
document."
Do you mean cloneNode?


>+
>+ <!-- SELECT: <FULL> -->
>+  <binding id="xformswidget-select-full"
>+           extends="chrome://xforms/content/select.xml#xformswidget-select">
>+    <content>
>+      <html:table xbl:inherits="style">

Inheriting style attribute is not according to the spec, but is actully ok to
me, I think.

Comment 18

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

forget those "Should this have display:inherit?" comments.

Comment 19

[aaronr]

Comment on attachment 192215 [details] [diff] [review]
added support for html elements in label for select children


>--- /dev/null	2005-07-06 06:24:31.632732624 -0500
>+++ extensions/xforms/resources/content/select.xml	2005-08-10 09:32:14.000000000 -0500
>@@ -0,0 +1,635 @@
>+<?xml version="1.0"?>
>+
>+<!-- ***** BEGIN LICENSE BLOCK *****
>+   - Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+   -
>+   - The contents of this file are subject to the Mozilla Public License Version
>+   - 1.1 (the "License"); you may not use this file except in compliance with
>+   - the License. You may obtain a copy of the License at
>+   - http://www.mozilla.org/MPL/
>+   -
>+   - Software distributed under the License is distributed on an "AS IS" basis,
>+   - WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+   - for the specific language governing rights and limitations under the
>+   - License.
>+   -
>+   - The Original Code is Mozilla XForms support.
>+   -
>+   - The Initial Developer of the Original Code is
>+   - IBM Corporation.
>+   - Portions created by the Initial Developer are Copyright (C) 2005
>+   - the Initial Developer. All Rights Reserved.
>+   -
>+   - Contributor(s):
>+   -  Doron Rosenberg <[e-mail]>
>+   -
>+   - Alternatively, the contents of this file may be used under the terms of
>+   - either the GNU General Public License Version 2 or later (the "GPL"), or
>+   - the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>+   - in which case the provisions of the GPL or the LGPL are applicable instead
>+   - of those above. If you wish to allow use of your version of this file only
>+   - under the terms of either the GPL or the LGPL, and not to allow others to
>+   - use your version of this file under the terms of the MPL, indicate your
>+   - decision by deleting the provisions above and replace them with the notice
>+   - and other provisions required by the GPL or the LGPL. If you do not delete
>+   - the provisions above, a recipient may use your version of this file under
>+   - the terms of any one of the MPL, the GPL or the LGPL.
>+   -
>+   - ***** END LICENSE BLOCK ***** -->
>+
>+<!--
>+  XBL select parses the childnodes of the xforms:select and then constructs
>+  the anonymous content programmatically.
>+
>+  The main _buildSelect() method assumes that each select implementation
>+  has three methods: _addSelectItem (for xf:item), _handleChoices (for xf:choice,
>+  which can be nested) and _addItemSetItem for itemsets.
>+
>+  _controlArray is used to store each generated selectable item in the UI
>+  (html:select for regulat select and checkboxes for appearanc="full" selects.
>+-->

Could you put comments here about what the anonymous content looks like for
each of the various flavors of select?	That if someone is debugging a form we
can look in here to see what we should expect to find for a select element in
the DOMInspector, for example.	Like the XTF files used to have in them.  Also,
what if it is appearance="full"?  How does that change the anonymous content. 
Choices, item, itemset...what does that anon content look like?



>+      <method name="_buildSelectItem">
>+        <parameter name="aLabel"/>
>+        <parameter name="aValue"/>
>+        <parameter name="aControl"/>
>+        <parameter name="aUseLabelValue"/>
>+        <body>
>+        <![CDATA[

Like this method, while you are doing a pretty good job commenting the method
as it goes along, there is no comment at the start of each method as to what
each method does.  And some of the methods are kinda similar to others, so it'd
be nice if there was a quick blurb on each method saying what it does and/or
how it fits in, what the parameters mean if it isn't obvious, etc.


>+
>+      <method name="_buildSelectItem">
>+        <parameter name="aLabel"/>
>+        <parameter name="aValue"/>
>+        <parameter name="aControl"/>
>+        <parameter name="aUseLabelValue"/>
>+        <body>
>+        <![CDATA[
>+          var div = document.createElementNS("http://www.w3.org/1999/xhtml",
>+                                             "div");
>+
>+          var labelSpan = document.createElementNS("http://www.w3.org/1999/xhtml",
>+                                                   "span");
>+

Looks like to me that you could simplify the calling of this function if you
want, by only taking an aControl and aUseLabelValue as parameters.  From what I
can tell, you could just take the aLabel and aValue from the control right here
inside this function rather than making the caller provide it to you.

That is the last of my review comments.  Looks really good.  I can't wait to
get it in the build!

Comment 20

[Doron Rosenberg (IBM)]

>>+          var selectedArray = this.delegate.value.split(" ");
>Is " " enough, should there be also \n and \t?

Hmm, perhaps you are right.  Is that per spec?

Comment 21

[Doron Rosenberg (IBM)]

>Looks like to me that you could simplify the calling of this function if you
>want, by only taking an aControl and aUseLabelValue as parameters.  From what I
>can tell, you could just take the aLabel and aValue from the control right here
>inside this function rather than making the caller provide it to you.

I need the value for defualt selection, and I decided that doing it in the
addItem() methods isn't correct, so I need to pass in label/value and then use
the value for that.  Else I'd have to do 2 QI's, and that ain't cheap.

Comment 22

[Doron Rosenberg (IBM)]

Attachment: with comments addressed

Comment 23

[aaronr]

Comment on attachment 192989 [details] [diff] [review]
with comments addressed

could still use some comments for what the more complicated methods do.  With
that, r=me

Comment 24

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

Doron, have you figured out what causes the problems when <select>
is used inside <repeat>?

(testcase http://www.cs.helsinki.fi/u/pettay/moztests/xforms/nested_select.xhtml)

Comment 25

[Doron Rosenberg (IBM)]

(In reply to comment #24)
> Doron, have you figured out what causes the problems when <select>
> is used inside <repeat>?
> 
> (testcase http://www.cs.helsinki.fi/u/pettay/moztests/xforms/nested_select.xhtml)

That testcase is invalid actually - you have spaces in the values (like "ItemB4
value") - so if you choose one, it thinks there are 2 values and everything is
unselected.

There is a issue still if the spaces are removed, which I believe has to do with
the cloning code in repeat.  I think perhaps to get select in and then figure
out the issue (I am guessing I might have to xblize repeat).

Comment 26

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

(In reply to comment #25)
> (In reply to comment #24)
> > Doron, have you figured out what causes the problems when <select>
> > is used inside <repeat>?
> > 
> > (testcase
http://www.cs.helsinki.fi/u/pettay/moztests/xforms/nested_select.xhtml)
> 
> That testcase is invalid actually - you have spaces in the values (like "ItemB4
> value") - so if you choose one, it thinks there are 2 values and everything is
> unselected.

Ah, of course. Sorry about that, I just modified the <select1> test by changing
<select1> to <select>.

Comment 27

[Doron Rosenberg (IBM)]

The remaining issue with repeat is (I believe) that we create 3 html:selects per
xforms select, and that causes the additional items to show.  Not sure what is
going on though since anyonmous content doesn't show up in DOMI due to repeat
being xtf.  I'll probably try xblizing it.

Comment 28

[Doron Rosenberg (IBM)]

Attachment: fixes repeat issue

what this does is make sure refresh doesn't run while it is already running,
which is what is causing the select/repear problems.

Comment 29

[aaronr]

(In reply to comment #28)
> Created an attachment (id=193453) [edit]
> fixes repeat issue
> 
> what this does is make sure refresh doesn't run while it is already running,
> which is what is causing the select/repear problems.

What?  Why does refresh happen if it is already running?  It should be all
single threaded.  We need to fix why refresh is getting hit twice like that.

Comment 30

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

(In reply to comment #29)
> (In reply to comment #28)
> > Created an attachment (id=193453) [edit] [edit]
> > fixes repeat issue
> > 
> > what this does is make sure refresh doesn't run while it is already running,
> > which is what is causing the select/repear problems.
> 
> What?  Why does refresh happen if it is already running?  It should be all
> single threaded.  We need to fix why refresh is getting hit twice like that.

As far as I understand it is happening because <select> and <repeat> are
cloning nodes and that makes <label> to call labelRefreshed, which again
will call Refresh on <select>. I think (and hope ;) ) it is ok to have a flag to
prevent extra refreshes.

Comment 31

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

Comment on attachment 193453 [details] [diff] [review]
fixes repeat issue


>Index: extensions/xforms/nsXFormsItemElement.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsItemElement.cpp,v
>retrieving revision 1.10
>diff -u -i -r1.10 nsXFormsItemElement.cpp
>--- extensions/xforms/nsXFormsItemElement.cpp	10 Aug 2005 07:58:29 -0000	1.10
>+++ extensions/xforms/nsXFormsItemElement.cpp	22 Aug 2005 16:40:13 -0000
>@@ -69,10 +69,7 @@
>                             public nsIXFormsItemElement
> {
> public:
>-  nsXFormsItemElement() : mElement(nsnull),
>-                          mContextPosition(0),
>-                          mContextSize(0),
>-                          mInsideSelect1(PR_FALSE)
>+  nsXFormsItemElement() : mElement(nsnull), mDoneAddingChildren(PR_FALSE)
>   {
>   }
> 
>@@ -89,22 +86,17 @@
>   NS_IMETHOD BeginAddingChildren();
>   NS_IMETHOD DoneAddingChildren();
> 
>-  // nsIXFormsControlBase overrides
>-  NS_IMETHOD Refresh();
>+  void Refresh();
> 
>   // nsIXFormsSelectChild
>   NS_DECL_NSIXFORMSSELECTCHILD
> 
> private:
>-
>   nsIDOMElement* mElement;
>-  nsCOMPtr<nsIDOMHTMLOptionElement> mOption;
>+  PRBool         mDoneAddingChildren;
> 
>   // context node (used by itemset in select)
>   nsCOMPtr<nsIDOMElement>           mContextNode;

mContextNode is not used anymore. Remove it.





>-NS_IMETHODIMP
>-nsXFormsItemSetElement::SetContext(nsIDOMElement *aContextNode,
>-                                   PRInt32 aContextPosition,
>-                                   PRInt32 aContextSize)
>-{
>-  // itemset can't be inside another itemset, so we don't need to worry about
>-  // this.
>-  return NS_OK;
>-}
>-
> // internal methods
> 
> NS_IMETHODIMP
>@@ -312,125 +252,82 @@
>   PRUint32 templateNodeCount = 0;
>   if (templateNodes)
>     templateNodes->GetLength(&templateNodeCount);
>-  
>-  nsCOMPtr<nsIDOMDocument> document;
>-  mElement->GetOwnerDocument(getter_AddRefs(document));
>-  if (!document)
>+
>+  if (!domDoc)
>     return NS_OK;
> 
>   PRUint32 nodeCount;
>   result->GetSnapshotLength(&nodeCount);
> 
>-  // XXX Possibly cleanup this when XBLizing <select>.
>   nsCOMPtr<nsIDOMNode> parent, tmp;
>   mElement->GetParentNode(getter_AddRefs(parent));
>-  PRBool parentIsSelect1 = PR_FALSE;
>-  while (parent &&
>-    !nsXFormsUtils::IsXFormsElement(parent, NS_LITERAL_STRING("select"))) {
>-    if (nsXFormsUtils::IsXFormsElement(parent, NS_LITERAL_STRING("select1"))) {
>-      parentIsSelect1 = PR_TRUE;
>+
>+  while (parent) {
>+    if (nsXFormsUtils::IsXFormsElement(parent, NS_LITERAL_STRING("select1")) ||
>+        nsXFormsUtils::IsXFormsElement(parent, NS_LITERAL_STRING("select"))) {
>       break;
>     }
>     tmp.swap(parent);
>     tmp->GetParentNode(getter_AddRefs(parent));
>   }
>-  
>-  if (parentIsSelect1) {
>-    // For select1
>-    nsCOMPtr<nsIXFormsItemSetUIElement> uiItemSet(do_QueryInterface(mElement));
>-    nsCOMPtr<nsIDOMElement> anonContent;
>-    if (uiItemSet) {
>-      uiItemSet->GetAnonymousItemSetContent(getter_AddRefs(anonContent));
>-    }
> 
>-    NS_ENSURE_STATE(anonContent);
>+  nsCOMPtr<nsIXFormsItemSetUIElement> uiItemSet(do_QueryInterface(mElement));
>+  nsCOMPtr<nsIDOMElement> anonContent;
>+  if (uiItemSet) {
>+    uiItemSet->GetAnonymousItemSetContent(getter_AddRefs(anonContent));
>+  }
> 
>-    nsCOMPtr<nsIDOMNode> childNode, nodeReturn;
>-    while (NS_SUCCEEDED(anonContent->GetFirstChild(getter_AddRefs(childNode))) &&
>-         childNode) {
>-      anonContent->RemoveChild(childNode, getter_AddRefs(nodeReturn));
>-    }
>+  NS_ENSURE_STATE(anonContent);
> 
>-    for (PRUint32 i = 0; i < nodeCount; ++i) {
>-      result->SnapshotItem(i, getter_AddRefs(node));
>-      NS_ASSERTION(node, "incorrect snapshot length");
>-
>-      nsresult rv = document->CreateElementNS(NS_LITERAL_STRING(NS_NAMESPACE_XFORMS),
>-                                              NS_LITERAL_STRING("item"),
>-                                              getter_AddRefs(itemNode));
>-      NS_ENSURE_SUCCESS(rv, rv);
>-
>-      // XXX Could we get rid of the <contextcontainer>?
>-      rv = document->CreateElementNS(NS_LITERAL_STRING(NS_NAMESPACE_XFORMS),
>-                                     NS_LITERAL_STRING("contextcontainer"),
>-                                     getter_AddRefs(contextContainer));
>-
>-      NS_ENSURE_SUCCESS(rv, rv);
>-
>-      nsCOMPtr<nsIDOMElement> modelElement = do_QueryInterface(model);
>-      nsAutoString modelID;
>-      modelElement->GetAttribute(NS_LITERAL_STRING("id"), modelID);
>-
>-      contextContainer->SetAttribute(NS_LITERAL_STRING("model"), modelID);
>-
>-      nsCOMPtr<nsIXFormsContextControl> ctx(do_QueryInterface(contextContainer));
>-      if (ctx) {
>-        ctx->SetContext(nsCOMPtr<nsIDOMElement>(do_QueryInterface(node)),
>-                        i + 1, nodeCount);
>-      }
>-      // Clone the template content under the item
>-      for (PRUint32 j = 0; j < templateNodeCount; ++j) {
>-        templateNodes->Item(j, getter_AddRefs(templateNode));
>-        templateNode->CloneNode(PR_TRUE, getter_AddRefs(cloneNode));
>-        contextContainer->AppendChild(cloneNode, getter_AddRefs(templateNode));
>-      }
>+  nsCOMPtr<nsIDOMNode> childNode, nodeReturn;
>+  while (NS_SUCCEEDED(anonContent->GetFirstChild(getter_AddRefs(childNode))) &&
>+       childNode) {
>+    anonContent->RemoveChild(childNode, getter_AddRefs(nodeReturn));
>+  }
> 
>-      itemNode->AppendChild(contextContainer, getter_AddRefs(tmpNode));
>-      anonContent->AppendChild(itemNode, getter_AddRefs(tmpNode));
>-    }
>-  } else {
>-    for (PRUint32 i = 0; i < nodeCount; ++i) {
>-      result->SnapshotItem(i, getter_AddRefs(node));
>-      NS_ASSERTION(node, "incorrect snapshot length");
>-
>-      document->CreateElementNS(NS_LITERAL_STRING(NS_NAMESPACE_XFORMS),
>-                                NS_LITERAL_STRING("item"),
>-                                getter_AddRefs(itemNode));
>+  for (PRUint32 i = 0; i < nodeCount; ++i) {
>+    result->SnapshotItem(i, getter_AddRefs(node));
>+    NS_ASSERTION(node, "incorrect snapshot length");
> 
>-      if (!itemNode)
>-        return NS_OK;
>+    nsresult rv = domDoc->CreateElementNS(NS_LITERAL_STRING(NS_NAMESPACE_XFORMS),
>+                                          NS_LITERAL_STRING("item"),
>+                                          getter_AddRefs(itemNode));
>+    NS_ENSURE_SUCCESS(rv, rv);
> 
>-      nsCOMPtr<nsIDOMElement> modelElement = do_QueryInterface(model);
>-      nsAutoString modelID;
>-      modelElement->GetAttribute(NS_LITERAL_STRING("id"), modelID);
>-
>-      itemNode->SetAttribute(NS_LITERAL_STRING("model"), modelID);
>-
>-      nsCOMPtr<nsIXFormsSelectChild> item = do_QueryInterface(itemNode);
>-      NS_ASSERTION(item, "item must be a SelectChild!");
>-
>-      item->SetContext(nsCOMPtr<nsIDOMElement>(do_QueryInterface(node)),
>-                       i + 1, nodeCount);
>-
>-      // Clone the template content under the item
>-      for (PRUint32 j = 0; j < templateNodeCount; ++j) {
>-        templateNodes->Item(j, getter_AddRefs(templateNode));
>-        templateNode->CloneNode(PR_TRUE, getter_AddRefs(cloneNode));
>-        itemNode->AppendChild(cloneNode, getter_AddRefs(templateNode));
>-      }
>+    // XXX Could we get rid of the <contextcontainer>?
>+    rv = domDoc->CreateElementNS(NS_LITERAL_STRING(NS_NAMESPACE_XFORMS),
>+                                 NS_LITERAL_STRING("contextcontainer"),
>+                                 getter_AddRefs(contextContainer));
> 
>-      mItems.AppendObject(item);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    nsCOMPtr<nsIDOMElement> modelElement = do_QueryInterface(model);
>+    nsAutoString modelID;
>+    modelElement->GetAttribute(NS_LITERAL_STRING("id"), modelID);
>+
>+    contextContainer->SetAttribute(NS_LITERAL_STRING("model"), modelID);
>+
>+    nsCOMPtr<nsIXFormsContextControl> ctx(do_QueryInterface(contextContainer));
>+    if (ctx) {
>+      ctx->SetContext(nsCOMPtr<nsIDOMElement>(do_QueryInterface(node)),
>+                      i + 1, nodeCount);
>+    }
>+    // Clone the template content under the item
>+    for (PRUint32 j = 0; j < templateNodeCount; ++j) {
>+      templateNodes->Item(j, getter_AddRefs(templateNode));
>+      templateNode->CloneNode(PR_TRUE, getter_AddRefs(cloneNode));
>+      contextContainer->AppendChild(cloneNode, getter_AddRefs(templateNode));
>     }
>-  }
> 
>-  // refresh parent control, since we are being defered
>-  nsCOMPtr<nsIDOMNode> parentNode;
>-  mElement->GetParentNode(getter_AddRefs(parentNode));
>-  if (parentNode) {
>+    itemNode->AppendChild(contextContainer, getter_AddRefs(tmpNode));
>+    anonContent->AppendChild(itemNode, getter_AddRefs(tmpNode));
>+  }
> 
>-    nsCOMPtr<nsIXFormsControlBase> stub = do_QueryInterface(parentNode);
>+  // tell parent we appended a child
>+  if (parent) {
>+    nsCOMPtr<nsIXTFElement> stub = do_QueryInterface(parent);
>     if (stub) {
>-      stub->Refresh();
>+      stub->ChildAppended(parent);

This is strange, use nsIXFormsControlBase, not nsIXTFElement.



>+nsXFormsSelectElement::SetValue(const nsAString& aValue)
> {

Same as in DelegateStub, so please remove.


>+      <field name="_refreshing">false</field>

Add a comment why this is needed.

Comment 32

[Andrew Douglas]

Can we have exception handling to ensure we always set _refreshing back to false
then? Also, it seems like a bit too much of a race condition to not immediately
set _refreshing to true after checking to see if it's already refreshing.
Thanks - I hope this helps.
-Andrew

Comment 33

[Doron Rosenberg (IBM)]

checked into trunk