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.

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.

-- 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 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?

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 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 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 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 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

(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

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

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 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.

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 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