Closed Bug 305730 Opened 17 years ago Closed 17 years ago

Support page up/down in richlistbox

Categories

(Toolkit :: XUL Widgets, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: zeniko, Assigned: zeniko)

Details

(Keywords: fixed1.8)

Attachments

(5 files, 4 obsolete files)

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 resolvedSplinter 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 lessSplinter 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: 17 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.