301737 - Support multiple selection for xul:listbox

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Aaron Leventhal]

The following keystrokes are not implemented but should be:
ctrl+up/ctrl+down/ctrl+home/ctrl+end/ctrl+pageup/ctrl+pagedown for moving focus
but not selection
ctrl+space for toggling selection on the current item

We also need to fire ItemSelected and ItemUnselected events for multiselects.

Comment 1

[Aaron Leventhal]

Also, not implemented:
shift+home, shift+end, shift+pageup, shift+pagedown

shift+arrow works incorrectly because the focus doesn't move to the new item.

Comment 2

[Aaron Leventhal]

Attachment: Implement keys correctly, and with code reuse

Comment 3

[[e-mail]]

Comment on attachment 190219 [details] [diff] [review]
Implement keys correctly, and with code reuse

This is a visual review based on my belief of the current XPFE tree behaviour,
because I haven't bothered to check whether toolkit and XPFE's listbox.xml are
in sync.

>+          // Ensure viable focus and selection
nsXULElement::RemoveChildAt is already supposed to fix up the current index and
selection; any correction needs to be applied there.

>-          this.clearSelection();
This changes the API :-( But you could add a third parameter to augment as per
nsITreeSelection::RangedSelect and pass event.ctrlKey to it.

>+      <handler event="keypress" keycode="vk_up" phase="target"
>+               action="moveByOffset(-1, true, false);"/>
>+      <handler event="keypress" keycode="vk_up" modifiers="shift" phase="target"
>+               action="moveByOffset(-1, true, true);"/>
>+      <handler event="keypress" keycode="vk_up" modifiers="control" phase="target"
You should be able to use <handler event="keypress" modifiers="control shift
any" ...> to capture all four combinations; you also have the choice of passing
the event to moveByOffset rather than the .shiftKey and the .ctrlKey
separately.

>+          if (this.currentItem.getAttribute("type") != "checkbox") {
>+            this.addItemToSelection(this.currentItem);
>+          }
>+          else if (!this.currentItem.disabled) {
>+            this.currentItem.checked = !this.currentItem.checked;
Inconsistent bracing style. Ask mconnor in case he has a preference.

Comment 4

[Aaron Leventhal]

Attachment: Address Neil's comments

-- Turns out I didn't need to modify removeItemAt at all.

-- I decided to keep moveByOffset params as they are since it is sometimes nice
to have a high level API rather than passing around events.

-- I changed selectItemRange to take an optional addToSelection param, but did
not use it in the unmodified key case, otherwise 2 or more items would get
selected then instead of 1. I kept the unmodified case as
this.selectItem(newItem);

-- I think it's cool that we're removing a lot more code than we're adding but
getting more functionality.

Comment 5

[[e-mail]]

Comment on attachment 190314 [details] [diff] [review]
Address Neil's comments

>+          if (typeof(addToSelection) == "undefined" || !addToSelection)
You don't have to check the type of addToSelection, !addToSelection will be
true if fewer parameters were passed.

>+              if (isAddingToSelection) {
>+                if (offset > 0)
>+                  this.selectItemRange(this.currentItem, newItem, true);
>+                else
>+                  this.selectItemRange(newItem, this.currentItem, true);
>+              }
Is it worth going for the ctrl+shift vs shift differentiation here while you're
at it?

>-         this._isUpSelection=0;
>-         this._isDownSelection=0;
Hmm... are these obsolete?

Comment 6

[Aaron Leventhal]

What is the ctrl+shift differentiation?

Comment 7

[Aaron Leventhal]

Attachment: Address Neil's comments, although I don't know what ctrl+shift is supposed to do

Comment 8

[Aaron Leventhal]

Attachment: The last patch didn't reduce the selection when you went shift+down then shift+up, or vice-versa. This patch also fixes the firing of accessible selection_* events

The key is to let the _selectionStart magic happen, but not to completely clear
selection and then reselecting the desired range. That leads to too many
accessibility events being fired because some items get unselected and
reselected in the same operation.

Comment 9

[[e-mail]]

Comment on attachment 190379 [details] [diff] [review]
The last patch didn't reduce the selection when you went shift+down then shift+up, or vice-versa. This patch also fixes the firing of accessible selection_* events

I haven't tested this nor do I understand the C++ changes so I feel I can't
(yet) set the r flag either way.

>-          while (currentItem) {
>+          var done = false;
>+          while (currentItem && !done) {
>             if (currentItem.localName == "listitem")
>               this.addItemToSelection(currentItem);
>             if (currentItem == endItem)
>-              break;
>+              done = true;
>             currentItem = this.getNextItem(currentItem, 1);
>           }
This appears to goes out of its way to exit with currentItem equal to
this.getNextItem(endItem, 1); if that's what you really want then it would be
simpler and more obvious just to set it explicitly below. sr=me with this
fixed.

>+          // Clear around new selection
>+          // Don't use clearSelection() because it causes a lot of noise
>+          // with respect to selection removed notifications used by the
>+          // accessibility API support.
OK, but shouldn't the notifications be suppressed somewhere?

>+                this.selectItemRange(null, newItem);
A nice improvement :-) But you gave up on Ctrl+Shift+Arrow (for now) :-(

Comment 10

[Aaron Leventhal]

Comment on attachment 190379 [details] [diff] [review]
The last patch didn't reduce the selection when you went shift+down then shift+up, or vice-versa. This patch also fixes the firing of accessible selection_* events

Already has sr=neil. I've tested it thoroughly.

Comment 11

[Mike Connor [:mconnor]]

Comment on attachment 190379 [details] [diff] [review]
The last patch didn't reduce the selection when you went shift+down then shift+up, or vice-versa. This patch also fixes the firing of accessible selection_* events

ok, r=me with Neil's comments addressed.  I think I have a handle on the C++
changes, at least enough to say "ok" to what you're doing.

Comment 12

[Mike Kaply [:mkaply]]

Comment on attachment 190379 [details] [diff] [review]
The last patch didn't reduce the selection when you went shift+down then shift+up, or vice-versa. This patch also fixes the firing of accessible selection_* events

a=mkaply with neil's comments addressed

Comment 13

[Aaron Leventhal]

(In reply to comment #9)
> (From update of attachment 190379 [details] [diff] [review] [edit])

- I dealt with Neil's comment about removing |var done|
- On the notification suppression -- we actually want notifications, but only on
the changes. If we clear everything and then select the range, we get double
notifcations for the things that stay selected. However, we don't want to
suppress notifications in general, for any of the cases.

> >+                this.selectItemRange(null, newItem);
> A nice improvement :-) But you gave up on Ctrl+Shift+Arrow (for now) :-(
Neil and I discussed it on IRC and noticed that Windows explorer doesn't
implement it. It was never something I had even heard of, so I'm not sure we
should bother.

Comment 14

[Aaron Leventhal]

Checking in accessible/src/base/nsDocAccessible.h;
/cvsroot/mozilla/accessible/src/base/nsDocAccessible.h,v  <--  nsDocAccessible.h
new revision: 1.30; previous revision: 1.29
done
Checking in accessible/src/base/nsDocAccessible.cpp;
/cvsroot/mozilla/accessible/src/base/nsDocAccessible.cpp,v  <--  nsDocAccessible.cpp
new revision: 1.71; previous revision: 1.70
done
Checking in toolkit/content/widgets/listbox.xml;
/cvsroot/mozilla/toolkit/content/widgets/listbox.xml,v  <--  listbox.xml
new revision: 1.15; previous revision: 1.14
done

Comment 15

[Ian Neal]

Attachment: xpfe version of toolkit changes

This patch:
* xpfe version of the checked in changes to toolkit's listbox.xml

Comment 16

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 191798 [details] [diff] [review]
xpfe version of toolkit changes

r+, meaning that this does indeed port the patch to xpfe, so that the files
only differ in bug 280153's patches. I assume that the reviews of the other
patch are valid for the actual code in xpfe too.

Comment 17

[[e-mail]]

Comment on attachment 191798 [details] [diff] [review]
xpfe version of toolkit changes

>       <property name="currentIndex">
>         <getter><![CDATA[
>           return this.getIndexOfItem(this.currentItem);
Since we don't have aaron's dodgy autoselection code we have to make sure that
currentIndex doesn't throw an exception by returning -1 if there is no current
item (selectedIndex has to make a similar check). sr=me with this fixed.

>+            var newIndex = this.currentIndex + offset;
>+            if (newIndex < 0)
>+              newIndex = 0;
>+            var numItems = this.getRowCount();
>+            if (newIndex > numItems - 1)
>+              var newIndex = numItems - 1;
Please also remove the unnecessary redeclaration of newIndex.

I noticed that shifted selection is really slow even in listboxes with only 163
elements, but I can't think of any good ways of speeding it up.

Comment 18

[Ian Neal]

Attachment: Revised xpfe version of patch v2.1 (Checked in trunk and 1.8 branch)

Changes v2.0
* Made suggested changes to currentIndex
* Removed unnecessary redeclaration of newIndex

Carrying forward r= and sr=

Requesting a= for this accessibility patch for suite

Comment 19

[Ian Neal]

Comment on attachment 192443 [details] [diff] [review]
Revised xpfe version of patch v2.1 (Checked in trunk and 1.8 branch)

Checking in (trunk)
listbox.xml;
new revision: 1.26; previous revision: 1.25
done
Checking in (1.8 branch)
mozilla/xpfe/global/resources/content/bindings/listbox.xml;
new revision: 1.25.2.1; previous revision: 1.25
done