281668 - select1 as a combobox

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

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

Comment 1

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

Attachment: v1

With this patch we don't support images etc. in selects but at least
select1 works as a combobox and no need to modify nsComboboxControlFrame.
Comments?

Comment 2

[Allan Beaufour]

(In reply to comment #1)
> With this patch we don't support images etc. in selects but at least
> select1 works as a combobox and no need to modify nsComboboxControlFrame.
> Comments?

1) Hmmm, it's kind of sad not to support complex content. But well, it's good to
support select1. But do you have a plan for supporting complex content?

2) Can select/select1 not watch for changes to child labels, instead of the
label checking for the parent? It seems the wrong way around for me.

Comment 3

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

(In reply to comment #2) 
> 2) Can select/select1 not watch for changes to child labels, instead of the
> label checking for the parent? It seems the wrong way around for me.

Unfortunately the MutationEvent approach doesn't seem to work :(
I don't know if anyone has tested whether MutationEvents work
properly with XTF/anonymous content.

Comment 4

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

Comment on attachment 175740 [details] [diff] [review]
v1

argh, sorry, this was a bad patch.

Comment 5

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

Attachment: v2-pre

putting one version here, so that I don't lose it.
This one doesn't use html:select at all, supports
@selection=open/close and item,choices child elements.
Not yet itemset.
(This patch isn't clean atm.)

I'm going to think also the option to convert this to an XBL binding.

Comment 6

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

Attachment: v2.1-pre

Allan, if you have time to test this one...
I had beaufour.mdgfix.patch in my tree and apparently it made 
select1 working much faster.
I think I should convert this from xtf to xtf+xbl.

Comment 7

[Allan Beaufour]

(In reply to comment #6)
> Created an attachment (id=179053) [edit]
> v2.1-pre
> 
> Allan, if you have time to test this one...

I will, before the weekend hopefully.

> I had beaufour.mdgfix.patch in my tree and apparently it made 
> select1 working much faster.

Adding a pinch of beaufour to something is always a good idea :)

Comment 8

[Allan Beaufour]

Attachment: Testcase

This testcase exibits (as a minimum) the following problems:
* Only "Vanilla" shows up in select1's
* It's not possible to see the highlight of "Chocolate" when you want to choose
it -- could be defined as correct (?), but add a bit of margin that is
highlighted then
* The width of "Select1 -- styled" changes, depending on whether you do press
Reload or SHIFT-Reload
* There's weird UI flickering when a select1 is refreshed

Comment 9

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

Thanks for this testcase!

(In reply to comment #8)
> Created an attachment (id=179264) [edit]
> Testcase
> 
> This testcase exibits (as a minimum) the following problems:
> * Only "Vanilla" shows up in select1's
hm, I had tested <choices> with only on <item>

> * It's not possible to see the highlight of "Chocolate" when you want to choose
> it -- could be defined as correct (?), but add a bit of margin that is
> highlighted then
> * The width of "Select1 -- styled" changes, depending on whether you do press
> Reload or SHIFT-Reload
strange.
> * There's weird UI flickering when a select1 is refreshed
I think I didn't see that with beaufour.mdgfix.patch

I'm still going to continue with this, but XBLifing it.

Comment 10

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

Attachment: v2.1.1

This fixes at least some of the problems:
I don't see UI flickering anymore, and now it is possible to
really select something else than the first item (if choices is used) ;)
But anyway, just putting this here, so that I don't loose this.

Comment 11

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

Just FYI, I started to implement XBL-fied select1.

Comment 12

[Doron Rosenberg (IBM)]

Can you mail me your xbl select1?  I need something to key off for select xbl :)

Comment 13

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

(In reply to comment #11)
> Just FYI, I started to implement XBL-fied select1.
I wonder where I lost the first (partial) implementation o_O

Comment 14

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

Attachment: work-in-progress

Not-yet-ready XBLized select1.
The delay with this one has been terrible, sorry about that :(

Comment 15

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

Attachment: work-in-progress 2

Comment 16

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

Attachment: work-in-progress 3

Comments/testing welcome.
One testcase
http://www.cs.helsinki.fi/u/pettay/moztests/xforms/nested_select1.xhtml

Comment 17

[Allan Beaufour]

(In reply to comment #16)
> Created an attachment (id=189220) [edit]
> work-in-progress 3

I applied the patch and tried attachment 179264 [details]. It did not look healthy, and
crashed firefox when I tried to select something in one of the select's.

Comment 18

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

(In reply to comment #17)
> I applied the patch and tried attachment 179264 [details] [edit]. It did not look
healthy, and
> crashed firefox when I tried to select something in one of the select's.

wow. I think I did test using that page too. Hmm...

Comment 19

[Allan Beaufour]

(In reply to comment #17)
> (In reply to comment #16)
> > Created an attachment (id=189220) [edit] [edit]
> > work-in-progress 3
> 
> I applied the patch and tried attachment 179264 [details] [edit]. It did not look
healthy, and
> crashed firefox when I tried to select something in one of the select's.

My bad, doing bug 298431 left an old xforms chrome lying around. It works now.

Comment 20

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

(In reply to comment #16)
> Created an attachment (id=189220) [edit]
> work-in-progress 3
> 

If you see some strange resizing problems, add the following to xforms.css
 
item {
  white-space : nowrap;
}

Comment 21

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

Attachment: XBL v1 for (at least preliminary) reviews

Comment 22

[Doron Rosenberg (IBM)]

for select, I am using the following to allow people to style the widget:
xbl:inherits="style"

You might want to add that to your html:input.

Comment 23

[Allan Beaufour]

(In reply to comment #22)
> for select, I am using the following to allow people to style the widget:
> xbl:inherits="style"

In what namespace?

Comment 24

[Doron Rosenberg (IBM)]

xmlns:xbl="http://www.mozilla.org/xbl", which already exists in your select1
xbl.  That way, we inherit any style="" on the xforms:select1.

Comment 25

[Doron Rosenberg (IBM)]

btw, shouldn't you be sending xforms-value-changed events?

Comment 26

[aaronr]

(In reply to comment #25)
> btw, shouldn't you be sending xforms-value-changed events?

The model does that as part of the SetStatesInternal stuff, after it sets the
readonly, relevant, etc. states

Comment 27

[Leigh L. Klotz, Jr.]

xforms-value-changed events go to all bound form controls.
they are not sent only to (or by) the form control that changed the value, but
instead by the processor during xforms-refresh.
they are in fact also sent when setvalue changes the value.  For the details,
see http://www.w3.org/2003/10/REC-xforms-10-20031014-errata.html#E70d

Comment 28

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

(In reply to comment #22)
> for select, I am using the following to allow people to style the widget:
> xbl:inherits="style"
> 
> You might want to add that to your html:input.

See https://bugzilla.mozilla.org/show_bug.cgi?id=283219#c3 .

Comment 29

[Doron Rosenberg (IBM)]

Comment on attachment 189341 [details] [diff] [review]
XBL v1 for (at least preliminary) reviews

>Index: nsIXFormsSelectChild.idl
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsIXFormsSelectChild.idl,v
>retrieving revision 1.3
>diff -u -8 -p -r1.3 nsIXFormsSelectChild.idl
>--- nsIXFormsSelectChild.idl	1 Feb 2005 01:18:27 -0000	1.3
>+++ nsIXFormsSelectChild.idl	14 Jul 2005 19:11:00 -0000
>@@ -47,40 +47,44 @@ class nsStringArray;
> 
> [ref] native constStringArray(const nsStringArray);
> 
> /**
>  * nsIXFormsSelectChild is implemented by elements that can be children
>  * of an XForms select element (choices, item, itemset).
>  */
> 
>-[uuid(21e37ef5-ce3e-4ff1-b7b2-fb34c4e2c8be)]
>+[scriptable, uuid(21e37ef5-ce3e-4ff1-b7b2-fb34c4e2c8be)]
> interface nsIXFormsSelectChild : nsISupports
> {
>   /**
>    * The DOM elements returned by this attribute are inserted into the select
>    * element's anonymous content tree.
>    */
>   readonly attribute nsIArray anonymousNodes;
> 
>+  readonly attribute AString value;
>+

so itemset can contain a copy, which we don't support atm.  Perhaps a warning
about this?  Or will we just take care of changing the API once copy lands?

>Index: nsXFormsItemElement.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsItemElement.cpp,v
>retrieving revision 1.8
>diff -u -8 -p -r1.8 nsXFormsItemElement.cpp
>--- nsXFormsItemElement.cpp	21 Jun 2005 16:47:29 -0000	1.8
>+++ nsXFormsItemElement.cpp	14 Jul 2005 19:11:02 -0000
> NS_IMETHODIMP
> nsXFormsItemElement::Refresh()
> {
>+  if (mInsideSelect1) {
>+    return NS_OK;
>+  }
>+
>+  nsCOMPtr<nsIDOMNode> parent;
>+  mElement->GetParentNode(getter_AddRefs(parent));
>+  if (!parent) {
>+    return NS_OK;
>+  }
>+

why do you not continue if it is inside a select1?  a comment would help.

>Index: nsXFormsItemSetElement.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsItemSetElement.cpp,v
>retrieving revision 1.6
>diff -u -8 -p -r1.6 nsXFormsItemSetElement.cpp
>--- nsXFormsItemSetElement.cpp	21 Jun 2005 16:47:29 -0000	1.6
>+++ nsXFormsItemSetElement.cpp	14 Jul 2005 19:11:02 -0000
> NS_IMETHODIMP
>+nsXFormsItemSetElement::SelectItemByValue(const nsAString &aValue, nsIDOMNode **aSelected)
>+{
>+  *aSelected = nsnull;
>+  nsCOMPtr<nsIXFormsItemSetUIElement> uiItemSet(do_QueryInterface(mElement));
>+  if (uiItemSet) {
>+    nsCOMPtr<nsIDOMElement> anonContent;
>+    uiItemSet->GetAnonymousItemSetContent(getter_AddRefs(anonContent));
>+    if (anonContent) {
>+      nsCOMPtr<nsIDOMNode> child, tmp;
>+      anonContent->GetFirstChild(getter_AddRefs(child));
>+      while (child) {
>+        child->GetFirstChild(getter_AddRefs(tmp)); // <item> is the first child.
>+        if (tmp) {
>+          nsCOMPtr<nsIXFormsSelectChild> selectChild(do_QueryInterface(tmp));
>+          if (selectChild) {
>+            selectChild->SelectItemByValue(aValue, aSelected);
>+            if (*aSelected) {
>+              return NS_OK;
>+            }
>+          }
>+        }
>+        tmp.swap(child);
>+        tmp->GetNextSibling(getter_AddRefs(child));
>+      }
>+    }
>+  }
>+  return NS_OK;
>+}

nit, put a newline before the first if, makes nested ifs more readable :)


>+++ resources/content/select1.xml	2005-07-14 19:31:38.000000000 +0300
>@@ -0,0 +1,558 @@
>+<?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
>+   - Olli Pettay
>+   - Portions created by the Initial Developer are Copyright (C) 2005
>+   - the Initial Developer. All Rights Reserved.
>+   -
>+   - Contributor(s):
>+   -  Olli Pettay <[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 ***** -->
>+
>+<bindings id="select1Bindings"
>+   xmlns="http://www.mozilla.org/xbl"
>+   xmlns:html="http://www.w3.org/1999/xhtml"
>+   xmlns:xbl="http://www.mozilla.org/xbl"
>+   xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
>+   xmlns:xforms="http://www.w3.org/2002/xforms">
>+
>+  <!-- select1 -->
>+  <binding id="xformswidget-select1"
>+           extends="chrome://xforms/content/xforms.xml#xformswidget-base">
>+    <resources>
>+      <stylesheet src="chrome://xforms/content/xforms.css"/>
>+    </resources>
>+    <content>
>+      <children includes="label"/>
>+      <html:div class="-moz-xforms-select1-popup" anonid="popup"
>+                onmouseover="this.parentNode.mouseOver(event);"
>+                onmouseup="this.parentNode.mouseUp(event);">
>+        <children/>
>+      </html:div>
>+      <html:span class="-moz-select1-container"
>+        anonid="container"><html:input class="-moz-xforms-select1-input"
>+                    anonid="control"
>+                    onblur="this.parentNode.parentNode.handleBlur();"
>+                    onclick="this.parentNode.parentNode.handleControlClick();"
>+                    onkeyup="this.parentNode.parentNode.handleKeyUp(event);"
>+                    onfocus="this.parentNode.parentNode.handleFocus();"
>+                    /><html:input class="-moz-xforms-select1-dropdown"
>+                                  type="button"
>+                                  anonid="dropmarker"
>+                                  tabindex="-1"
>+                                  onclick="this.parentNode.parentNode.togglePopup();
>+                                           if (this.parentNode.parentNode.popupOpen)
>+                                             this.previousSibling.focus();"/></html:span></content>
>+

why are you not adding newlines to make this more readable?  Because of the
whitespace nodes it creates?

Looks good

Comment 30

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

> 
> so itemset can contain a copy, which we don't support atm.  Perhaps a warning
> about this?  Or will we just take care of changing the API once copy lands?
I think we'll probably have to change APIs anyway, so perhaps a warning 
is not needed.

> 
> why do you not continue if it is inside a select1?  a comment would help.
> 
Will fix.

> 
> nit, put a newline before the first if, makes nested ifs more readable :)
>
Will do in the next patch.
 
> 
> why are you not adding newlines to make this more readable?  Because of the
> whitespace nodes it creates?
yes, because of the whitespace nodes

Comment 31

[aaronr]

Things I noticed playing around with the latest patch:

1) Clicking on the dropdown button when the list is already down should cause
the dropdown to go back up and disappear.
2) There is no visual indicator when the combobox has focus and no selected item
yet (f.x. the default case).  I gave it focus by tabbing to it, but I could not
see any indication that it had focus until I pressed the down arrow key and the
dropdown showed up.  Yet in HTML I see it.  Be really nice if we had it too. 
Really helps accessibility or when no mouse is present (task centric users).
3) If I click on the item from the list with the mouse, then I can no longer
select an item using the keyboard unless I tab around to the combobox again.  It
is like the focus got lost/moved.  This doesn't happen with the HTML combobox.

Comment 32

[Allan Beaufour]

Attachment: Testcase - event

This testcase has an event listener for xforms-recalculate with a modal
message. 1. Click a selection
2. Click OK to close the message window
3. hover the mouse over the select/select1
=> The control behaves like the mouseclick is still active.

Comment 33

[Allan Beaufour]

(In reply to comment #16)
> Created an attachment (id=189220) [edit]
> work-in-progress 3
> 
> Comments/testing welcome.
> One testcase
> http://www.cs.helsinki.fi/u/pettay/moztests/xforms/nested_select1.xhtml 

This one does not seem to work for me with the latest patch. When I try to
select something it seems like some updating appears but nothing gets selected?

Comment 34

[Allan Beaufour]

Attachment: Sequence testcase

This testcase has 2 select1's, one with incremental="true". They should behave
differently wrt. "xforms-value-changed", they do not.

(You're leaving xforms-select/deselect for bug 299273 I guess?)

Comment 35

[Allan Beaufour]

Attachment: Testcase - sequencing (v2)

(In reply to comment #34)
> Created an attachment (id=189668) [edit]
> Sequence testcase
> 
> This testcase has 2 select1's, one with incremental="true". They should
behave
> differently wrt. "xforms-value-changed", they do not.

Somebody *ahem* did not read the spec. properly, incremental is true by default
for select and select1. Oops. Here's a valid testcase. Seems to work as it
should.

Comment 36

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

Attachment: previous + focus/selected indicators + other fixes

Comment 37

[Allan Beaufour]

Attachment: Testcase - select/select1 styling

My original testcase was referencing a local gif-file. This should fix that.

The "select1 --styled" has an error: Because of the image, the text is "sort
of" right-aligned, and you can not see the entire text of "Strawberry" when it
is selected.

Comment 38

[Allan Beaufour]

(In reply to comment #36)
> Created an attachment (id=189891) [edit]
> previous + focus/selected indicators + other fixes

Do not know whether it's an issue, but in
http://www.cs.helsinki.fi/u/pettay/moztests/xforms/nested_select1.xhtml
try clicking any of the select1's, and press "arrow up". Only the selected
select1 clears it's data, while pressing "arrow down" will change all select1s
immediately.

Comment 39

[Allan Beaufour]

Attachment: Testcase - xforms vs. html select

Current patch has two problems with this testcase:
* The "pp" in Apple is cut of in the bottom (the input field is not high
enough)
* If you show the dropdown box, hovers the mouse over the values, and clicks
outside the select1 the focused selection is chosen. It should only be selected
if the user actually has clicked it. Try comparing with the html:select.

Comment 40

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

(In reply to comment #39)
> Current patch has two problems with this testcase:
> * The "pp" in Apple is cut of in the bottom (the input field is not high
> enough)

I can see this only if I increase text size.

> * If you show the dropdown box, hovers the mouse over the values, and clicks
> outside the select1 the focused selection is chosen. It should only be selected
> if the user actually has clicked it. Try comparing with the html:select.

Hö, I have fixed this already once. Some change broke it again.

Comment 41

[Allan Beaufour]

(In reply to comment #40)
> (In reply to comment #39)
> > Current patch has two problems with this testcase:
> > * The "pp" in Apple is cut of in the bottom (the input field is not high
> > enough)
> 
> I can see this only if I increase text size.

It changes when I change text size, yes, but it never fails for the html:select.

Comment 42

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

Attachment: ...and a new patch....

Fixing Allan's comments...

Comment 43

[Doron Rosenberg (IBM)]

Smaug, playing with my select xbl, I found this bug with itemset:

http://www.nexgenmedia.net/xforms/select.xhtml

If a select1 has an itemset, and a input changes the label of one of the items,
the value does not refresh in the dropdown.  Same happens for select,
itemset::refresh is never called.

Comment 44

[Allan Beaufour]

(In reply to comment #41)
> (In reply to comment #40)
> > (In reply to comment #39)
> > > Current patch has two problems with this testcase:
> > > * The "pp" in Apple is cut of in the bottom (the input field is not high
> > > enough)
> > 
> > I can see this only if I increase text size.
> 
> It changes when I change text size, yes, but it never fails for the html:select.

This still happens in the latest patch. Also, the html:input changes its width
according to the text size, but the select1 does not.

Comment 45

[Allan Beaufour]

Comment on attachment 190051 [details] [diff] [review]
...and a new patch....

Again smaug: Do not ask for review without documentation of your interfaces,
and please include a comment for the patch including a textual description of
what the patch actually does.

> Index: nsIXFormsSelectChild.idl
> ===================================================================
> -[uuid(21e37ef5-ce3e-4ff1-b7b2-fb34c4e2c8be)]
> +[scriptable, uuid(21e37ef5-ce3e-4ff1-b7b2-fb34c4e2c8be)]

It shouldn't matter much for this, but shouldn't you change the uuid when you
change the interface?

>  interface nsIXFormsSelectChild : nsISupports
>  {
>    /**
>     * The DOM elements returned by this attribute are inserted into the select
>     * element's anonymous content tree.
>     */
>    readonly attribute nsIArray anonymousNodes;

> +  readonly attribute AString value;
> +

Documentation.

>    /**
>     * These two methods allow the select to synchronize selection with the
>     * instance data.
>     *
>     * selectItemsByValue() handles children that have a value element child.
>     * If the value element text for this child is contained in the |values|
>     * array, then the item should select itself.
>     *
>     * selectItemsByContent() handles children that have a copy element child.
>     * If |node| points to the same instance data node as the item, then the
>     * item should select itself.
>     */
> -  void selectItemsByValue(in constStringArray values);
> +  [noscript] void selectItemsByValue(in constStringArray values);
>    void selectItemsByContent(in nsIDOMNode node);
>
> +  nsIDOMNode selectItemByValue(in AString value);
> +

Move the description SIBV down to the function itself.

Why does SIBV return a node? (better docs please).

> Index: nsXFormsChoicesElement.cpp
> ===================================================================

>  private:
> -  nsIDOMElement *mElement;
> +  nsIDOMElement* mElement;

nit: Why?

>  NS_IMETHODIMP
> +nsXFormsChoicesElement::SelectItemByValue(const nsAString &aValue, nsIDOMNode **aSelected)
> +{
> +  *aSelected = nsnull;

You should null-check this, especially when it is a scriptable function.
Possibly also check mElement, we usally do that.

> +  nsCOMPtr<nsIDOMNodeList> children;
> +  nsresult rv = mElement->GetChildNodes(getter_AddRefs(children));
> +  NS_ENSURE_SUCCESS(rv, rv);

> +NS_IMETHODIMP
>  nsXFormsChoicesElement::SelectItemsByValue(const nsStringArray &aValueList)
>  {
>    nsCOMPtr<nsIDOMNodeList> children;
>    nsresult rv = mElement->GetChildNodes(getter_AddRefs(children));
>    NS_ENSURE_SUCCESS(rv, rv);

>    PRUint32 childCount = 0;
>    children->GetLength(&childCount);

The SelectItem(s)ByValue functions are almost identical. We cannot reuse
functionality?

> +NS_IMETHODIMP
> +nsXFormsChoicesElement::GetValue(nsAString &aValue)
> +{
> +  return NS_OK;
> +}

Please comment on why you shouldn't return anything here.

> Index: nsXFormsItemElement.cpp
> ===================================================================

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

This needs some comments.
* why do you not refresh when you are inside a select1?
* does this not fail if the element is moved from a select1 to a select
(mInsideSelect1 is only reset in ctor)?

>  NS_IMETHODIMP
> +nsXFormsItemElement::SelectItemByValue(const nsAString &aValue, nsIDOMNode **aSelected)
> +{
> +  nsAutoString value;
> +  GetValue(value);
> +  if (aValue.Equals(value))
> +    NS_ADDREF(*aSelected = mElement);
> +  else
> +    *aSelected = nsnull;

null-check aSelected and use curly braces around if and else.

> +NS_IMETHODIMP
> +nsXFormsItemElement::SetActive(PRBool aActive)
> +{

Include comment about what this is used for.

> +NS_IMETHODIMP
> +nsXFormsItemElement::GetLabelText(nsAString& aValue)
> +{
> +  NS_ENSURE_STATE(mElement);
> +  nsCOMPtr<nsIDOMNodeList> children;
> +  mElement->GetChildNodes(getter_AddRefs(children));
> +  NS_ENSURE_STATE(children);
> +
> +  PRUint32 childCount;
> +  children->GetLength(&childCount);
> +
> +  nsAutoString value;
> +
> +  nsCOMPtr<nsIDOMDocument> doc;
> +  mElement->GetOwnerDocument(getter_AddRefs(doc));
> +
> +  nsCOMPtr<nsIDOMNode> child;
> +  for (PRUint32 i = 0; i < childCount; ++i) {
> +    children->Item(i, getter_AddRefs(child));
> +    if (nsXFormsUtils::IsXFormsElement(child, NS_LITERAL_STRING("label"))) {
> +      nsCOMPtr<nsIXFormsLabelElement> label(do_QueryInterface(child));
> +      if (label) {
> +        nsAutoString labelValue;
> +        label->GetTextValue(labelValue);
> +        value.Append(labelValue);
> +      }
> +    }
> +  }
> +  aValue = value;
> +  return NS_OK;
> +}

So having two labels "aa" and "bb", the resulting label would actually be
"aabb"? Is that really correct?

> +NS_IMETHODIMP
> +nsXFormsItemElement::LabelRefreshed()
> +{
> +  if (mAddingChildren) {
> +    return NS_OK;
> +  }
> +
> +  nsCOMPtr<nsIDOMDocument> domDoc;
> +  mElement->GetOwnerDocument(getter_AddRefs(domDoc));
> +  nsCOMPtr<nsIDocument> doc = do_QueryInterface(domDoc);
> +  // This is an optimization. It prevents us doing some of the unnecessary
> +  // 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"))) {
> +      nsCOMPtr<nsIXFormsControl> select1(do_QueryInterface(parent));
> +      if (select1) {
> +        select1->Refresh();
> +      }
> +      return NS_OK;
> +    }
> +    current = parent;
> +  } while(current);
> +  return NS_OK;
> +}

If this is the way we want to handle label changes, how about making this more
general? I can image that it would be handy in general for XBL (custom)
controls to be notified about label changes. Ie. find the first xforms parent
and inform that about the refreshing.

> Index: nsXFormsItemSetElement.cpp
> ===================================================================

>  NS_IMETHODIMP
> +nsXFormsItemSetElement::SelectItemByValue(const nsAString &aValue, nsIDOMNode **aSelected)
> +{
> +  *aSelected = nsnull;
> +  nsCOMPtr<nsIXFormsItemSetUIElement> uiItemSet(do_QueryInterface(mElement));

This is implemented by XBL, right? Then please add a comment saying that.

> +  if (uiItemSet) {
> +    nsCOMPtr<nsIDOMElement> anonContent;
> +    uiItemSet->GetAnonymousItemSetContent(getter_AddRefs(anonContent));
> +    if (anonContent) {
> +      nsCOMPtr<nsIDOMNode> child, tmp;
> +      anonContent->GetFirstChild(getter_AddRefs(child));

Please use early returns, and add a comment about what you are doing.

>  NS_IMETHODIMP
> +nsXFormsItemSetElement::GetValue(nsAString &aValue)
> +{
> +  return NS_OK;
> +}

Again comment about null return. (And are we using the wrong interface, when we
need this?)

> @@ -266,63 +303,131 @@ nsXFormsItemSetElement::Refresh()
...
> +  // XXX This is temporary. Remove when XBLizing <select>.

Exactly how much is temporary?

> +        document->CreateElementNS(NS_LITERAL_STRING(NS_NAMESPACE_XFORMS),
> +                                  NS_LITERAL_STRING("item"),
> +                                  getter_AddRefs(itemNode));
> +
> +        document->CreateElementNS(NS_LITERAL_STRING(NS_NAMESPACE_XFORMS),
> +                                  NS_LITERAL_STRING("contextcontainer"),
> +                                  getter_AddRefs(contextContainer));
> +
> +        if (!itemNode || !contextContainer)
> +          return NS_OK;

? I would say that was a severe error...

> +
> +        nsCOMPtr<nsIDOMElement> modelElement = do_QueryInterface(model);
> +        nsAutoString modelID;
> +        modelElement->GetAttribute(NS_LITERAL_STRING("id"), modelID);
> +
> +        itemNode->SetAttribute(NS_LITERAL_STRING("model"), modelID);
> +
> +        // 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));
> +        }
> +        
> +        nsCOMPtr<nsIXFormsContextControl> ctx(do_QueryInterface(contextContainer));
> +        if (ctx) {
> +          ctx->SetContext(nsCOMPtr<nsIDOMElement>(do_QueryInterface(node)),
> +                          i, nodeCount);

I think that should be 'i + 1'. DOMNodes are 0-based, XForms positions are
1-based. (if it's valid, I can see that's an old bug...)

> +        }
> +        anonContent->AppendChild(contextContainer, getter_AddRefs(tmpNode));
> +        contextContainer->AppendChild(itemNode, getter_AddRefs(tmpNode));

With mutation events and whatever, does it not make most sense to change the
order of these two?

> +      item->SetContext(nsCOMPtr<nsIDOMElement>(do_QueryInterface(node)),
> +                       i, nodeCount);

'i + 1' again.

> Index: nsXFormsModelElement.cpp
> ===================================================================

> +nsVoidArray* 
> +nsPostRefresh::PostRefreshList()
> +{
> +  return sPostRefreshList;
> +}
> +

How about returning a const'ed version?

> Index: resources/content/xforms.css
> ===================================================================
> +item {
> +  white-space : nowrap;
> +}
> + [... lines and lines of CSS voodoo :) ]

I guess you know what you are doing in the following CSS voodoo? Possibly
include a comment and link to your "inspiration source"?

> +++ nsIXFormsItemElement.idl	2005-07-20 00:41:11.000000000 +0300
> +/**
> + * Interface implemented by the item element.
> + */
> +[scriptable, uuid(796a2e26-a40b-4ebf-be2c-42faf5fea6c4)]
> +interface nsIXFormsItemElement : nsISupports
> +{
> +  readonly attribute AString labelText;
> +  void setActive(in boolean aActive);
> +  void labelRefreshed();
> +};

Argh. Add documentation.

> +++ nsIXFormsItemSetUIElement.idl	2005-07-20 00:41:11.000000000 +0300
> +[scriptable, uuid(19862a10-42ac-4d20-8204-9c44a122797d)]
> +interface nsIXFormsItemSetUIElement : nsISupports
> +{
> +  readonly attribute nsIDOMElement anonymousItemSetContent;
> +};

Can you guess what I will say here?

> +++ nsIXFormsLabelElement.idl	2005-07-20 00:41:11.000000000 +0300
> +/**
> + * Interface implemented by the label element.
> + */
> +[uuid(f357747b-a1a2-4044-bd25-cb5d5b4fde3a)]
> +interface nsIXFormsLabelElement : nsISupports
> +{
> +  readonly attribute AString textValue;
> +};

!

> +++ nsXFormsSelect1Element.cpp	2005-07-20 00:41:11.000000000 +0300

> +NS_IMETHODIMP
> +nsXFormsSelect1Element::SetValue(const nsAString& aValue)
> +{
> +  if (!mBoundNode || !mModel)
> +    return NS_OK;
> +
> +  PRBool changed;
> +  nsresult rv = mModel->SetNodeValue(mBoundNode, aValue, &changed);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  if (changed) {
> +    nsCOMPtr<nsIDOMNode> model = do_QueryInterface(mModel);
> + 
> +    if (model) {
> +      rv = nsXFormsUtils::DispatchEvent(model, eEvent_Recalculate);
> +      NS_ENSURE_SUCCESS(rv, rv);
> +      rv = nsXFormsUtils::DispatchEvent(model, eEvent_Revalidate);
> +      NS_ENSURE_SUCCESS(rv, rv);
> +      rv = nsXFormsUtils::DispatchEvent(model, eEvent_Refresh);
> +      NS_ENSURE_SUCCESS(rv, rv);
> +    }
> +  }
> +
> +  return NS_OK;
> +}

This is identical to nsXFormsInputElement::SetValue(), right? Guess my comment.

> +++ resources/content/select1.xml	2005-07-22 00:23:47.000000000 +0300

I'm sorry to say that I loose focus extremely quickly reviewing javascript, so
I only have a few comments... :(

You use the XForms namespace a lot of places, can we not put that some where
global? Eventually as a property/field on the widget-base?

> +<bindings id="select1Bindings"

General comment for the file.

> +   xmlns="http://www.mozilla.org/xbl"
> +   xmlns:html="http://www.w3.org/1999/xhtml"
> +   xmlns:xbl="http://www.mozilla.org/xbl"
> +   xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
> +   xmlns:xforms="http://www.w3.org/2002/xforms">
> +
> +  <!-- select1 -->

This _seriously_ needs some general comments about the implementation.

> +      <html:span class="-moz-select1-container"
> +                 anonid="container"><html:input
> +                    class="-moz-xforms-select1-input"
> +                    anonid="control"
> +                    onblur="this.parentNode.parentNode.handleBlur();"
> +                    onclick="this.parentNode.parentNode.handleControlClick();"
> +                    onkeypress="this.parentNode.parentNode.handleKeyPress(event);"
> +                    onkeyup="this.parentNode.parentNode.handleKeyUp(event);"

What happened to the indentation here?

> +  <binding id="xformswidget-select1-itemset">

Comment please.

> +    <resources>
> +      <stylesheet src="chrome://xforms/content/xforms.css"/>

Why is this needed?

A couple of general comments:
* We are restricting the select1 implementation to HTML, f.x. choices uses
"optgroup", item, etc. Not a problem for the bug, but shouldn't we note that
somewhere?

* Why all the different handling of select and select1, shouldn't they function
more or less the same? Is it because of xbl-select is work-in-progress?

* Somewhere the responsability of XTF and XBL should be documented. We need to
write some more general design notes on devmo.

Comment 46

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

Attachment: and more fixes

Adding some comments, fixing a bug when scrolling the
popup using keyboard - now the right <item> should be always visible.

Also fixing the bug Doron noticed; if a <label> inside an <itemset> was
refreshed, the new value wasn't shown in the popup. 
Now the content of the <itemset> is generated in a bit different way:
<itemset>
(anonymous-->)
  <item><contextcontainer><label/><value/></contextcontainer></item>
  <item><contextcontainer><label/><value/></contextcontainer></item>
  <item><contextcontainer><label/><value/></contextcontainer></item>
  ...
(<--anonymous)
</itemset>

>A couple of general comments:
>* We are restricting the select1 implementation to HTML, f.x. choices uses
>"optgroup", item, etc. Not a problem for the bug, but shouldn't we note that
>somewhere?

This is not restricted to XHTML. All the (X)HTML specific code is on XBL side,
not in XTF. 

> * Why all the different handling of select and select1, shouldn't they 
> functionmore or less the same? Is it because of xbl-select is 
> work-in-progress?

Yes, I guess mainly because of the XTF <select>, which is implemented in
a quite strange way. (And in very XHTML specific way)

> * Somewhere the responsability of XTF and XBL should be documented. We need 
> to write some more general design notes on devmo.

I think after XBLizing <select>

Comment 47

[Doron Rosenberg (IBM)]

Comment on attachment 190451 [details] [diff] [review]
and more fixes

>Index: nsXFormsItemElement.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsItemElement.cpp,v
>retrieving revision 1.8
>diff -u -8 -p -r1.8 nsXFormsItemElement.cpp
>--- nsXFormsItemElement.cpp	21 Jun 2005 16:47:29 -0000	1.8
>+++ nsXFormsItemElement.cpp	25 Jul 2005 20:12:34 -0000
>@@ -36,88 +36,96 @@
> private:
>-  NS_HIDDEN_(nsresult) GetValue(nsString &aValue);
> 
>-  nsIDOMElement *mElement;
>+  nsIDOMElement* mElement;
>   nsCOMPtr<nsIDOMHTMLOptionElement> mOption;
> 
>-  // context node (used by itemset)
>-  nsCOMPtr<nsIDOMElement> mContextNode;
>-  PRInt32 mContextPosition;
>-  PRInt32 mContextSize;
>+  // context node (used by itemset in select)
>+  nsCOMPtr<nsIDOMElement>           mContextNode;
>+  PRInt32                           mContextPosition;
>+  PRInt32                           mContextSize;
>+  PRPackedBool                      mInsideSelect1;
> };

Why a packed bool?  From what I understand, packed bools only make sense if you
need several booleans, and you only have one.  Plus I am told it is less
efficient if only one is used.

>Index: resources/content/xforms.xml
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/resources/content/xforms.xml,v
>retrieving revision 1.4
>diff -u -8 -p -r1.4 xforms.xml
>--- resources/content/xforms.xml	13 Jul 2005 20:09:00 -0000	1.4
>+++ resources/content/xforms.xml	25 Jul 2005 20:12:36 -0000
>@@ -54,16 +54,22 @@
>       </constructor>
> 
>       <destructor>
>         _delegate = null;
>       </destructor>
> 
>       <field name="_delegate">null</field>
> 
>+      <property name="XFORMS_NS" readonly="true">
>+        <getter>
>+        return "http://www.w3.org/2002/xforms";
>+        </getter>
>+      </property>

style nit: shouldn't return "http://www.w3.org/2002/xforms"; be indented like
the rest of the file? :)

>--- /dev/null	2005-07-25 15:15:10.691723656 +0300
>+++ resources/content/select1.xml	2005-07-25 23:11:52.000000000 +0300

>+      <method name="handleKeyPress">
>+        <parameter name="aEvent"/>
>+        <body>
>+          <![CDATA[
>+            if (aEvent.keyCode == aEvent.DOM_VK_RETURN ||
>+                aEvent.keyCode == aEvent.DOM_VK_ENTER) {
>+              var open = this.popupOpen;
>+              this.togglePopup();
>+              if (open && this._selected) {
>+                this.updateInputField();
>+                this.delegate.value = this._selected.value;
>+              }
>+            } else if (aEvent.keyCode == aEvent.DOM_VK_UP ||
>+                       aEvent.keyCode == aEvent.DOM_VK_DOWN) {
>+              this.internalScroll(aEvent.keyCode == aEvent.DOM_VK_DOWN);
>+              if (this._selected && this.popupOpen) {
>+                var el = this._selected.QueryInterface(Components.interfaces.nsIDOMElement);
>+                if ("scrollIntoView" in el) {
>+                  el.scrollIntoView(false);
>+                }
>+              }
>+              
>+              if (!this.popupOpen && this.getAttribute("incremental") != "false") {
>+                if (this._selected) {
>+                  this.updateInputField();
>+                  this.delegate.value = this._selected.value;
>+                } else if (this.selectionOpen) {
>+                  this.delegate.value = this.inputField.value;
>+                }
>+              }
>+            } else if (aEvent.keyCode == aEvent.DOM_VK_TAB) {
>+              // Hiding popup when user uses keyboard to focus out
>+              // from <select1>. No need to update the value of the control
>+              // in this case.
>+              this.hidePopup();
>+              if (this._selected) {
>+                this._selected.setActive(false);
>+                this._selected = this._tmpSelected;
>+                this._tmpSelected = null;
>+                if (this._selected) {
>+                  this._selected.setActive(true);
>+                }
>+              }
>+            }
>+            return true;
>+          ]]>
>+        </body>
>+      </method>
>+

perhaps cache aEvent.keyCode so we don't have to always query the property?

Comment 48

[Allan Beaufour]

(In reply to comment #41)
> (In reply to comment #40)
> > (In reply to comment #39)
> > > Current patch has two problems with this testcase:
> > > * The "pp" in Apple is cut of in the bottom (the input field is not high
> > > enough)
> > 
> > I can see this only if I increase text size.
> 
> It changes when I change text size, yes, but it never fails for the html:select.

I'm still seeing this problem in the newest patch

Comment 49

[Allan Beaufour]

in attachment 189873 [details]: try tabbing into "select1", choose a value with the arrow
keys, and then tab to "select1_inc". Nothing is selected.

Comment 50

[Allan Beaufour]

(In reply to comment #46)
> Created an attachment (id=190451) [edit]
> and more fixes

There are still unanswered questions from comment 45.

Comment 51

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

Attachment: + doron's nits and fix for the tabbing issue.

> The SelectItem(s)ByValue functions are almost identical. We cannot reuse
> functionality?

Parameters aren't the same and recursion is a bit different too, so I didn't 
change it.

I didn't change LabelRefreshed either, not sure to which generic interface
that should be added or perhaps a new interface is needed(?)

Comment 52

[Allan Beaufour]

(In reply to comment #48)
> (In reply to comment #41)
> > (In reply to comment #40)
> > > (In reply to comment #39)
> > > > Current patch has two problems with this testcase:
> > > > * The "pp" in Apple is cut of in the bottom (the input field is not high
> > > > enough)
> > > 
> > > I can see this only if I increase text size.
> > 
> > It changes when I change text size, yes, but it never fails for the html:select.
> 
> I'm still seeing this problem in the newest patch

Olli, are you ignoring this problem, or? It's not a giant problem, but it needs
to be fixed sometime.

Comment 53

[Allan Beaufour]

(In reply to comment #51)
> Created an attachment (id=191323) [edit]
> + doron's nits and fix for the tabbing issue.
> 
> > The SelectItem(s)ByValue functions are almost identical. We cannot reuse
> > functionality?
> 
> Parameters aren't the same and recursion is a bit different too, so I didn't 
> change it.

The difference is minimal in my eyes, but no biggie, so np.

Comment 54

[Allan Beaufour]

(In reply to comment #46)
> >A couple of general comments:
> >* We are restricting the select1 implementation to HTML, f.x. choices uses
> >"optgroup", item, etc. Not a problem for the bug, but shouldn't we note that
> >somewhere?
> 
> This is not restricted to XHTML. All the (X)HTML specific code is on XBL side,
> not in XTF. 

Am I missing something, or? nsXFormsItemElement::OnCreated():
...
 nsCOMPtr<nsIDOMElement> element;
  domDoc->CreateElementNS(NS_LITERAL_STRING(NS_NAMESPACE_XHTML),
                          NS_LITERAL_STRING("option"),
                          getter_AddRefs(element));
...

Comment 55

[Allan Beaufour]

(In reply to comment #54)
> (In reply to comment #46)
> > >A couple of general comments:
> > >* We are restricting the select1 implementation to HTML, f.x. choices uses
> > >"optgroup", item, etc. Not a problem for the bug, but shouldn't we note that
> > >somewhere?
> > 
> > This is not restricted to XHTML. All the (X)HTML specific code is on XBL side,
> > not in XTF. 
> 
> Am I missing something, or? nsXFormsItemElement::OnCreated():
> ...
>  nsCOMPtr<nsIDOMElement> element;
>   domDoc->CreateElementNS(NS_LITERAL_STRING(NS_NAMESPACE_XHTML),
>                           NS_LITERAL_STRING("option"),
>                           getter_AddRefs(element));
> ...

... we both have XTF and XBL content for items until bug 300801 is in?

Comment 56

[Allan Beaufour]

Comment on attachment 191323 [details] [diff] [review]
+ doron's nits and fix for the tabbing issue.

> Index: nsIXFormsSelectChild.idl
> ===================================================================
> @@ -68,19 +68,26 @@ interface nsIXFormsSelectChild : nsISupp
>     * selectItemsByValue() handles children that have a value element child.
>     * If the value element text for this child is contained in the |values|
>     * array, then the item should select itself.
>     *
>     * selectItemsByContent() handles children that have a copy element child.
>     * If |node| points to the same instance data node as the item, then the
>     * item should select itself.
>     */
> -  void selectItemsByValue(in constStringArray values);
> +  [noscript] void selectItemsByValue(in constStringArray values);
>    void selectItemsByContent(in nsIDOMNode node);

I would still like to see the comments for the two functions split up.

> Index: nsXFormsChoicesElement.cpp
> ===================================================================

>  NS_IMETHODIMP
>  nsXFormsChoicesElement::Refresh()
>  {
> +  if (mInsideSelect1) {
> +    return NS_OK;
> +  }
> +

Include the same comment as you did for items.

> Index: nsXFormsDelegateStub.h
> ===================================================================

> +#ifdef DEBUG_smaug
> +  virtual const char* Name()
> +  {
> +    if (mControlType.IsEmpty()) {
> +      return "[undefined delegate]";
> +    } else {

Not the most meaningful else, but it's your own debug code so I do not care
much :)

> Index: nsXFormsItemElement.cpp
> ===================================================================

>  NS_IMETHODIMP
> -nsXFormsItemElement::OnCreated(nsIXTFGenericElementWrapper *aWrapper)
> +nsXFormsItemElement::OnCreated(nsIXTFBindableElementWrapper *aWrapper)

See comment 55. If it's only there until bug 300801 lands, please include an
XXX about it.

> +NS_IMETHODIMP
> +nsXFormsItemElement::GetValue(nsAString &aValue)
>  {
> + 
> +  nsCOMPtr<nsIDOMNode> firstChild, container;
> +  mElement->GetFirstChild(getter_AddRefs(firstChild));
> +
> +  // If this element is generated inside an <itemset>,
> +  // there is a <contextcontainer> element between this element
> +  // and the actual childnodes.

It's fine, but I suspect that we might be able to get rid of that. I cannot
remember the reasoning about it when select was implented. bryner had a
reason.

> +NS_IMETHODIMP
> +nsXFormsItemElement::LabelRefreshed()

Ok to leave it here. I'll come up with a generic thing. I also have some
refinements to the UIWidget interface anyhow.

> Index: nsXFormsLabelElement.cpp
> ===================================================================

>  NS_IMETHODIMP
> +nsXFormsLabelElement::Refresh()
> +{
> +  nsXFormsDelegateStub::Refresh();
> +  nsCOMPtr<nsIDOMNode> parent;
> +  mElement->GetParentNode(getter_AddRefs(parent));
> +  nsCOMPtr<nsIXFormsItemElement> item(do_QueryInterface(parent));
> +  if (item) {
> +    item->LabelRefreshed();
> +  } else if (parent) {

Include a note about the contextcontainer business. (which I guess it is...)

> +++ nsIXFormsItemElement.idl	2005-08-02 13:33:13.000000000 +0300

> +  /**
> +   * This is called by the \<label\> child element whenever it is refreshed.
> +   * This information will be propagated by the \<item\> to the nearest
> +   * \<select1\> element, which can the refresh its UI.

then refresh


> +++ resources/content/select1.xml	2005-08-02 14:10:42.000000000 +0300

> +            if (this._selected) {
> +              this.updateInputField();
> +            } else if (this.selectionOpen) {
> +              this.inputField.value = newValue;
> +            } else {
> +              // @todo out-of-range

Remember XXX

With the above fixed r=me

Comment 57

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

Attachment: +Allan's comments

I haven't forgotten the 'apple' problem, but normally I don't see it at all and

I haven't figured out what would be the best way to solve that problem.

Comment 58

[Doron Rosenberg (IBM)]

checked in.