298371 - make richlistbox multi-selectable

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect)

Description

[Doron Rosenberg (IBM)]

Comment 1

[Aaron Leventhal]

I suggest allowing the attribute seltype="multiple" to determine whether it is
multi selectable. That's what a regular listbox does.

We also need to support nsIDOMXULMultiSelectControlElement. That interface
inherits from nsIDOMXULSelectControlElement so we need to support that as well.
While we're at it we should finish implementing
nsIDOMXULSelectControlItemElement for the rich list items.

Ctrl+up/down should move focus without changing selection
Shift+up/down should extend selection
Ctrl+space should toggle selection on the current line

Comment 2

[Aaron Leventhal]

Only in a multiple selection listbox, please fire a DOM ItemSelected or
ItemUnselected event on the actual list items when their selection state changes.

Comment 3

[Simon Bünzli]

Attachment: make richlistbox inherit from listbox

If richlistbox inherits from listbox, multiple selection, FAYT (see bug 298993)
and keyboard accessibility are pretty cheap to have. This patch does most of
that. Still missing are some "details" around the difference of the current
item (previously called selected) and the selected items for generated content
(see _refreshSelection). Furthermore the behavior of the richlistbox changes in
some details (e.g. if seltype is not "multiple", selected items can't be
deselected anymore). If you're interested in this patch, I'll try to fiddle out
these details.

One more thing I don't know how to solve: by implementing
nsIDOMXULSelectControlElement I have to provide appendItem and insertItemAt,
which both take a label as argument. However the items' label property is read
only and there is no xul:richlistitem anyway. As for now, I accept an item
instead of the label (which I can handle and which may even make some sense).
As an alternative, I could provide a generic richlistitem which contains just a
label and use that. What to do here?

Comment 4

[Simon Bünzli]

Attachment: take 2

In this version, appendItem and insertItemAt accept both a string label or a
richlistitem as parameter. Additionally, template handling has been improved
far enough for the Extensions and Downloads managers to work again as expected
(so far in single selection mode).

Comment 5

[Simon Bünzli]

Comment on attachment 197925 [details] [diff] [review]
take 2

These two patches are far from perfect. Now before you waste your time on them,
I'd rather ask you to have a look at the state of the moment. The new
richlistbox.xml and richlistbox.css are both included verbatim in my Console²
extension (http://forums.mozillazine.org/viewtopic.php?t=318102). The changes
to listbox.xml are included about as far as necessary. The implementation is
stable enough to replace the richlistbox used in the Extensions and Downloads
managers with hardly any notable difference (problems are mostly due to the
differences of selectedItem and currentItem and modifications to
_refreshSelection). For testing this, you can install the extension and
uncomment in its chrome.manifest the two overrides at the bottom (should work
with trunk and branch builds). Multiple selection handling can be tested in the
new JavaScript/Error Console.

In case there isn't any (negative) feedback, I'll post another complete patch
for review when Doron is back from his holidays.

Comment 6

[Doron Rosenberg (IBM)]

Hmm, the way I did it in my tree was implement the multi-selection stuff myself
rather than inherit from listbox.

Not sure what the best solution is, but we have time to think about it, since
this is a 1.9 thing.

Comment 7

[Simon Bünzli]

Attachment: Take 3

Console²'s richlistbox implementation has now been stable for about two months and there are no known issues. Thus you get here a first patch to review, in case this should be the way to go.

Advantages of this way:
* Less duplicate code is needed (you get e.g. a fix for bug 298993 for free).
* There's a (mostly) unified API for listbox and richlistbox - listbox-parity is thus guaranteed.
* Due to backward-compatibility methods, this patch should be usable on the 1.8.1 branch as well.

Caveats:
* richlistbox's scrollOnePage method is not backwards compatible since it has the same signature but a different contract in the 1.8 richlistbox and listbox implementations (OTOH it's officially not documented at all).

Notes:
* If you have installed Console² v0.2.1 or newer, you've already been using this patch (i.e. FAYT was enabled in the Extensions and Downloads managers).
* This patch also includes a patch to bug 267866.

Comment 8

[Doron Rosenberg (IBM)]

The problem I have is that things like insertItemAt are really useless for a richlistbox, so I am not sure inheriting from listbox is a good idea.  We want a lot, but not all of its features.

Comment 9

[Simon Bünzli]

The other problem is that as of comment #1 we want to support nsIDOMXULMultiSelectControlElement which inherits from nsIDOMXULSelectControlElement which promises precisely these methods. If insertItemAt et al. must go, these interfaces probably should be rewritten (although I don't know how important a set of such interfaces are for richlistbox). If they are, this patch still applies - except for the different/missing signatures of insertItemAt and appendItem.

If we went with the existing interfaces, I'd vote for either providing "clever" methods (which insert a richlistitem or create a new richlistitem containing a label having the passed string as value depending of the argument - that's what the patch does) or "dummy" methods which throw NS_ERROR_NOT_IMPLEMENTED.

In any case, I'd still try to minimize code duplication/divergent APIs and at worst create a common base class for listbox and richlistbox (which doesn't claim to implement any interface).

Comment 10

[Simon Bünzli]

One place to discuss the design issues involved in fixing this bug: http://wiki.mozilla.org/XUL:Richlistbox

Comment 11

[Doron Rosenberg (IBM)]

Comment on attachment 205541 [details] [diff] [review]
Take 3

can you update this to trunk?

Comment 12

[Simon Bünzli]

Attachment: take 3 (pass 2)

There hasn't been much change during the last three months, so the patch is pretty stable. I've fixed some minor nits and one bug (suppressonselect vs. suppressOnSelect) and added a few comments.

On the branch I haven't had any issues during the last three months. While testing on the 20060331 trunk build, there were some scrolling issues - but there are similar issues with the original richlistbox, so I suppose it's rather a scrollbox issue.

Has there been any discussion on whether this is the way to go?

Comment 13

[Doron Rosenberg (IBM)]

mconnor/rob strong - you guys are the owners of toolkit.  Up to you.

Comment 14

[Simon Bünzli]

Comment on attachment 217033 [details] [diff] [review]
take 3 (pass 2)

Mike, who else could review this patch?

Comment 15

[Doron Rosenberg (IBM)]

I'll review it once a toolkit peer oks the concept.

Comment 16

[Asaf Romano (gone)]

Comment on attachment 217033 [details] [diff] [review]
take 3 (pass 2)

Nice work, but some of this stuff isn't acceptable API-wise:

+      <method name="insertItemAt">
+        <parameter name="index"/>
+        <parameter name="label"/>
+        <parameter name="value"/>
+        <body>
...
+            if (label instanceof Components.interfaces.nsIDOMXULSelectControlItemElement) {

No, this is extremely problematic, esp. for c++ use cases.

I would go with the basic-bindings approach from comment 9. For a11y reasons we can (temporary!) implement nsIDOMXULSelectControlItemElement for richlistitem and return NS_ERROR_NOT_IMPLEMENTED from insertItemAt and from appendItem. You should talk to Neil D. about adding a new xul interface for richlistitem (or rewriting the current interface).


>Index: toolkit/content/widgets/listbox.xml
>===================================================================

>+      <property name="suppressOnSelect"
>+                onget="return this.getAttribute('suppressonselect') == 'true';"
>+                onset="this.setAttribute('suppressonselect', val);"/>
>+

Could we check the field too instead of checking them both every time?

Comment 17

[Simon Bünzli]

(In reply to comment #16)
> I would go with the basic-bindings approach from comment 9.

Alright. Unfortunately I don't anymore have the resources for doing this myself.

Comment 18

[alexander :surkov (:asurkov)]

(In reply to comment #17)
> (In reply to comment #16)
> > I would go with the basic-bindings approach from comment 9.
> 
> Alright. Unfortunately I don't anymore have the resources for doing this
> myself.
> 

I'd like to continue the work.

Comment 19

[alexander :surkov (:asurkov)]

Attachment: patch4

Mainly all I did is moving of common methods of listbox and richlistbox to baselistbox.xml

Comment 20

[alexander :surkov (:asurkov)]

(In reply to comment #16)
> +            if (label instanceof
> Components.interfaces.nsIDOMXULSelectControlItemElement) {
> 
> No, this is extremely problematic, esp. for c++ use cases.

Right, fixed.

> I would go with the basic-bindings approach from comment 9.

Fixed.

> For a11y reasons we
> can (temporary!) implement nsIDOMXULSelectControlItemElement for richlistitem
> and return NS_ERROR_NOT_IMPLEMENTED from insertItemAt and from appendItem. You
> should talk to Neil D. about adding a new xul interface for richlistitem (or
> rewriting the current interface).

richlistitem implements nsIDOMXULSelectControlItemElement in Simon's patch already. richlistitem is inherited from listitem. Should I put base binding for them too?

> >+      <property name="suppressOnSelect"
> >+                onget="return this.getAttribute('suppressonselect') == 'true';"
> >+                onset="this.setAttribute('suppressonselect', val);"/>
> >+
> 
> Could we check the field too instead of checking them both every time?
> 

I guess no. Since js author awaits to get the same value as he setted. Other behaviour may confuse.

Comment 21

[Asaf Romano (gone)]

Comment on attachment 242646 [details] [diff] [review]
patch4

I didn't actually review this. But in any case, the base (common, whatever) bindings should live in listbox.xml. Standard naming for the bindings would be listbox-base, etc.

One more note: the xul:listbox C++ part (i.e. nsIListBoxObject and friends) has a good amount of bugs which do not exist in richlistbox (which uses a normall scrollbox). For that reason, any patch here should not make richlistbox use nsIListBoxObject. Looking at your patch, it looks like it doesn't , but please do note if I'm wrong...

Comment 22

[alexander :surkov (:asurkov)]

Attachment: patch5

(In reply to comment #21)
> (From update of attachment 242646 [details] [diff] [review] [edit])
> I didn't actually review this. But in any case, the base (common, whatever)
> bindings should live in listbox.xml. Standard naming for the bindings would be
> listbox-base, etc.

Fixed. The only one reason why I did it is I don't like big files.

> One more note: the xul:listbox C++ part (i.e. nsIListBoxObject and friends) has
> a good amount of bugs which do not exist in richlistbox (which uses a normall
> scrollbox). For that reason, any patch here should not make richlistbox use
> nsIListBoxObject. Looking at your patch, it looks like it doesn't , but please
> do note if I'm wrong...
> 

He doesn't use it. I even don't know how richlistbox can get access to listbox object.

Comment 23

[Asaf Romano (gone)]

Comment on attachment 242669 [details] [diff] [review]
patch5

+      <method name="getIndexOfItem">
+        <parameter name="aItem"/>
+        <body>
+          throw Components.results.NS_ERROR_NOT_IMPLEMENTED;
+        </body>
+      </method>
+
+      <method name="getItemAtIndex">
+        <parameter name="aIndex"/>
+        <body>
+          throw Components.results.NS_ERROR_NOT_IMPLEMENTED;
+        </body>
+      </method>
+
+      <method name="getRowCount">
+        <body>
+          throw Components.results.NS_ERROR_NOT_IMPLEMENTED;
+        </body>
+      </method>
+
+      <method name="getNumberOfVisibleRows">
+        <body>
+          throw Components.results.NS_ERROR_NOT_IMPLEMENTED;
+        </body>
+      </method>
+
+      <method name="getIndexOfFirstVisibleRow">
+        <body>
+          throw Components.results.NS_ERROR_NOT_IMPLEMENTED;
+        </body>
+      </method>
+
+      <method name="ensureIndexIsVisible">
+        <parameter name="aIndex"/>
+        <body>
+          throw Components.results.NS_ERROR_NOT_IMPLEMENTED;
+        </body>
+      </method>
+
+      <method name="ensureElementIsVisible">
+        <parameter name="element"/>
+        <body>
+          throw Components.results.NS_ERROR_NOT_IMPLEMENTED;
+        </body>
+      </method>
+
+      <method name="scrollToIndex">
+        <parameter name="aIndex"/>
+        <body>
+          throw Components.results.NS_ERROR_NOT_IMPLEMENTED;
+        </body>
+      </method>

What are those? they're not part of any interface your binding is implementing AFAICT and therefore don't have to be a part of listbox-base.

Comment 24

[alexander :surkov (:asurkov)]

Attachment: patch6

(In reply to comment #23)

> What are those? they're not part of any interface your binding is implementing
> AFAICT and therefore don't have to be a part of listbox-base.
> 

I removed them and list those methods in comment before baselistbox.

Comment 25

[Asaf Romano (gone)]

Comment on attachment 245836 [details] [diff] [review]
patch6

>Index: toolkit/content/widgets/listbox.xml
>===================================================================

>+   - The Original Code is toolkit code.

I don't think so. ;)


>-      <method name="insertItemAt">
....
>-          var XULNS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
>-          var item = document.createElementNS(XULNS, "listitem");
>-          item.setAttribute("label", label);
>-          item.setAttribute("value", value);
>-          var before = this.getItemAtIndex(index);
>-          if (before)
>-            this.insertBefore(item, before);
>-          else
>-            this.appendChild(item);
>-          return item;
..
>         </body>
>       </method>

>+      <method name="insertItemAt">
>+        <parameter name="index"/>
>+        <parameter name="label"/>
>+        <parameter name="value"/>
>+        <body>
>+        <![CDATA[
>+          var XULNS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
>+          var item = document.createElementNS(XULNS, "listitem");
>+          item.setAttribute("label", label);
>+          item.setAttribute("value", value);
>+          try {
>+            var before = this.getItemAtIndex(index);
>+            this.insertBefore(item, before);
>+          } catch (e) {
>+            this.appendChild(item);
>+          }
>+          return item;
>+        ]]>
>+        </body>
>+      </method>

I don't think this change is right (i.e. you should keep |if (before) instead of the try..catch block), what was the reasoning behind this change?


>+   <binding id="listbox-base"
>+            extends="chrome://global/content/bindings/general.xml#basecontrol">
> 
>-      <!-- ///////////////// nsIAccessibleProvider ///////////////// -->
>+    <implementation implements="nsIDOMXULMultiSelectControlElement, nsIAccessibleProvider">
> 
>+    <!-- nsIAccessibleProvider -->
>       <property name="accessibleType" readonly="true">
>         <getter>
>-          <![CDATA[
>-            return Components.interfaces.nsIAccessibleProvider.XULListbox;
>-          ]]>
>+          return Components.interfaces.nsIAccessibleProvider.XULListbox;
>         </getter>

This kind of cleanup isn't necessary, but OK.

>+    <!-- nsIDOMXULSelectControlElement -->
>+      <property name="selectedItem">
>+        <getter>
>         <![CDATA[
>-          var suppress = this._suppressOnSelect;
>-          if (timeout != -1)
>-            this._suppressOnSelect = true;
>-          
>-          this.selectItem(item);
>-          
>-          this._suppressOnSelect = suppress;         
>-          
>-          if (timeout != -1) {
>-            if (this._selectTimeout)
>-              window.clearTimeout(this._selectTimeout);
>-              
>-            this._selectTimeout = window.setTimeout(this._selectTimeoutHandler, timeout, this); 
>-          }
>+          return this.selectedItems.length > 0 ? this.selectedItems[0] : null;
>         ]]>
>-        </body>
>-      </method>      
>-
>-      <!-- ///////////////// private listbox members ///////////////// -->
>+        </getter>
>+        <setter>
>+          this.selectItem(val);

maybe use onget/onset instead?


>+  <!-- Binding for xul:listbox element.
>+  -->
>+  <binding id="listbox"
>+           extends="#listbox-base">
>+
...
>+      <method name="_fireOnSelect">
>+        <body>
>+        <![CDATA[
>+          if (!this._suppressOnSelect && !this.suppressOnSelect) {
>+            var event = document.createEvent("Events");
>+            event.initEvent("select", true, true);
>+            this.dispatchEvent(event);
>+          }
>+        ]]>
>+        </body>
>+      </method>
>+

Shouldn't this live in the base binding?

>+      <!-- ///////////////// nsIDOMXULSelectControlElement ///////////////// -->
>+
>+      <method name="insertItemAt">
>+        <parameter name="index"/>
>+        <parameter name="label"/>
>+        <parameter name="value"/>
>+        <body>
>+        <![CDATA[
>+          var XULNS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";

please change this to "const" while you are here.


>            for (var i = 0; i < rowCount; i++) {
>              var k = (i + start + c) % rowCount;
>              var item = this.getItemAtIndex(k); //listitem
>-             var cellText = item.getAttribute("label");
>+             // fayt_label allows to display different text than is returned
>+             // (in case a visible image is represented through text in the label)
>+             var cellText = "fayt_label" in item ? item.fayt_label : item.label;

What's this about?

>     <implementation implements="nsIDOMXULSelectControlItemElement, nsIAccessibleProvider">
>       <property name="current" onget="return this.getAttribute('current') == 'true';">
>         <setter><![CDATA[
>           if (val)
>             this.setAttribute("current", "true");
>           else
>             this.removeAttribute("current");
>+
>+          this._fireEvent(val ? "DOMMenuItemActive" : "DOMMenuItemInactive");

I suppose aaronlev approved this?

>Index: toolkit/content/widgets/richlistbox.xml
>===================================================================

>+    <!-- Overriding baselistbox -->
>+      <method name="_fireOnSelect">
...
>+            // make sure not to modify last-selected when suppressing select events
>+            // (otherwise we'll lose the selection when a template gets rebuilt)
>+            if (this._suppressOnSelect || this.suppressOnSelect)
>+              return;
>+
>+            // remember the current item and all selected items with IDs
>+            var state = this.currentItem ? this.currentItem.id : "";

What if it doesn't have an id?

>+      <property name="children" readonly="true">
>+        <getter>
>+          <![CDATA[
>+            var childNodes = [];
>+            for (var child = this.firstChild; child; child = child.nextSibling) {
>+              if (child instanceof Components.interfaces.nsIDOMXULSelectControlItemElement)
>+                childNodes.push(child);
>+            }
>+            return childNodes;
>+          ]]>
>+        </getter>
>+      </property>
>+

You can use this.childNodes instead of this property (and make sure the "includes" attribute is set to "richlistitem" on the <children> element of this binding).

>+      <method name="_refreshSelection">
>         <body>
>           <![CDATA[
...
>+              var suppressSelect = this._suppressOnSelect;
>+              this._suppressOnSelect = true;
>+              this.clearSelection();
>+              for (var i = 1; i < ids.length; i++) {
>+                var selectedItem = document.getElementById(ids[i]);
>+                if (selectedItem)
>+                  this.addItemToSelection(selectedItem);
>+              }
>+
>+              var currentItem = document.getElementById(ids[0]);
>+              if (!currentItem && this._currentIndex)
>+                currentItem = this.getItemAtIndex(Math.min(
>+                  this._currentIndex - 1, this.getRowCount()));
>+              if (currentItem) {
>+                this.currentItem = currentItem;
>+                if (this.selType != "multiple" && this.selectedCount == 0)
>+                  this.selectedItem = currentItem;
>+
>+                if (this.scrollBoxObject.height)
>+                  this.ensureElementIsVisible(currentItem);
>+                else // XXX hack around a bug in ensureElementIsVisible

bug # ?

>+                  this.ensureElementIsVisible(currentItem.previousSibling);
>+              }
>+              this._suppressOnSelect = suppressSelect;
>+              // XXX actually it's just a refresh, but at least
>+              // the Extensions manager expects this:
>+              this._fireOnSelect();

so why is _suppressOnSelect set to true then?

>+              return;
>+            }
>+
>+            // try to restore the selected items according to their IDs
>+            // (applies after a template rebuild, if last-selected was not set)
>+            if (this.selectedItems) {
>+              for (i = this.selectedCount - 1; i >= 0; i--) {
>+                if (this.selectedItems[i] && this.selectedItems[i].id)
>+                  this.selectedItems[i] = document.getElementById(this.selectedItems[i].id);
>+                else
>+                  this.selectedItems[i] = null;
>+                if (!this.selectedItems[i])
>+                  this.selectedItems.splice(i, 1);
>+              }
>+            }
>+            if (this.currentItem && this.currentItem.id)
>+              this.currentItem = document.getElementById(this.currentItem.id);
>+            else
>+              this.currentItem = null;

more ID dependencies.

>+            if (this.selType != "multiple" && this.selectedCount == 0)
>+              this.selectedItem = this.currentItem;
>+
>+            // XXX hack for the Downloads manager (better to focus the list than
>+            // the individual items - these usually aren't tabbable anyway, and
>+            // we need the keyboard focus for navigation):
>+            this.focus();

Download manager hacks in the download manager code please :)

>+      <!-- deprecated (use moveByOffset instead) -->
>+      <method name="goUp">

My approach would be "For backwards-compatibility and for convenience", then you wouldn't need to repeat this code in the keyup handler ;)

>+      <method name="goDown">

ditto.


>Index: toolkit/themes/pinstripe/global/richlistbox.css
>===================================================================

> 
> richlistbox {
>+  margin: 2px 4px;

Why is this change?


Neil Deakin should review this too.

Comment 26

[alexander :surkov (:asurkov)]

(In reply to comment #25)
> (From update of attachment 245836 [details] [diff] [review] [edit])
> >Index: toolkit/content/widgets/listbox.xml
> >===================================================================
> 
> >+   - The Original Code is toolkit code.
> 
> I don't think so. ;)

So, what do you think is more right here? XPFE where this code migrated from? Or? :)

> 
> >-      <method name="insertItemAt">
> ....
> >-          var XULNS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
> >-          var item = document.createElementNS(XULNS, "listitem");
> >-          item.setAttribute("label", label);
> >-          item.setAttribute("value", value);
> >-          var before = this.getItemAtIndex(index);
> >-          if (before)
> >-            this.insertBefore(item, before);
> >-          else
> >-            this.appendChild(item);
> >-          return item;
> ..
> >         </body>
> >       </method>
> 
> >+      <method name="insertItemAt">
> >+        <parameter name="index"/>
> >+        <parameter name="label"/>
> >+        <parameter name="value"/>
> >+        <body>
> >+        <![CDATA[
> >+          var XULNS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
> >+          var item = document.createElementNS(XULNS, "listitem");
> >+          item.setAttribute("label", label);
> >+          item.setAttribute("value", value);
> >+          try {
> >+            var before = this.getItemAtIndex(index);
> >+            this.insertBefore(item, before);
> >+          } catch (e) {
> >+            this.appendChild(item);
> >+          }
> >+          return item;
> >+        ]]>
> >+        </body>
> >+      </method>
> 
> I don't think this change is right (i.e. you should keep |if (before) instead
> of the try..catch block), what was the reasoning behind this change?
> 

this.getItemAtIndex(index) throws exception on wrong index. But appendItem is implemented by insertItemAt(-1). Therefore if getItemAtIndex() failed to return item, I append item into end of listbox.
> 
> >+    <!-- nsIDOMXULSelectControlElement -->
> >+      <property name="selectedItem">
> >+        <getter>
> >         <![CDATA[
> >-          var suppress = this._suppressOnSelect;
> >-          if (timeout != -1)
> >-            this._suppressOnSelect = true;
> >-          
> >-          this.selectItem(item);
> >-          
> >-          this._suppressOnSelect = suppress;         
> >-          
> >-          if (timeout != -1) {
> >-            if (this._selectTimeout)
> >-              window.clearTimeout(this._selectTimeout);
> >-              
> >-            this._selectTimeout = window.setTimeout(this._selectTimeoutHandler, timeout, this); 
> >-          }
> >+          return this.selectedItems.length > 0 ? this.selectedItems[0] : null;
> >         ]]>
> >-        </body>
> >-      </method>      
> >-
> >-      <!-- ///////////////// private listbox members ///////////////// -->
> >+        </getter>
> >+        <setter>
> >+          this.selectItem(val);
> 
> maybe use onget/onset instead?

onset is fine here. But i can't keep '>' sign in onget attribute, and I don't like &gt there.

> >+      <method name="_fireOnSelect">
> >+        <body>
> >+        <![CDATA[
> >+          if (!this._suppressOnSelect && !this.suppressOnSelect) {
> >+            var event = document.createEvent("Events");
> >+            event.initEvent("select", true, true);
> >+            this.dispatchEvent(event);
> >+          }
> >+        ]]>
> >+        </body>
> >+      </method>
> >+
> 
> Shouldn't this live in the base binding?

I guess no, richlistbox makes something special in that method. Therefore I can't share that implementation.

> >            for (var i = 0; i < rowCount; i++) {
> >              var k = (i + start + c) % rowCount;
> >              var item = this.getItemAtIndex(k); //listitem
> >-             var cellText = item.getAttribute("label");
> >+             // fayt_label allows to display different text than is returned
> >+             // (in case a visible image is represented through text in the label)
> >+             var cellText = "fayt_label" in item ? item.fayt_label : item.label;
> 
> What's this about?

No one idea :). I removed this since this presented neither in listbox nor richlistbox before. Simon, if you have reason to keep it here please let us know.

> >     <implementation implements="nsIDOMXULSelectControlItemElement, nsIAccessibleProvider">
> >       <property name="current" onget="return this.getAttribute('current') == 'true';">
> >         <setter><![CDATA[
> >           if (val)
> >             this.setAttribute("current", "true");
> >           else
> >             this.removeAttribute("current");
> >+
> >+          this._fireEvent(val ? "DOMMenuItemActive" : "DOMMenuItemInactive");
> 
> I suppose aaronlev approved this?

In trunk "DOMMenuItemActive" is fired in listbox.currentItem() method. Simon moved this event into this property implemntation to localize I guess this call. But currently "DOMMenuItemInactive" isn't used and afaik this event is not important for ally code. So, what should I do?

> 
> >Index: toolkit/content/widgets/richlistbox.xml
> >===================================================================
> 
> >+    <!-- Overriding baselistbox -->
> >+      <method name="_fireOnSelect">
> ...
> >+            // make sure not to modify last-selected when suppressing select events
> >+            // (otherwise we'll lose the selection when a template gets rebuilt)
> >+            if (this._suppressOnSelect || this.suppressOnSelect)
> >+              return;
> >+
> >+            // remember the current item and all selected items with IDs
> >+            var state = this.currentItem ? this.currentItem.id : "";
> 
> What if it doesn't have an id?

Original richlistbox has dependencies on id attribute. Attribute "last-selected" contains a list of id of item elements. I don't like myself a lot any id stuff. But I'd like rather file new bug for this than to try to fix it here.

> 
> >+      <property name="children" readonly="true">
> >+        <getter>
> >+          <![CDATA[
> >+            var childNodes = [];
> >+            for (var child = this.firstChild; child; child = child.nextSibling) {
> >+              if (child instanceof Components.interfaces.nsIDOMXULSelectControlItemElement)
> >+                childNodes.push(child);
> >+            }
> >+            return childNodes;
> >+          ]]>
> >+        </getter>
> >+      </property>
> >+
> 
> You can use this.childNodes instead of this property (and make sure the
> "includes" attribute is set to "richlistitem" on the <children> element of this
> binding).

This is also from original richlistbox code. But if you like then I'll for sure fix this.

> >+      <method name="_refreshSelection">
> >         <body>
> >           <![CDATA[
> ...
> >+              var suppressSelect = this._suppressOnSelect;
> >+              this._suppressOnSelect = true;
> >+              this.clearSelection();
> >+              for (var i = 1; i < ids.length; i++) {
> >+                var selectedItem = document.getElementById(ids[i]);
> >+                if (selectedItem)
> >+                  this.addItemToSelection(selectedItem);
> >+              }
> >+
> >+              var currentItem = document.getElementById(ids[0]);
> >+              if (!currentItem && this._currentIndex)
> >+                currentItem = this.getItemAtIndex(Math.min(
> >+                  this._currentIndex - 1, this.getRowCount()));
> >+              if (currentItem) {
> >+                this.currentItem = currentItem;
> >+                if (this.selType != "multiple" && this.selectedCount == 0)
> >+                  this.selectedItem = currentItem;
> >+
> >+                if (this.scrollBoxObject.height)
> >+                  this.ensureElementIsVisible(currentItem);
> >+                else // XXX hack around a bug in ensureElementIsVisible
> 
> bug # ?

Dunno. I failed to find something like. Simon, can you help?

> >+                  this.ensureElementIsVisible(currentItem.previousSibling);
> >+              }
> >+              this._suppressOnSelect = suppressSelect;
> >+              // XXX actually it's just a refresh, but at least
> >+              // the Extensions manager expects this:
> >+              this._fireOnSelect();
> 
> so why is _suppressOnSelect set to true then?

Why on true? That value what was used previosuly I guess.

> >+            if (this.selType != "multiple" && this.selectedCount == 0)
> >+              this.selectedItem = this.currentItem;
> >+
> >+            // XXX hack for the Downloads manager (better to focus the list than
> >+            // the individual items - these usually aren't tabbable anyway, and
> >+            // we need the keyboard focus for navigation):
> >+            this.focus();
> 
> Download manager hacks in the download manager code please :)

Should I file new bug? Ok? :) This live here during some time already.

> 
> >Index: toolkit/themes/pinstripe/global/richlistbox.css
> >===================================================================
> 
> > 
> > richlistbox {
> >+  margin: 2px 4px;
> 
> Why is this change?

This is trying to make look like the rest xul elements is looked.

Comment 27

[Simon Bünzli]

(In reply to comment #26)
> > >+      <method name="_fireOnSelect">
> > 
> > Shouldn't this live in the base binding?
> 
> I guess no, richlistbox makes something special in that method. Therefore I
> can't share that implementation.

If you have it in the base binding, you can always override it in the richlistbox specific binding afterwards.

> > >+             // fayt_label allows to display different text than is returned
> > >+             // (in case a visible image is represented through text in the label)
> > >+             var cellText = "fayt_label" in item ? item.fayt_label : item.label;
> > 
> > What's this about?

This patch enables as a side-effect FAYT for richlistboxes (as requested by bug 298993). Now, per default the richlistitems' label property is used for FAYT to work with. That property might however also contain text from invisible labels or text representation of graphical elements. So I ended up allowing richlistitems to explicitly state what FAYT should work with without putting any limitations on the label property. See the Console² extension for an example where this is used.

> > >+                if (this.scrollBoxObject.height)
> > >+                  this.ensureElementIsVisible(currentItem);
> > >+                else // XXX hack around a bug in ensureElementIsVisible
> > 
> > bug # ?

I don't know where the real issue lies, it just happens that ensureElementIsVisible scrolls one item too far, when the richlistbox is initialized (i.e. while it's not correctly layed out).

Comment 28

[alexander :surkov (:asurkov)]

(In reply to comment #27)
> (In reply to comment #26)
> > > >+      <method name="_fireOnSelect">
> > > 
> > > Shouldn't this live in the base binding?
> > 
> > I guess no, richlistbox makes something special in that method. Therefore I
> > can't share that implementation.

> If you have it in the base binding, you can always override it in the
> richlistbox specific binding afterwards.

Yes, but I can't see any advantages of this.

> 
> > > >+             // fayt_label allows to display different text than is returned
> > > >+             // (in case a visible image is represented through text in the label)
> > > >+             var cellText = "fayt_label" in item ? item.fayt_label : item.label;
> > > 
> > > What's this about?
> 
> This patch enables as a side-effect FAYT for richlistboxes (as requested by bug
> 298993). Now, per default the richlistitems' label property is used for FAYT to
> work with. That property might however also contain text from invisible labels
> or text representation of graphical elements. So I ended up allowing
> richlistitems to explicitly state what FAYT should work with without putting
> any limitations on the label property. See the Console² extension for an
> example where this is used.

Then I'd like to save this code for bug 298993.

Comment 29

[alexander :surkov (:asurkov)]

Attachment: patch7

(In reply to comment #25)

> 
> >-      <method name="insertItemAt">
> ....
> >-          var XULNS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
> >-          var item = document.createElementNS(XULNS, "listitem");
> >-          item.setAttribute("label", label);
> >-          item.setAttribute("value", value);
> >-          var before = this.getItemAtIndex(index);
> >-          if (before)
> >-            this.insertBefore(item, before);
> >-          else
> >-            this.appendChild(item);
> >-          return item;
> ..
> >         </body>
> >       </method>
> 
> >+      <method name="insertItemAt">
> >+        <parameter name="index"/>
> >+        <parameter name="label"/>
> >+        <parameter name="value"/>
> >+        <body>
> >+        <![CDATA[
> >+          var XULNS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
> >+          var item = document.createElementNS(XULNS, "listitem");
> >+          item.setAttribute("label", label);
> >+          item.setAttribute("value", value);
> >+          try {
> >+            var before = this.getItemAtIndex(index);
> >+            this.insertBefore(item, before);
> >+          } catch (e) {
> >+            this.appendChild(item);
> >+          }
> >+          return item;
> >+        ]]>
> >+        </body>
> >+      </method>
> 
> I don't think this change is right (i.e. you should keep |if (before) instead
> of the try..catch block), what was the reasoning behind this change?

Fixed. The issue is saved for bug 308292.

> 
> >+    <!-- nsIDOMXULSelectControlElement -->
> >+      <property name="selectedItem">
> 
> maybe use onget/onset instead?
> 

Fixed, I use onset.

> 
> >+          var XULNS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
> 
> please change this to "const" while you are here.

Fixed.

> >-             var cellText = item.getAttribute("label");
> >+             // fayt_label allows to display different text than is returned
> >+             // (in case a visible image is represented through text in the label)
> >+             var cellText = "fayt_label" in item ? item.fayt_label : item.label;
> 
> What's this about?

Fixed. Saved for bug 298993.

> >+      <property name="children" readonly="true">
> You can use this.childNodes instead of this property (and make sure the
> "includes" attribute is set to "richlistitem" on the <children> element of this
> binding).

Fixed.

Comment 30

[Asaf Romano (gone)]

Comment on attachment 248078 [details] [diff] [review]
patch7

Index: toolkit/content/widgets/listbox.xml
===================================================================


       <method name="invertSelection">
         <body>
-        <![CDATA[

Please avoid this kind of change.

+    <!-- Other public members -->
+      <property name="disableKeyNavigation">
+        <getter>
+          return this.hasAttribute("disableKeyNavigation");
+        </getter>
+        <setter>
+          if (val)
+            this.setAttribute("disableKeyNavigation", "true");
+          else
+            this.removeAttribute("disableKeyNavigation");
+          return val;
+        </setter>
+      </property>


I prefer the simplified onget/onset property from current listbox.xml.

+      <method name="moveByOffset">
...
+          if (newItem) {
+            this.ensureIndexIsVisible(newIndex);
+            if (aIsSelectingRange) {
+              this.selectItemRange(null, newItem);
+            } else if (aIsSelecting) {
+              this.selectItem(newItem);
+            }

nit: remove braces around single lines.


Index: toolkit/content/widgets/richlistbox.xml
===================================================================

     <content>
       <xul:scrollbox allowevents="true" orient="vertical" anonid="main-box"
                      flex="1" style="overflow: auto;">
-        <children />
+        <children includes="rishlistitem"/>

[Sigh] I was wrong earlier, other child nodes would be inserted here as well (default behavior), you could be safe and check each node's localname when using this.childNodes, but I don't think you should.

So, please change this back to <children/>.

r=mano otherwise. Though:
 * I didn't review each moved-method here.
 * This needs a unit test (for multi-selectable rich/listbox).
 * Still interested in Enn's review as well.

Comment 31

[alexander :surkov (:asurkov)]

Attachment: patch8

Comment 32

[alexander :surkov (:asurkov)]

(In reply to comment #30)

>  * This needs a unit test (for multi-selectable rich/listbox).

What form of unit test should be and where the test should be placed in tree? Can you point me some example?

Comment 33

[Nickolay_Ponomarev]

Seemingly relevant quote from #developers:

<NeilAway> bsmedberg: out of interest, how do you unit-test a ui widget?
<bsmedberg> NeilAway: 1) you make sure that <xul:calendar> or whatever the tagname has the correct properties, and you can set and retrieve them reliably
<bsmedberg> NeilAway: 2) you fire keyboard events at it to simulate user actions
<bsmedberg> and make sure that the right dates are selected as a result

If you need more info, you could try talking to bsmedberg.

Comment 34

[Neil Deakin]

Comment on attachment 251083 [details] [diff] [review]
patch8

>+   <!-- Interface binding that is base for bindings of xul:listbox and
>+       xul:richlistbox elements. This binding assumes that successors bindings
>+       will implement the following methods:

Should use the preprocessor here probably to strip out this giant comment.

> 
>   <binding id="listrows"
>-           extends="chrome://global/content/bindings/listbox.xml#listbox-base">
>+           extends="chrome://global/content/bindings/general.xml#basecontrol">
>+

Neither listrows, listheader or listhead should extend basecontrol.

>       
>       <property name="selected" onget="return this.getAttribute('selected') == 'true';">
>         <setter><![CDATA[
>           if (val)
>             this.setAttribute("selected", "true");
>           else
>             this.removeAttribute("selected");
> 
>-          var event = document.createEvent("Events");
>-          event.initEvent("DOMMenuItemActive", true, true);
>-          this.dispatchEvent(event);
>+          if (!this.control || this.control.selType == "multiple")
>+            this._fireEvent(val ? "DOMItemSelected" : "DOMItemUnselected");
> 

Why are these events being fired? Make sure to ask aaron to look at this method (and the 'current' setter)

>     <handlers>
>       <!-- If there is no modifier key, we select on mousedown, not
>            click, so that drags work correctly. -->
>       <handler event="mousedown">
>       <![CDATA[
>-        if (this.parentNode.disabled) return;
>+        var control = this.control;
>+        if (control.disabled) return;

This should return null if control is null also.

> 
>       <!-- On a click (up+down on the same item), deselect everything
>            except this item. -->
>       <handler event="click" button="0">
>       <![CDATA[
>-        if (this.parentNode.disabled) return;
>-        if (parentNode.selType != "multiple") {
>-          parentNode.selectItem(this);
>+        var control = this.control;
>+        if (control.disabled) return;

Same here.

>-  <binding id="richlistbox">
>+  <binding id="richlistbox"

>+      <method name="_fireOnSelect">
>+
>+            // remember the current item and all selected items with IDs
>+            var state = this.currentItem ? this.currentItem.id : "";
>+            if (this.selType == "multiple" && this.selectedCount) {
>+              function getId(aItem) { return aItem.id; }
>+              state += " " + this.selectedItems.filter(getId).map(getId).join(" ");
>             }

Is it possible for currentItem to be null while there are selected items? Probably not, but if there was, an extra space would be prepended here

>+            // preserve the index just in case no IDs are available
>+            if (this.currentIndex > -1)
>+              this._currentIndex = this.currentIndex + 1;

richlistbox._currentIndex isn't defined in a <field> anywhere

>+      <method name="getItemAtIndex">
>+        <parameter name="aIndex"/>
>+        <body>
>+          return this.children[aIndex] || null;

This will cause a strict warning. Instead just check if it is greater than the length.

>+      <method name="ensureIndexIsVisible">
>+        <parameter name="aIndex"/>
>         <body>

Please don't implement scrollToIndex or ensureElementIsVisible. Bug 364612 will implement scrolling apis for xul elements so no point adding other methods that will be obsolete anyway.
If possible, don't have ensureIndexIsVisible either, and just keep ensureSelectedIndexIsVisible for compatibility. 

> 
>       <method name="scrollOnePage">
>+            // If the current item is visible, we scroll by one page so that
>+            // the newly current item is at approximately the same position as
>+            // the actually current one

Use instead:
            // If the current item is visible, scroll by one page so that
            // the new current item is at approximately the same position as
            // the existing current item

> 
>   <binding id="richlistitem"
>-           extends="chrome://global/content/bindings/general.xml#basecontrol">
>+           extends="chrome://global/content/bindings/listbox.xml#listitem">
>-    <implementation implements="nsIAccessibleProvider, nsIDOMXULSelectControlItemElement">
>+    <implementation implements="nsIDOMXULSelectControlItemElement, nsIAccessibleProvider">

The interfaces implemented should be inherited

>+      <property name="label" readonly="true">
>         <!-- Setter purposely not implemented; the getter returns a
>-             concatentation of label text to expose via accessibility APIs-->
>+             concatentation of label text to expose via accessibility APIs -->
>         <getter>
>           <![CDATA[
>+            var XULNS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";

Use a const here
 
>+
>+richlistbox[seltype="multiple"]:focus > richlistitem[current="true"][selected="true"] {
>+  outline: 1px dotted #F3D982; /* TODO: find a suiting system color */
>+}

s/suiting/suitable/ in both places

This is good, but one more patch should do it.

Comment 35

[Simon Bünzli]

(In reply to comment #34)
> >+   <!-- Interface binding that is base for bindings of xul:listbox and
> Should use the preprocessor here probably to strip out this giant comment.

And please make sure that everything is indented to the same column.

> >+        var control = this.control;
> >+        if (control.disabled) return;
> This should return null if control is null also.

s/null//, so: if (!control || control.disabled) return;

> Is it possible for currentItem to be null while there are selected items?
> Probably not, but if there was, an extra space would be prepended here

That extra space would still be required. If currentItem was null, we would somehow have to know later on that we don't have to set any item to current when restoring the list.

> >+          return this.children[aIndex] || null;
> This will cause a strict warning.

Little known facts about JavaScript: the "|| null" bit prevents the strict warning.

> Please don't implement scrollToIndex or ensureElementIsVisible.

Aren't they required for the proper functioning of the patch until bug 364612 is fixed? At worst, call them _scrollToIndex and _ensureElementIsVisible and add a "this will go away soon" comment.

Comment 36

[alexander :surkov (:asurkov)]

Attachment: patch9

Comment 37

[alexander :surkov (:asurkov)]

(In reply to comment #34)
> (From update of attachment 251083 [details] [diff] [review])
> >+   <!-- Interface binding that is base for bindings of xul:listbox and
> >+       xul:richlistbox elements. This binding assumes that successors bindings
> >+       will implement the following methods:
> 
> Should use the preprocessor here probably to strip out this giant comment.

I think it's usefull to have the comment inside toolkit.jar. If you are not against then I'd like to save it.

> >+          if (!this.control || this.control.selType == "multiple")
> >+            this._fireEvent(val ? "DOMItemSelected" : "DOMItemUnselected");
> > 
> 
> Why are these events being fired?

Simon, why these events are needed?

> Please don't implement scrollToIndex or ensureElementIsVisible. Bug 364612 will
> implement scrolling apis for xul elements so no point adding other methods that
> will be obsolete anyway.
> If possible, don't have ensureIndexIsVisible either, and just keep
> ensureSelectedIndexIsVisible for compatibility. 

I saved these methods and I added proper XXX comment sections.

> >+richlistbox[seltype="multiple"]:focus > richlistitem[current="true"][selected="true"] {
> >+  outline: 1px dotted #F3D982; /* TODO: find a suiting system color */
> >+}
> 
> s/suiting/suitable/ in both places
> 
> This is good, but one more patch should do it.
> 

Do you prefer to remove these lines in any way? ;)

Comment 38

[alexander :surkov (:asurkov)]

Attachment: patch10

Comment 39

[alexander :surkov (:asurkov)]

(In reply to comment #34)

> Why are these events being fired? Make sure to ask aaron to look at this method
> (and the 'current' setter)

The question why do we need "DOMItemSelected"/"DOMItemUnselected" should be addressed to Simon I guess. These events are not used in accessibility. "DOMMenuItemActive" and "select" are important for ally only. So, the 'current' method should fire "DOMMenuItemActive" and when selection is changed then 'select' event should be fired. That is how the patch does.

Comment 40

[Simon Bünzli]

(In reply to comment #37)
> Simon, why these events are needed?

I've added them because of comment #2. DOMItem(Un)Selected seems to have been removed from Mozilla code about two years ago, though. DOMMenuItemActive seems kind of wrong however, so it's probably |select| you'll want to fire if |val| is |true|.

In fact, |select| could/should probably be removed from the main (rich)listbox and only be fired by the (rich)listitems, leaving the (rich)listbox with intercepting them if suppressOnSelect is true. That'd be for another bug though -- and for Aaron to figure out the details...

Comment 41

[Neil Deakin]

Comment on attachment 251549 [details] [diff] [review]
patch10

>-      <method name="clearSelection">
>+      <method name="getItemAtIndex">
>+        <parameter name="aIndex"/>
>+        <body>
>+        <![CDATA[
>+          if (aIndex < this.children.length)
>+            return this.children[aIndex];
>+          return null;
>+        ]]>
>+        </body>
>+      </method>

Actually, this is better the other way. See comment 35.

Comment 42

[alexander :surkov (:asurkov)]

Attachment: patch11

(In reply to comment #41)
> Actually, this is better the other way. See comment 35.
> 

Ah, sorry, I missed it.

Comment 43

[alexander :surkov (:asurkov)]

Attachment: for checkin

Comment 44

[alexander :surkov (:asurkov)]

checked in by smaug

Comment 45

[Simon Bünzli]

Thanks for getting this in, Alexander!

One further question, though: Is there any reason the keypress handlers don't live in the base binding? AFAICT they're the same for both listbox and richlistbox. Otherwise, we'll get avoidable duplicate code when implementing FAYT in bug 298993 (resp. we'd already have most of it -- which was the other reasons I wanted to inherit from listbox in the first place).

Comment 46

[alexander :surkov (:asurkov)]

(In reply to comment #45)
> Thanks for getting this in, Alexander!
> 
> One further question, though: Is there any reason the keypress handlers don't
> live in the base binding? AFAICT they're the same for both listbox and
> richlistbox. Otherwise, we'll get avoidable duplicate code when implementing
> FAYT in bug 298993 (resp. we'd already have most of it -- which was the other
> reasons I wanted to inherit from listbox in the first place).
> 

Thank you.

Actually I can't see a reason. I didn't do it because they are a bit different. I just was afraid to broke something.

Comment 48

[Eric Shepherd [:sheppy]]

This was documented a while ago; setting to dev-doc-complete.

Comment 49

[Axel Grude]

Now that I am porting my Add-ons to thunderbird 66 I had to find to my dismay that (after the listbox element has already been deprecated) now richlistbox does not support insertItemAt() anymore? having appendItem is not enough I have some advanced sorting / cut / paste features - how would I go about rewriting my code without crippling speed penalties - is there an alternative to insertItemAt that I can use?