make richlistbox multi-selectable

RESOLVED FIXED

Status

()

Toolkit
XUL Widgets
RESOLVED FIXED
12 years ago
10 years ago

People

(Reporter: Doron Rosenberg (IBM), Assigned: surkov)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

unspecified
x86
All
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 12 obsolete attachments)

(Reporter)

Description

12 years ago
 

Comment 1

12 years ago
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

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

Comment 3

12 years ago
Created attachment 197836 [details] [diff] [review]
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

12 years ago
Created attachment 197925 [details] [diff] [review]
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).

Updated

12 years ago
Attachment #197836 - Attachment is obsolete: true

Comment 5

12 years ago
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.
Attachment #197925 - Attachment is obsolete: true
(Reporter)

Comment 6

12 years ago
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.

Updated

12 years ago
Blocks: 228842

Comment 7

12 years ago
Created attachment 205541 [details] [diff] [review]
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.
Attachment #205541 - Flags: first-review?(doronr)
(Reporter)

Comment 8

12 years ago
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

12 years ago
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

12 years ago
One place to discuss the design issues involved in fixing this bug: http://wiki.mozilla.org/XUL:Richlistbox
(Reporter)

Comment 11

12 years ago
Comment on attachment 205541 [details] [diff] [review]
Take 3

can you update this to trunk?
Attachment #205541 - Flags: first-review?(doronr) → first-review-

Comment 12

12 years ago
Created attachment 217033 [details] [diff] [review]
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?
Attachment #205541 - Attachment is obsolete: true
Attachment #217033 - Flags: first-review?(doronr)
(Reporter)

Comment 13

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

Comment 14

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

Mike, who else could review this patch?
Attachment #217033 - Flags: first-review?(doronr) → first-review?(mconnor)
(Reporter)

Comment 15

11 years ago
I'll review it once a toolkit peer oks the concept.
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?
Attachment #217033 - Flags: first-review?(mconnor) → first-review-

Comment 17

11 years ago
(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.
(Assignee)

Comment 18

11 years ago
(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.
Assignee: doronr → surkov.alexander
(Assignee)

Comment 19

11 years ago
Created attachment 242646 [details] [diff] [review]
patch4

Mainly all I did is moving of common methods of listbox and richlistbox to baselistbox.xml
Attachment #217033 - Attachment is obsolete: true
Attachment #242646 - Flags: first-review?(mano)
(Assignee)

Updated

11 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 20

11 years ago
(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 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...
Attachment #242646 - Flags: first-review?(mano) → first-review-
(Assignee)

Comment 22

11 years ago
Created attachment 242669 [details] [diff] [review]
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.
Attachment #242646 - Attachment is obsolete: true
Attachment #242669 - Flags: first-review?(mano)
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.
Attachment #242669 - Flags: first-review?(mano) → first-review-
(Assignee)

Comment 24

11 years ago
Created attachment 245836 [details] [diff] [review]
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.
Attachment #242669 - Attachment is obsolete: true
Attachment #245836 - Flags: first-review?(mano)
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.
Attachment #245836 - Flags: first-review?(mano) → first-review-
(Assignee)

Comment 26

11 years ago
(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

11 years ago
(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).
(Assignee)

Comment 28

11 years ago
(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.
(Assignee)

Comment 29

11 years ago
Created attachment 248078 [details] [diff] [review]
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.
Attachment #245836 - Attachment is obsolete: true
Attachment #248078 - Flags: first-review?(mano)
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.
Attachment #248078 - Flags: first-review?(mano) → first-review+
(Assignee)

Comment 31

11 years ago
Created attachment 251083 [details] [diff] [review]
patch8
Attachment #248078 - Attachment is obsolete: true
(Assignee)

Updated

11 years ago
Attachment #251083 - Flags: second-review?(enndeakin)
(Assignee)

Comment 32

11 years ago
(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

11 years ago
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 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.
Attachment #251083 - Flags: second-review?(enndeakin) → second-review-

Comment 35

11 years ago
(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.
(Assignee)

Comment 36

11 years ago
Created attachment 251546 [details] [diff] [review]
patch9
Attachment #251083 - Attachment is obsolete: true
(Assignee)

Comment 37

11 years ago
(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? ;)
(Assignee)

Updated

11 years ago
Attachment #251546 - Flags: second-review?(enndeakin)
(Assignee)

Comment 38

11 years ago
Created attachment 251549 [details] [diff] [review]
patch10
Attachment #251546 - Attachment is obsolete: true
Attachment #251549 - Flags: second-review?(enndeakin)
Attachment #251546 - Flags: second-review?(enndeakin)
(Assignee)

Comment 39

11 years ago
(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

11 years ago
(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 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.
Attachment #251549 - Flags: second-review?(enndeakin) → second-review+
(Assignee)

Comment 42

11 years ago
Created attachment 251637 [details] [diff] [review]
patch11

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

Ah, sorry, I missed it.
Attachment #251549 - Attachment is obsolete: true
(Assignee)

Comment 43

11 years ago
Created attachment 251640 [details] [diff] [review]
for checkin
Attachment #251637 - Attachment is obsolete: true
(Assignee)

Updated

11 years ago
Flags: in-testsuite?
(Assignee)

Comment 44

11 years ago
checked in by smaug
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Comment 45

11 years ago
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).

Updated

11 years ago
Keywords: dev-doc-needed
(Assignee)

Comment 46

11 years ago
(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.
Duplicate of this bug: 335642
This was documented a while ago; setting to dev-doc-complete.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.