Closed Bug 301737 Opened 18 years ago Closed 18 years ago

Support multiple selection for xul:listbox

Categories

(Core :: Disability Access APIs, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: aaronlev, Assigned: aaronlev)

References

Details

(Keywords: access, fixed1.8)

Attachments

(2 files, 4 obsolete files)

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.
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.
Depends on: 301776
Attachment #190219 - Flags: review?(neil.parkwaycc.co.uk)
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.
Attachment #190219 - Flags: review?(neil.parkwaycc.co.uk) → review-
Attached patch Address Neil's comments (obsolete) — Splinter Review
-- 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.
Attachment #190219 - Attachment is obsolete: true
Attachment #190314 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #190314 - Flags: review?(neil.parkwaycc.co.uk)
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?
What is the ctrl+shift differentiation?
Attachment #190314 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #190314 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #190314 - Attachment is obsolete: true
Attachment #190356 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #190356 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #190356 - Attachment is obsolete: true
Attachment #190356 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #190356 - Flags: review?(neil.parkwaycc.co.uk)
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.
Attachment #190379 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #190379 - Flags: review?(neil.parkwaycc.co.uk)
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) :-(
Attachment #190379 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview+
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.
Attachment #190379 - Flags: review?(neil.parkwaycc.co.uk) → review?(mconnor)
Flags: blocking1.8b4+
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.
Attachment #190379 - Flags: review?(mconnor) → review+
Attachment #190379 - Flags: approval1.8b4?
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
Attachment #190379 - Flags: approval1.8b4? → approval1.8b4+
(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.
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
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Attached patch xpfe version of toolkit changes (obsolete) — Splinter Review
This patch:
* xpfe version of the checked in changes to toolkit's listbox.xml
Attachment #191798 - Flags: review?(cbiesinger)
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.
Attachment #191798 - Flags: review?(cbiesinger) → review+
Attachment #191798 - Flags: superreview?(neil.parkwaycc.co.uk)
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.
Attachment #191798 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview+
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
Attachment #191798 - Attachment is obsolete: true
Attachment #192443 - Flags: superreview+
Attachment #192443 - Flags: review+
Attachment #192443 - Flags: approval1.8b4?
Attachment #192443 - Flags: approval1.8b4? → approval1.8b4+
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
Attachment #192443 - Attachment description: Revised xpfe version of patch v2.1 → Revised xpfe version of patch v2.1 (Checked in trunk and 1.8 branch)
Keywords: fixed1.8
Depends on: 1190368
You need to log in before you can comment on or make changes to this bug.