Closed Bug 305730 Opened 19 years ago Closed 19 years ago

Support page up/down in richlistbox

Categories

(Toolkit :: UI Widgets, enhancement)

Product:
Toolkit
Component:
UI Widgets
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: zeniko, Assigned: zeniko)

Details

(Keywords: fixed1.8)

Attachments

(5 files, 4 obsolete files)

Nits resolved
4.08 KB, patch
mconnor
: first-review+
Details | Diff | Splinter Review
3 nits less
4.09 KB, patch
Details | Diff | Splinter Review
Performance improvement (incl. corrected start handling)
3.06 KB, patch
mconnor
: first-review+
Details | Diff | Splinter Review
follow-up: take margins into account
2.06 KB, patch
doronr
: first-review+
Details | Diff | Splinter Review
all-in-one branch patch
4.46 KB, patch
asa
: approval1.8b5+
Details | Diff | Splinter Review
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050823 Firefox/1.0+ Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050823 Firefox/1.0+ Keyboard navigation in a richlistbox is so far restricted to up/down and home/end. This makes navigating without a mouse in longer list (e.g. in the Download Manager) a pain. Additionally, in most other places page up/down is supported, which might cause the users to expect it here, too, making richlistbox appear broken to some degree. Reproducible: Always Steps to Reproduce: 1. Download some 30 different files (if you haven't yet). 2. Open the Download Manager - resize it to display only 5 files at once. 3. Try to launch the 14th file by just using the keyboard. Actual Results: You have to press [down] 13 times. Expected Results: Use [page down], until the file shows up. This reduces the necessary key strokes by at least half.
This pretty much fixes the issue, although I'm not sure whether all corner cases are handled correctly.
Attachment #193679 - Flags: first-review?(benjamin)
Assignee: nobody → zeniko
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attachment #193679 - Flags: first-review?(benjamin) → first-review?(doronr)
Comment on attachment 193679 [details] [diff] [review] Add page up/down support to the richlistbox widget >Index: richlistbox.xml >=================================================================== >RCS file: /cvsroot/mozilla/toolkit/content/widgets/richlistbox.xml,v >retrieving revision 1.14 >diff -U8 -r1.14 richlistbox.xml >--- richlistbox.xml 13 Aug 2005 22:32:14 -0000 1.14 >+++ richlistbox.xml 24 Aug 2005 08:51:05 -0000 >@@ -200,16 +200,106 @@ > return true; > } > } > return false; > ]]> > </body> > </method> > >+ <method name="isItemVisible"> >+ <parameter name="aItem"/> >+ <body> >+ <![CDATA[ >+ if (!aItem) { >+ return false; >+ } >+ >+ var y = {}; >+ this.scrollBoxObject.getPosition({}, y); >+ y.value += this.scrollBoxObject.y; // why is this correction necessary? >+ >+ // partially visible items are also considered visible >+ return (aItem.boxObject.y + aItem.boxObject.height >= y.value) && (aItem.boxObject.y < y.value + this.scrollBoxObject.height); >+ ]]> >+ </body> >+ </method> please call it _isItemVisible, since I don't think we want this as a public method. >+ >+ <method name="goPageUp"> >+ <body> >+ <![CDATA[ >+ if (this.getRowCount() == 0) { >+ return false; >+ } >+ >+ var current = this.selectedItem, y = {}; >+ >+ // if the selected item is not visible or no item is selected, >+ // select the first entirely visible item >+ if (!this.isItemVisible(current)) { >+ this.scrollBoxObject.getPosition({}, y); >+ y.value += this.scrollBoxObject.y; >+ } Is that the behavior of other controls? I noticed one issue: If I have 4 extensions, and I resize the window so only 2 are visible at any time. If I select the last one and page up, it goes to the first one in the list. Seems it would make more sense to select the 2nd item (meaning the last selected in the new view of the 1st and 2nd).
aaron: is this behavior correct keyboard behavior?
(In reply to comment #3) > aaron: is this behavior correct keyboard behavior? You can look at regular XUL listboxes now for completely correct keyboard behavior. It handles pgup/pgdn, multiple selection, incremental find as you type, etc.
At a quick glance, this impl appears to duplicate a lot of code between pageup and pagedown. I recommend looking at http://lxr.mozilla.org/seamonkey/source/toolkit/content/widgets/listbox.xml
Attached patch Simplified, better behaving fix (obsolete) — — Splinter Review
This implementation is still not as clean as the listbox one (since the items can have varying heights), but it should behave pretty much the same now.
Attachment #193679 - Attachment is obsolete: true
Attachment #193728 - Flags: first-review?(doronr)
Comment on attachment 193728 [details] [diff] [review] Simplified, better behaving fix >Index: richlistbox.xml >=================================================================== >RCS file: /cvsroot/mozilla/toolkit/content/widgets/richlistbox.xml,v >retrieving revision 1.14 >diff -U8 -r1.14 richlistbox.xml >--- richlistbox.xml 13 Aug 2005 22:32:14 -0000 1.14 >+++ richlistbox.xml 24 Aug 2005 18:21:26 -0000 >@@ -200,16 +200,78 @@ > return true; > } > } > return false; > ]]> > </body> > </method> > >+ <method name="_isItemVisible"> >+ <parameter name="aItem"/> >+ <body> >+ <![CDATA[ >+ if (!aItem) { >+ return false; >+ } >+ >+ var y = {}; >+ this.scrollBoxObject.getPosition({}, y); >+ y.value += this.scrollBoxObject.y; >+ >+ // partially visible items are also considered visible >+ return (aItem.boxObject.y + aItem.boxObject.height >= y.value) && (aItem.boxObject.y < y.value + this.scrollBoxObject.height); >+ ]]> >+ </body> >+ </method> Change it to: + return (aItem.boxObject.y + aItem.boxObject.height >= y.value) && + (aItem.boxObject.y < y.value + this.scrollBoxObject.height) If you prefer having the && on the 2nd line, fine too. Opinions vary one what is better :) >+ >+ <method name="scrollOnePage"> >+ <parameter name="aDirection"/> <!-- Must be -1 or 1 --> >+ <body> >+ <![CDATA[ >+ if (this.getRowCount() == 0) { >+ return false; >+ } >+ >+ var index = this.selectedIndex; >+ >+ // if nothing is selected, we start at the extreme we're moving away from >+ if (index == -1) { >+ index = (aDirection == -1)?this.getRowCount() - 1:0; >+ } Put some spaces around the "?" and ":" - so (foo) ? bar1 : bar2 need review from a toolkit peer though
Attachment #193728 - Flags: second-review?(mconnor)
Attachment #193728 - Flags: first-review?(doronr)
Attachment #193728 - Flags: first-review+
Attachment #193728 - Attachment is obsolete: true
Attachment #193740 - Flags: second-review?(mconnor)
Attachment #193740 - Flags: first-review?(doronr)
Comment on attachment 193740 [details] [diff] [review] Minor style corrections (cf. comment #7) looks fine, note that you really don't need r= from me since I ain't a peer.
Attachment #193740 - Flags: first-review?(doronr) → first-review+
Comment on attachment 193740 [details] [diff] [review] Minor style corrections (cf. comment #7) >+ <![CDATA[ >+ if (!aItem) { >+ return false; >+ } nit: kill unnecessary brackets >+ var y = {}; >+ this.scrollBoxObject.getPosition({}, y); >+ y.value += this.scrollBoxObject.y; >+ >+ // Partially visible items are also considered visible >+ return (aItem.boxObject.y + aItem.boxObject.height >= y.value) >+ && (aItem.boxObject.y < y.value + this.scrollBoxObject.height); despite what doron says, operators (excepting .) on the first line here is clearly specced in the style guide. >+ if (this.getRowCount() == 0) { >+ return false; >+ } nit: extra brackets >+ var index = this.selectedIndex; >+ please eliminate trailing whitespace like here >+ // If nothing is selected, we start at the extreme we're moving away from >+ if (index == -1) { >+ index = (aDirection == -1) ? this.getRowCount() - 1 : 0; >+ } nit: extra brackets >+ var current = this.getItemAtIndex(index); >+ if (this._isItemVisible(current)) { >+ this.scrollBoxObject.scrollBy(0, this.scrollBoxObject.height * aDirection); >+ } nit: extra brackets >+ var height = this.scrollBoxObject.height; >+ while (height >= 0 && 0 <= index && index < this.getRowCount()) { I'd prefer index >= 0 here, since that's convention. >+ if (index != this.selectedIndex) { >+ this.selectedItem = this.getItemAtIndex(index); >+ return true; >+ } >+ // Try to move by at least one item if the view port is too small >+ else if (aDirection == -1) { >+ return this.goUp(); >+ } >+ else { >+ return this.goDown(); >+ } else after return is ugly. if (foo) return 1; if (bar) return 2; return 3; please.
Attachment #193740 - Flags: second-review?(mconnor) → second-review+
Comment on attachment 193740 [details] [diff] [review] Minor style corrections (cf. comment #7) >+ var height = this.scrollBoxObject.height; >+ while (height >= 0 && 0 <= index && index < this.getRowCount()) { >+ height -= this.getItemAtIndex(index).boxObject.height; >+ index += aDirection; > Could you please add a comment describing your algorithm? If I understand it correctly, it is figuring out how many visible items are above (ie before) the selected item, then scrolls to the item right before the last visible item (ie thefirst item outside the visible range).
Attached patch Nits resolved — — Splinter Review
Apart from fixing all mentioned nits, I've made one minor correction to the height-determining algorithm: When scrolling up, it now sums the heights starting at the top of the currently selected item (instead of at the bottom). This should produce more appropriate results when the items are of varying sizes.
Attachment #193740 - Attachment is obsolete: true
Attachment #194013 - Flags: approval1.8b4?
Comment on attachment 194013 [details] [diff] [review] Nits resolved If you are changing algorithms, please get at least one person to re-review the patch before seeking approval.
Attachment #194013 - Flags: approval1.8b4?
Attachment #194013 - Flags: first-review?(mconnor)
Comment on attachment 194013 [details] [diff] [review] Nits resolved Please have doron check this out, I'm on vacation until after next weekend.
Attachment #194013 - Flags: first-review?(mconnor) → first-review?(doronr)
Attachment #193679 - Flags: first-review?(doronr)
Attachment #193728 - Flags: second-review?(mconnor)
Attachment #194013 - Flags: first-review?(doronr) → first-review?(mconnor)
Flags: blocking1.8b5?
Flags: blocking1.8b5? → blocking1.8b5-
Comment on attachment 194013 [details] [diff] [review] Nits resolved >+ // If nothing is selected, we start at the extreme >+ // we're moving away from >+ if (index == -1) >+ index = (aDirection == -1) ? this.getRowCount() - 1 : 0; nit: the brackets around aDirection == -1 aren't necessary here. >+ // If the selected item is visible, we scroll by one page so that >+ // the newly selected item is at approximately the same position as >+ // the currently selected one >+ var currentItem = this.getItemAtIndex(index); >+ if (this._isItemVisible(currentItem)) >+ this.scrollBoxObject.scrollBy(0,this.scrollBoxObject.height * aDirection); nit: spaces after commas >+ // Move by at least one item if the view port is too small >+ if (aDirection == -1) >+ return this.goUp(); >+ return this.goDown(); nit: blank line after the first return r=me with these last few nits addressed.
Attachment #194013 - Flags: first-review?(mconnor) → first-review+
Attached patch 3 nits less — — Splinter Review
Attachment #196118 - Flags: approval1.8b5?
Whiteboard: [checkin needed]
please land on the trunk and resolve there before requesting approval. Thanks.
Flags: blocking1.8b5- → blocking1.8b5+
Trunk: mozilla/toolkit/content/widgets/richlistbox.xml; new revision: 1.21;
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed]
Attached patch Performance improvement (obsolete) — — Splinter Review
I've just noted that my patch is rather wasteful with resources: I access the children getter at least 5 times (plus 2 times for each visible item), which shouldn't really be notable - but it might still give a bad example, and it doesn't scale well at all (I accidentally accessed it in an inner loop, around 200 times, which made Firefox hang for several seconds). The attached patch reduces this to one single call. If you don't want to do this, remind me to fix the one issue I've otherwise found: if (this.selectedItem != index) { should be replaced with if (this.selectedIndex != index) {
Attachment #196118 - Flags: approval1.8b5?
Comment on attachment 196593 [details] [diff] [review] Performance improvement I've finally had the chance of testing the performance of both versions in a pretty large richlistbox (~150 entries) and the difference is strikingly notable. I therefore ask you to include this follow-up patch (and at the same time the 3-line patch from Bug 309289 which fixes a minor issue in _isItemVisible). Thanks! Please ask doronr for the second-review if you deem it necessary.
Attachment #196593 - Flags: first-review?(mconnor)
One more thing I've noted while intensively using my code: if no item is selected, page up scrolls directly to the last second page, even if the last one wasn't visible. We'd probably rather want the last element to be shown and focused first and really scroll but the second time. (The same applies also for page down.) Now, I know that you're all very busy with the show-stoppers for 1.8b5, but could anybody please just give me a hint how to get these corrections checked into the trunk for testing and whether I should file them as a separate bug? Thanks.
Attachment #196593 - Attachment is obsolete: true
Attachment #197363 - Flags: first-review?(mconnor)
Attachment #196593 - Flags: first-review?(mconnor)
Attachment #197363 - Flags: first-review?(mconnor) → first-review+
last patch landed on trunk, this looks pretty solid, we should look at getting this in after tomorrow's trunk builds.
This will not work when there is a margin for the items. The reason why is that the loop only uses the boxObject.height and that does not include the margin. It seems like it would be better to base this of bo.y + bo.height? It looks that this issue only applies to page up and page down but I'm not sure? Should I open a new bug or reopen this?
This fixes the margin issue by measuring the distance between the top borders of two items (resp. the bottom borders when scrolling up).
Attachment #197678 - Flags: first-review?(doronr)
This patch includes the original patch, the two follow-up patches and the relevant half of the patch for bug 309289.
Attachment #197678 - Flags: first-review?(doronr) → first-review+
Attachment #197679 - Flags: approval1.8b5?
Attachment #197679 - Flags: approval1.8b5? → approval1.8b5+
Whiteboard: [checkin needed][a+]
Mike is going to check this into the branch today for Simon.
landed the all-in-one branch patch. Simon, please verify that everything has landed branch and trunk now.
Keywords: fixed1.8
Whiteboard: [checkin needed][a+]
The second follow-up patch (attachment 197678 [details] [diff] [review]) is still missing on the trunk, as is the part concerning _isItemVisible from my patch to bug 309289 (if you want, you can fix that last part by hand by replacing >= with > in _isItemVisible's return statement). Otherwise it's great, thanks.
(In reply to comment #28) > The second follow-up patch (attachment 197678 [details] [diff] [review] [edit]) is still missing on the > trunk, as is the part concerning _isItemVisible from my patch to bug 309289 Bug 323818 was filed for this, and richlistbox is now synced branch/trunk, minus the changes from bug 330190.
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: