270572 - Implement <select>

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[Brian Ryner (not reading)]

This covers implementation of <select> and <select1> (which should be 99%
identical).

The most straightforward way of implementing this is something like this (arrows
indicate explicit content, all other children should be considered anonymous):

<xforms:select>  <----
  <html:select>
    <xforms:item>   <-----
       <html:option>
       <xforms:label/>   <-----
       <xforms:value/>   <-----
    </xforms:item>  <-----
  </html:select>
</xforms:select>  <-----

In other words, xforms:item, xforms:choices, and xforms:itemset would all
construct anonymous content that would be interleaved into the xforms:select's
anonymous content tree.

Unfortuantely, this approach is currently blocked by bug 270567.

The only way of implementing this that I _don't_ think would be impacted by
270567 is for the xforms:item/choices/itemset implementations to directly
manipulate the xforms:select's anonymous content tree.  That is, we'd have a
private API on the select element that allows these children to insert their
anonymous content, and retrieve it later to make changes.  It's more
complicated, but it's workable if we can't find a fix for 270567 reasonably soon.

Comment 1

[Brian Ryner (not reading)]

Darin and I worked this out and decided that hooking up the parent pointer would
not actually help the situation.  Even if you could walk up from the HTML
<option> to the XForms <item>, it would not be able to see the HTML <select>
since that is in a different anonymous content tree.

So, I think we're going to go with the alternate approach outlined here.  Since
this will make the items not actually be XTF visuals, we don't need multiple
insertion points either.  All items can be inserted into the span.

Comment 2

[Brian Ryner (not reading)]

Attachment: patch

Implementation of select, itemset, item, and value elements (copy is not
implemented).

Parts of this patch are kind of convoluted so I'll try to give an overview of
how it works.  The idea is simple, we want to create an anonymous HTML <select>
and use <optgroup> and <option> to implement the items.  Right away, there is a
problem with the way anonymous content interacts with our HTML select and
option implementations -- if the <option> is anonymous content for the <item>,
then the option element cannot see the select element at all, and it needs to
in order to work correctly.  XTF doesn't set the parent pointer for the root
anonymous content, and even if it did, it would point to the XForms <select>,
not the HTML <select>.	So, to solve that, I make all of the options live in
the XForms <select>'s anonymous content tree.  nsXFormsSelectElement crawls its
children and asks each of them for an array of content nodes, which are
appended as children of the HTML select.  These element implementations do the
same thing recursively, so <choices> will hand back an <optgroup> which already
contains <option>s for all of the <item>s.

With that problem addressed, there is an additional problem of getting an item
label into an option element.  You really want the anonymous content to end up
like this:

<option>
  <label/>
</option>

but that's not currently possible in XTF (even moreso because these items
aren't XTF visuals for the reason mentioned above, so they can't use insertion
points).  So I just clone the label into the option.  It's not perfect, since
it will cause CSS selectors that affect the label not to match, but it works
ok.

Finally, to support itemset, we need to provide a context node for the
anonymous "item" elements, very similar to what <repeat> does.	In fact, I
reused the RepeatItem implementation, and added support for it having an inline
anonymous element instead of a div.

Comment 3

[Darin Fisher]

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

>Index: nsXFormsChoicesElement.cpp

>+NS_IMETHODIMP
>+nsXFormsChoicesElement::BeginAddingChildren()
...
>+  wrapper->SetNotificationMask(nsIXTFElement::NOTIFY_PARENT_CHANGED |
>+                               nsIXTFElement::NOTIFY_DONE_ADDING_CHILDREN);

I don't see this being reset in DoneAddingChildren.

>Index: nsXFormsItemElement.cpp

>+static PRBool IsLabelChild(nsIDOMNode *aChild)
>+{
>+  nsAutoString value;
>+  aChild->GetLocalName(value);
>+  if (value.EqualsLiteral("label")) {
>+    aChild->GetNamespaceURI(value);
>+    return value.EqualsLiteral(NS_NAMESPACE_XFORMS);
>+  }
>+
>+  return PR_FALSE;
>+}

this appears to be duplicated... move to nsXFormsUtils?


>+nsXFormsItemElement::BeginAddingChildren()
...
>+  wrapper->SetNotificationMask(nsIXTFElement::NOTIFY_PARENT_CHANGED |
>+                               nsIXTFElement::NOTIFY_DONE_ADDING_CHILDREN);

this also needs to be reset in DoneAddingChildren.

>+  mElement->GetAttribute(NS_LITERAL_STRING("model"), modelID);
>+  repeatItem->SetAttribute(NS_LITERAL_STRING("model"), modelID);

nit: use NS_NAMED_LITERAL_STRING for "model"


>Index: nsXFormsItemSetElement.cpp

>+nsXFormsItemSetElement::BeginAddingChildren()
...
>+  wrapper->SetNotificationMask(nsIXTFElement::NOTIFY_PARENT_CHANGED |
>+                               nsIXTFElement::NOTIFY_DONE_ADDING_CHILDREN);

also needs to be unsuppressed


>Index: nsXFormsModelElement.cpp

>+__declspec(dllexport) void
>+test_comptr_assign(nsISupports *aPtr)
>+{
>+  nsCOMPtr<nsISupports> i = aPtr;
>+}
>+
>+__declspec(dllexport) void
>+test_comptr_assign2(nsISupports *aPtr)
>+{
>+  nsCOMPtr<nsISupports> i(aPtr);
>+}

ooops ;)


>Index: nsXFormsSelectElement.cpp

>+inline PRBool
>+IsWhiteSpace(PRUnichar c)
>+{
>+  return (c == PRUnichar(' ') || c == PRUnichar('\r') || c == PRUnichar('\n')
>+          || c == PRUnichar('\t'));
>+}
>+
>+void
>+nsXFormsSelectElement::SelectItemsInList(const nsString &aValueList)
>+{
>+  // Start by separating out the space-separated list of values.
>+
>+  const PRUnichar *start = aValueList.get();
>+  nsStringArray valueList;
>+
>+  do {
>+    // Skip past any whitespace.
>+    while (*start && IsWhiteSpace(*start))
>+      ++start;
>+
>+    const PRUnichar *valueStart = start;
>+
>+    // Now search for the end of the value
>+    while (*start && !IsWhiteSpace(*start))
>+      ++start;
>+
>+    valueList.AppendString(nsDependentSubstring(valueStart, start));
>+  } while (*start);

I wonder if we should add this as nsStringArray::ParseString to mirror
nsCStringArray::ParseString.  I could also use this code for parsing
the schema attribute on: <model schema="foo   bar baz">


sr=darin

Comment 4

[Allan Beaufour]

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

+++ nsXFormsRepeatItemElement.cpp	9 Dec 2004 01:22:57 -0000
...
+  PRBool isBlock;
+
+  nsAutoString localName;
+  mElement->GetLocalName(localName);
+  isBlock = localName.EqualsLiteral("repeatitem");
+

Needs a comment.

Reusing repeatitem is good, but I do not like the approach. Do we need to code
it this way, can we not rely purely on CSS styling to get the needed
functionality?

I would also like a rename of RepeatItemElement ... to ContextContainer or
something like that?


+++ nsXFormsSelectElement.cpp	9 Dec 2004 01:22:57 -0000

Where is support for @selection and @incremental? At least there should be a
todo for each of them.

>+nsXFormsChoicesElement::Refresh()
...
+	 node3->GetTextContent(labelValue);

This is a general issue I guess, but a labal can contain images, styling, etc.
so you cannot use GetTextContent() to represent it.

>+        // XXX mutation event listener

Needs a better explanation.

+++ General
Mozilla complains quite loudly when the mouse is over the control:
###!!! ASSERTION: Form control has a frame, but it's not a form frame: 'Error',
file nsGenericHTMLElement.cpp, line 2275

Darin has gone through the code, so I've tried to focus on the use of it, but
without a functioning label that's kind of hard. I've tried applying the latest
patch from bug 269023, but it only makes Mozilla crash... so I need some help
to get label to work, before I can really say yes or no to this patch.

Comment 5

[Brian Ryner (not reading)]

> Reusing repeatitem is good, but I do not like the approach. Do we need to code
> it this way, can we not rely purely on CSS styling to get the needed
> functionality?

What don't you like about it, specifically?  The alternative I'd see is to use
an attribute on repeatitem that becomes the style attribute of the anonymous
child, but that would cause a lot of extra work since OnCreated is called before
any attributes are set.

> >+nsXFormsChoicesElement::Refresh()
> ...
> +	 node3->GetTextContent(labelValue);
> 
> This is a general issue I guess, but a labal can contain images, styling, etc.
> so you cannot use GetTextContent() to represent it.

Actually that's a good point, there are a number of cases that this misses.  It
would be better if this could clone the label into the optgroup, like item does.
 I'll play around with that.

> 
> >+        // XXX mutation event listener
> 
> Needs a better explanation.

Not needed if I do the above fix.

> 
> +++ General
> Mozilla complains quite loudly when the mouse is over the control:
> ###!!! ASSERTION: Form control has a frame, but it's not a form frame: 'Error',
> file nsGenericHTMLElement.cpp, line 2275
> 

It seems to be a general problem with HTML optgroup.

Comment 6

[Brian Ryner (not reading)]

Attachment: updated patch

Patch updated with Allan's comments.  I now clone the entire label into the
optgroup, so it should support arbitrary content.

I've also heard that the patch on bug 269023 causes a crash from Aaron, but I
haven't seen any stack trace or seen the crash for myself, so it's difficult
for me to diagnose it.	What OS/compiler are you using, and is it Firefox or
Seamonkey?

Comment 7

[Allan Beaufour]

Comment on attachment 168522 [details] [diff] [review]
updated patch

+++ nsIXFormsSelectChild.idl	11 Dec 2004 08:35:20 -0000
...
>+/**
>+ * nsIXFormsSelectChild is implemented by elements that can be children
>+ * of an XForms select element (choices, item, itemset).
>+ */

Comment-nit: In doxygen, the comment attaches itself to the first structure
after the comment. So this comment attaches itself to:
>+class nsStringArray;

It needs to be moved down before the interface.


+++ nsXFormsChoicesElement.cpp	11 Dec 2004 08:35:20 -0000

Include a comment for the class, please.

>+#include "nsIDOM3Node.h"

Are you using this? (goes for other files too)

>+class nsXFormsChoicesElement : public nsXFormsStubElement,
>+                               public nsIXFormsSelectChild
...
>+  NS_HIDDEN_(nsresult) GetValue(nsString &aValue);

Am I blind (quite possible ...), or is this definition never used?

>+nsXFormsChoicesElement::ChildInserted(nsIDOMNode *aChild, PRUint32 aIndex)
...
>+  } else if (nsXFormsUtils::IsLabelElement(aChild)) {
>+    // If a label child was inserted, we need to grab its text content
>+    // as our label attribute.

Comment about text content is outdated.

>+nsXFormsChoicesElement::ChildAppended(nsIDOMNode *aChild)

Same here.

>+nsXFormsChoicesElement::WillRemoveChild(PRUint32 aIndex)
>+{
>+  // If a label child was removed, we need to grab its text content
>+  // as our label attribute.

I do not understand this one. You clone the label element, just before it is
removed?

>+NS_IMETHODIMP
>+nsXFormsChoicesElement::SelectItemsByContent(nsIDOMNode *aNode)
>+{
>+  return NS_OK;
>+}

Comment?
// Not applicable for choices element

>+nsXFormsChoicesElement::Refresh()
>+{
>+  // Remove any existing first child that is not an option.

For clearing any existing label I guess?, this shoud be included in the
comment.

>+      mOptGroup->InsertBefore(child2, firstChild, getter_AddRefs(child));

How does InsertBefore behave if firstChild == nsnull?


+++ nsXFormsContextContainer.cpp	11 Dec 2004 08:35:20 -0000

>+ * an "unrolled" \<repeat\> or \<itsemset\>. @see nsXFormsRepeatElement and

itsemsementesem :)


+++ nsXFormsGroupElement.cpp	11 Dec 2004 08:35:20 -0000
>@@ -284,7 +284,8 @@ nsXFormsGroupElement::Process()
>     model->AddFormControl(this);
>   }
>   
>-  NS_ENSURE_TRUE(result, NS_ERROR_FAILURE);
>+  if (!result)
>+    return NS_ERROR_FAILURE;

Sneaking in a patch for group are you? :) It is fine, direct checking is
correct behaviour, but we still depend on bug 272225 (that is, me).


+++ nsXFormsItemElement.cpp	11 Dec 2004 08:35:21 -0000

Include a comment for the class, please.

>+nsXFormsItemElement::ChildInserted(nsIDOMNode *aChild, PRUint32 aIndex)

Same comments on ChildInserted, ChildAppended, and WillRemoveChild as for
nsXFormsChoicesElement.

>+nsXFormsItemElement::SelectItemsByContent(nsIDOMNode *aNode)

Same comment as for nsXFormsChoicesElement

>+nsXFormsItemElement::WriteSelectedItems(nsIDOMNode *aContainer)
...
>+  if (!textNode) {
>+    // No text node, so make one.
>+    nsCOMPtr<nsIDOMDocument> doc;
>+    aContainer->GetOwnerDocument(getter_AddRefs(doc));
>+    NS_ENSURE_TRUE(doc, NS_ERROR_FAILURE);
>+
>+    nsCOMPtr<nsIDOMText> text;
>+    rv = doc->CreateTextNode(EmptyString(), getter_AddRefs(text));
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    textNode = text;
>+  }

Shouldn't textNode be appended to aContainer?

>+nsXFormsItemElement::Refresh()
...
>+  nsCOMPtr<nsIDOMElement> container;
>+  doc->CreateElementNS(NS_LITERAL_STRING(NS_NAMESPACE_XFORMS),
>+                       NS_LITERAL_STRING("contextcontainer-inline"),
>+                       getter_AddRefs(container));

Hmmm, can you in fact not implement nsIXFormsContextControl directly on
nsXFormsItemElement, instead of wrapping a container around it?


+++ nsXFormsItemSetElement.cpp	11 Dec 2004 08:35:21 -0000

> +nsXFormsItemSetElement::Refresh()
...
>+    item->SetContextNode(nsCOMPtr<nsIDOMElement>(do_QueryInterface(node)));

You need to set @contextsize and @contextposition too, to support a label like:
<label value="string(position())"/>
See nsXFormsRepeatElement::Refresh().

We still depend on bug 265460, to actually make use it of course.


+++ nsXFormsSelectElement.cpp	11 Dec 2004 08:35:25 -0000

*ahem* IACFTC, please :)

And, I still do not see a todo/XXX about @selection?

>+nsXFormsSelectElement::Refresh()
>+nsXFormsSelectElement::HandleEvent(nsIDOMEvent *aEvent)

These two both call EvaluateNodeBinding() for the same expression. With
incremental == 'true', this will be expensive. How about storing the node they
are bound to, and update it on AttributeSet(), ie. splitting binding and
refreshing. I'm doing some of the same for nsXFormsInput/Output in bug 265467.

>+IsWhiteSpace(PRUnichar c)

Somewhat nit. This is the same as:
nsXFormsXPathXMLUtil::IsWhitespace()

Whether it will stay there, hmmm ... not sure, when bug 265212 is landed, but
how about using that until then?

>+nsXFormsSelectElement::SelectCopiedItem(nsIDOMNode *aNode)
>+{
>+}

Needs a todo/XXX.

+++ nsXFormsValueElement.cpp	11 Dec 2004 08:35:25 -0000

+// The value element does not display any content, it simply provides
+// a value from inline text or instance data.

/**
 * Implementation of the XForms \<value\> element.
 *
 * @note The value element does not display any content, it simply provides
 * a value from inline text or instance data.
 */

Please? :)


I still need to get the label patch (bug 269023) to work to test functionality,
but it is currently out of sync with the tree. When it is updated I'll try to
get it to work or create a proper crash report.

Comment 8

[Allan Beaufour]

(In reply to comment #5)
> > Reusing repeatitem is good, but I do not like the approach. Do we need to code
> > it this way, can we not rely purely on CSS styling to get the needed
> > functionality?
> 
> What don't you like about it, specifically?

(This might cancel itself, if we do not need to wrap the elements. See above
comments.)

My idea was to create the same nsXFormsRepeatItemElement instantiation for both
uses, the only difference being the tag-name; repeatitem and repeatitem-inline,
and the just use CSS stylesheet to style the two tags as group and inline,
accordingly. Then we do not need to change any code. Is that possible? Will it
get us into trouble? I am all ears :)

Comment 9

[Brian Ryner (not reading)]

(In reply to comment #7)
> >+      mOptGroup->InsertBefore(child2, firstChild, getter_AddRefs(child));
> 
> How does InsertBefore behave if firstChild == nsnull?
> 

As DOM 3 Core says, it appends it to the end.

> Hmmm, can you in fact not implement nsIXFormsContextControl directly on
> nsXFormsItemElement, instead of wrapping a container around it?

Not really, because I only want the label content to be cloned into the option,
not the entire item.

> +++ nsXFormsSelectElement.cpp	11 Dec 2004 08:35:25 -0000
> 
> *ahem* IACFTC, please :)

huh?

> >+nsXFormsSelectElement::Refresh()
> >+nsXFormsSelectElement::HandleEvent(nsIDOMEvent *aEvent)
> 
> These two both call EvaluateNodeBinding() for the same expression. With
> incremental == 'true', this will be expensive. How about storing the node they
> are bound to, and update it on AttributeSet(), ie. splitting binding and
> refreshing. I'm doing some of the same for nsXFormsInput/Output in bug 265467.
> 

I think I'd rather wait and see if this is actually a performance issue in practice.

> >+IsWhiteSpace(PRUnichar c)
> 
> Somewhat nit. This is the same as:
> nsXFormsXPathXMLUtil::IsWhitespace()
> 
> Whether it will stay there, hmmm ... not sure, when bug 265212 is landed, but
> how about using that until then?

I'd rather the function be inlined; I can either leave it or make
nsXFormsXPathXMLUtil inline.

> +++ nsXFormsValueElement.cpp	11 Dec 2004 08:35:25 -0000
> 
> +// The value element does not display any content, it simply provides
> +// a value from inline text or instance data.
> 
> /**
>  * Implementation of the XForms \<value\> element.
>  *
>  * @note The value element does not display any content, it simply provides
>  * a value from inline text or instance data.
>  */
> 
> Please? :)

Updated, however, please note that I don't spend time using doxygen, I read
comments in the source files, and tend to format them to look nice there.

I'll post a patch with all of your comments addressed that I understood/agreed with.

Comment 10

[Brian Ryner (not reading)]

Attachment: updated patch

Comment 11

[Allan Beaufour]

(In reply to comment #10)
> Created an attachment (id=168715) [edit]
> updated patch

It is out of sync with the tree.

Comment 12

[Brian Ryner (not reading)]

Attachment: updated patch

updated to the trunk

Note that in general, patches in Bugzilla may have merge conflicts and may not
be updated every time the code in the tree changes.  Most of these merge
conflicts can be easily resolved locally, as is the case with this one.

Comment 13

[Allan Beaufour]

(In reply to comment #12)
> Note that in general, patches in Bugzilla may have merge conflicts and may not
> be updated every time the code in the tree changes.  Most of these merge
> conflicts can be easily resolved locally, as is the case with this one.

Yeah, but my time was somewhat short and my health was shaky ... bad excuses (tm) :)

Comment 14

[aaronr]

Just a suggestion: use GetModel instead of GetNodeContext.  Does pretty much the
same thing under the covers.  Just nicer to read and won't be affected by
changes to GetNodeContext (via bug 265931).

Comment 15

[aaronr]

Attachment: select1 testcase

built the patch.  I don't get select1 working.	It appears and behaves
identically to a select for me.  Shows me both options in my testcase and I can
select both items.

Comment 16

[Allan Beaufour]

Comment on attachment 168812 [details] [diff] [review]
updated patch

>+nsXFormsChoicesElement::WillRemoveChild(PRUint32 aIndex)
...
>+  } else if (child && nsXFormsUtils::IsLabelElement(child)) {
>+    // If a label child was removed, we need to remove the label from our
>+    // anonymous content.
>+    Refresh();
>+  }

I still do not get this one. _Before_ a label is removed you clone it's
contents in Refresh(). Should this not be triggered by ChildRemoved() and not
WillRemoveChild()?

>+void
>+nsXFormsSelectElement::SelectCopiedItem(nsIDOMNode *aNode)
>+{
>+}

This still needs a XXX/TODO.

Besides the above two, it looks good. Well, select1 does not work as Aaron also
noted, so that should be fixed too :)

r=allan

Comment 17

[Brian Ryner (not reading)]

Attachment: patch checked into XFORMS_20050106_BRANCH

I incorperated fixes suggested by allan and aaron.  Part of the select1 problem
was just a thinko, calling the method on the wrong object.  Ideally select1
should display like an HTML select size=1, but there seems to be a problem with
selecting options if I do that.  I left that to fix later.

Comment 18

[Brian Ryner (not reading)]

checked into the trunk.