317422 - Richlistbox accepts to give focus to 'display: none' items and blocks

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect)

Description

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

If one richlistitem in a richlistbox has 'display: none', you can still select
it moving selection with the up and down arrow keys. Once such an undisplayed
element is selected, arrow keys stop working (and that's normal).

richlistitem elements having computed value of 'none' for 'display' should not
be selectable.

Comment 1

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

taking.

Comment 2

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Attachment: patch #1

Already successfully used in my forthcoming OpenWengo extension. I also tortured the Extensions panel to verify nothing is horked.

Comment 3

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Comment on attachment 204024 [details] [diff] [review]
patch #1

Doron, can you review please?

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 204024 [details] [diff] [review]
patch #1

>+++ richlistbox.xml	2005-11-23 10:17:56.295251200 +0100

>+            // we won't select the item if it is hidden
>+            if (val && document.defaultView.getComputedStyle(val, "").getPropertyValue("display") == "none")

Other code uses just .display instead of .getPropertyValue, so if it's all the same, shorten up the lines? :)

>+            {
>+              this.clearSelection();
>+              return;
>+            }

The prevailing style in this file (and toolkit in general) is same-line opening brackets.

>@@ -255,6 +262,8 @@
>               this._selectedItem.selected = false
> 
>             this._selectedItem = aItem;
>+            if (aItem)
>+            {
>             this._selectedIndex = this.getIndexOf(aItem);
>             this.ensureSelectedElementIsVisible();
> 
>@@ -262,6 +271,9 @@
>               aItem.selected = true;
>               aItem.focus();
>             }
>+            }
>+            else
>+              this._selectedIndex = -1;

Why is this chunk necessary? getIndexOf already handles a null aItem (and returns -1), and ensureSelectedElementIsVisible does too (indirectly, since _selectedItem was just set to null). It also makes the inner null check useless...

>@@ -347,7 +363,16 @@
>             // at the extreme we're moving away from
>             if (index == -1) {
>               index = aDirection == -1 ? children.length - 1 : 0;
>-              this.selectedItem = children[index];
>+              var target = null;
>+              for (var i = index; i < children && i >= 0; i += aDirection)
>+              {
>+                target = children[i];
>+                if (document.defaultView.getComputedStyle(target , "").getPropertyValue("display") != "none")
>+                  break;
>+                target = null;
>+              }
>+              if (target)
>+                this.selectedItem = target;
>               return true;
>             }

Why not just set selectItem instead of breaking then checking whether it's null?
Like:
>+              for (var i = index; i < children && i >= 0; i += aDirection) {
>+                if (document.defaultView.getComputedStyle(children[i], "").display != "none")
>+                  this.selectedItem = children[i];
>+              }

Comment 5

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #4)
> Like:
> >+              for (var i = index; i < children && i >= 0; i += aDirection) {
> >+                if (document.defaultView.getComputedStyle(children[i], "").display != "none")
> >+                  this.selectedItem = children[i];
> >+              }

And add a break in there, of course :).

Comment 6

[Doron Rosenberg (IBM)]

If you fix gavin's comments, r=me :)

Comment 7

[Simon Bünzli]

(In reply to comment #4)
> >+            // we won't select the item if it is hidden
> >+            if (val && document.defaultView.getComputedStyle(val, "").getPropertyValue("display") == "none")
> 
> Other code uses just .display instead of .getPropertyValue, so if it's all the
> same, shorten up the lines? :)

What about testing for val.boxObject.height > 0 instead? Might be faster.

> Why not just set selectItem instead of breaking then checking whether it's
> null?
> Like:
> >+              for (var i = index; i < children && i >= 0; i += aDirection) {
> >+                if (document.defaultView.getComputedStyle(children[i], "").display != "none")
> >+                  this.selectedItem = children[i];
> >+              }

Even less code:
-              index = aDirection == -1 ? children.length - 1 : 0;
-              this.selectedItem = children[index];
-              return true;
+              return aDirection == -1 ? this.goUp() : this.goDown();

Comment 8

[Doron Rosenberg (IBM)]

Comment on attachment 204024 [details] [diff] [review]
patch #1

gavin's points

Comment 9

[Dave Townsend [:mossop]]

Attachment: patch rev 2

Unfortunately richlistbox has become a bit of a different beast and now shares a common parent binding with listbox which makes this a slightly different challenge.

Rather than override a bunch of the parent binding I've added an isListItem method to it. As standard (and for the unchanged listbox) it just returns true for nsIDOMXULSelectControlItemElement elements. For richlistbox we add the extra requirement that it be visible.

At some point in the future we could make listbox not select hidden items but that would require either completely changing it to not use nsIListBoxObject for everything, or change nsIListBoxObject itself.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 270346 [details] [diff] [review]
patch rev 2

>Index: toolkit/content/widgets/listbox.xml

>       <method name="selectItem">

>             return;
>+          // Don't select things that aren't list items

nit: newline after the return.

r=me, but this should have a unit test before landing.

Comment 11

[Simon Bünzli]

I'm not too happy with the isListItem method - especially since it unexpectedly returns |false| for (rich)listitems which just happen to be hidden.

What's wrong with adding a read-only "visible" property to listitem-base which just returns true and override that one for richlistitem? Instead of |this.isListItem(aItem)| you'd get |aItem.visible| which seems more readable/understandable to me.

If that doesn't convice you, you might at least want to ask one of the Neils for additional review.

Note to self: This check should also find its way into the FAYT handler from bug 298993.

Comment 12

[:Gavin Sharp [email: [e-mail]]]

All good points. Maybe you should have reviewed this one :)

Comment 13

[Simon Bünzli]

(In reply to comment #12)
> All good points. Maybe you should have reviewed this one :)

I would if they'd just let me. ;-)

BTW: Does this patch handle the case of collapsed richlistitems correctly (i.e. ignore them as well)?

Furthermore, there will still be several ways to select hidden items, e.g. through selectAll or invertSelection if the very first richlistitem happens to be hidden (all other hidden items are ignored) or through moveByOffset when not selecting but just focusing (e.g. through Ctrl+Up/Down).

There are probably more cases like this, so you'd want to first specify the expected behavior before tackling them individually. Most tricky will be the case where items are hidden _after_ having been selected...

Comment 14

[Dave Townsend [:mossop]]

(In reply to comment #11)
> What's wrong with adding a read-only "visible" property to listitem-base which
> just returns true and override that one for richlistitem? Instead of
> |this.isListItem(aItem)| you'd get |aItem.visible| which seems more
> readable/understandable to me.

Well I guess my main issue with that is that it requires all children of the (rich)listbox to be derived from listitem-base or have that property, whereas this allows the listbox to specify what it considers to be valid children.

(In reply to comment #13)
> BTW: Does this patch handle the case of collapsed richlistitems correctly (i.e.
> ignore them as well)?

probably not no but that could be added in the same way.

> Furthermore, there will still be several ways to select hidden items, e.g.
> through selectAll or invertSelection if the very first richlistitem happens to
> be hidden (all other hidden items are ignored) or through moveByOffset when not
> selecting but just focusing (e.g. through Ctrl+Up/Down).

I'm pretty confident that this isn't the case. All the methods you mention ultimately use getItemAtIndex which on richlistbox calls into the children property which only returns an array of elements that are valid items. They also use getNext and getPrevious which also use the added isListItem method to check the items they find.

Gavin, you've done the review, your call if you want any further review or changes here, I'll hold off till you let me know.

Comment 15

[Simon Bünzli]

(In reply to comment #14)
> it requires all children of the (rich)listbox to be derived from listitem-base

We do already have a few dependencies on listitem-base, e.g. concerning mouse (click) support. This would be just one more. I'm not sure you're supposed to place anything else than (rich)listitems inside a (rich)listbox anyway (except of course listheader, listcolumn, etc.). Not my call, though.

> probably not no but that could be added in the same way.

Please do so.

> I'm pretty confident that this isn't the case.

It indeed is. The critic is thus that the children property IMHO unintuitively no longer returns all children. Iterating over all children (even hidden ones) would now require a rewrite of the children property getter - while just filtering out invisible children would be quite easy:

this.children.filter(function(aItem) aItem.visible);

Of course your stance would be correct if hidden items no longer were listitems. However IMO the need for having to remember that fact at all makes your API suggestion sub-optimal at best.

Comment 16

[Dave Townsend [:mossop]]

(In reply to comment #15)
> > I'm pretty confident that this isn't the case.
> 
> It indeed is. The critic is thus that the children property IMHO unintuitively
> no longer returns all children. Iterating over all children (even hidden ones)
> would now require a rewrite of the children property getter - while just
> filtering out invisible children would be quite easy:

The children property is already filtered to not return all children. It currently only returns children that are nsIDOMXULSelectControlItemElement. Short of any actual spec I read that as it filtering the list to only selectable items in which I would not expect hidden things to be included.

Comment 17

[Simon Bünzli]

(In reply to comment #16)
> The children property is already filtered to not return all children. It
> currently only returns children that are nsIDOMXULSelectControlItemElement.
> Short of any actual spec I read that as it filtering the list to only
> selectable items in which I would not expect hidden things to be included.

From http://developer.mozilla.org/en/docs/XUL:Richlistbox:
> DOMNodeList children - returns all <richlistitem> child elements.

It's just easier to filter for nsIDOMXULSelectControlItemElement than for richlistitems - and the filter is mainly to keep templates out. My point still stands.

Anyway, I'd really like to hear Neil's opinion on this change (comment #9 onward).

Comment 18

[Neil Deakin]

The children must all implement the nsIDOMXULSelectControlItemElement interface and be included in the list of children whether they are hidden or not.

The UI could deal with this by skipping hidden items during keyboard events, although html selects don't currently do this either.

Comment 19

[Simon Bünzli]

Attachment: ignore invisible elements on manual selection only

This patch fixes the bug as originally filed by just making sure that a user can't manually select (through mouse or keyboard) a hidden or collapsed list item.

Neil: Do you want anything like this, nothing at all or a aVisibleOnly parameter on most methods so that developers who actually need this won't have to do too much adaptation on their own?

Comment 20

[Neil Deakin]

Comment on attachment 270955 [details] [diff] [review]
ignore invisible elements on manual selection only

       <method name="selectItemRange">
         <parameter name="aStartItem"/>
         <parameter name="aEndItem"/>
+        <parameter name="aManualSelection"/>

This method is defined by idl so it's arguments shouldn't really be changed. Not a huge deal as we have some other widgets which do this, but it would be good to avoid this if there's a clean way to.

Also, call it userSelection rather than manualSelection

>+      <method name="_canManuallySelect">

_canUserSelect

>         }
>         else if (event.shiftKey) {
>-          control.selectItemRange(null, this);
>+          control.selectItemRange(null, this, true);

There's another caller of selectItemRange as well.

Also, there are several callers of getNextItem/getPreviousItem that should be changed.

Comment 21

[Dave Townsend [:mossop]]

(In reply to comment #19)
> This patch fixes the bug as originally filed by just making sure that a user
> can't manually select (through mouse or keyboard) a hidden or collapsed list
> item.

Simon, if you are going to work on a bug that is already assigned to me I would appreciate you letting me know in advance so I don't waste any further time working on it.

From what I can see your patch will have a problem if you cursor up to a hidden row, in that event I believe your code will then select the next list item, i.e. the one you just came from.

It also appears to suffer from the same issue that I have with the patch I had already developed which is that page up and page down would no longer select an element that is visually a page away from the previous item.

As part of the work I did I made a mochitest for the listbox and richlistbox to test the behaviour, let me know if you want to use it, though you'd have to adapt it since the behaviour you have here is a little different to what I had implemented.

Comment 22

[Simon Bünzli]

(In reply to comment #21)
> Simon, if you are going to work on a bug that is already assigned to me I would
> appreciate you letting me know in advance so I don't waste any further time
> working on it.

Hm, that's actually a slight misunderstanding. I wasn't volunteering to do all the work myself, just suggesting a slightly different way and volunteering some code (and sure: asking Neil whether he actually preferred that direction). Please don't consider your time wasted (rather's mine) but take from my input as much as you want and go on yourself - should you still intend to fix this bug.

(In reply to comment #20)
> This method is defined by idl so it's arguments shouldn't really be changed.

I feared as much. One alternative I had considered was using a _userSelecting field similar to _suppressOnSelect and then pass the argument behind the methods' backs - would have made a slightly bigger mess out of the code, though.

> There's another caller of selectItemRange as well.

Where it doesn't matter, though (at least if you consider aUserSelection only an optional argument).

> Also, there are several callers of getNextItem/getPreviousItem that should be
> changed.

Same as above. BTW: these two seem to have moved into listbox's private section. Is that intended? If so, what about cleaning them up to _get*Item and dropping the mostly useless aDelta argument?

Comment 23

[Dave Townsend [:mossop]]

Attachment: patch rev 3

This adds a private method to listbox.xml to allow moving around by visible elements only and makes the keyboad handlers call it. It also modifies scrollByOnePage in richlistbox to return the number of visible items it moved by rather than all the items.

There are a bunch of testcases for various bits applied to a richlistbox and a listbox. The richlistbox passes all of them as this patch stands. The listbox fails the movement using page-up and page-down.

The issue with the listbox is more complex however, it suffers from problems with the scrollbar being able to move beyond the end of the list of items (by as many items as are hidden), not being able to scroll all the way back to the top and some repaint issues around the hidden items.

Fundamental problem is that the ListBoxBodyFrame hasn't got the concept of hidden items and I believe that there would have to be changes there to support hidden items in a listbox.

Neil, maybe you have some suggestions?

Comment 24

[Simon Bünzli]

This patch has the disadvantage of letting the user select invisible items quite easily: just select an item followed by an invisible one and Shift-click the next visible one after that. That'll require implementors to work-around, which AFAICT we'd prefer them not to have to.

Further nits:
* Couldn't you rather re-use moveByOffset by just calculating the correct offset in _moveByVisibleOffset before calling it? Or even just add a further argument (aUserSelection) to moveByOffset?

* If you modify scrollOnePage's meaning, please also adjust the (already incorrect) comment at the top of listbox.xml. Why not use moveByOffset for page-scrolling instead of modifying this, though (which should already treat hidden items correctly, as they simply don't add any height to the distance needed for measuring the visible view-port)?

Comment 25

[Dave Townsend [:mossop]]

(In reply to comment #24)
> This patch has the disadvantage of letting the user select invisible items
> quite easily: just select an item followed by an invisible one and Shift-click
> the next visible one after that. That'll require implementors to work-around,
> which AFAICT we'd prefer them not to have to.

Yes, but this is what Neil agreed to over IRC.

> Further nits:
> * Couldn't you rather re-use moveByOffset by just calculating the correct
> offset in _moveByVisibleOffset before calling it? Or even just add a further
> argument (aUserSelection) to moveByOffset?

There isn't a great deal to be gained from this, the last block from |if (newItem)| I guess.

> * If you modify scrollOnePage's meaning, please also adjust the (already
> incorrect) comment at the top of listbox.xml.

The comment doesnt specify what it returns but sure that can be added.

> Why not use moveByOffset for
> page-scrolling instead of modifying this, though (which should already treat
> hidden items correctly, as they simply don't add any height to the distance
> needed for measuring the visible view-port)?

Because if the hidden items are at the start/end of the list then moveByOffset will happily move into them leaving you with a focused hidden item. Changing the return is necessary since it is currently broken in regards to hidden items. listbox returns a fixed number of items in the view regardless of whether any items in the current view are hidden. richlistbox on the other hand returns the number of items to scroll by which grows as you add hidden items into the mix.

Comment 26

[Simon Bünzli]

(In reply to comment #25)
> Yes, but this is what Neil agreed to over IRC.

Well, then at least add a warning that users can still easily select hidden items in multi-select listboxes and shortly think about what you'd recommend extension authors to do in order to get what we currently can't... oh, and please file a follow-up bug so that we don't forget about it (bonus points for adding that bug number to the above comment - that might even get us a fix).

Out of curiosity, though: what don't you like about the approach of attachment 270955 [details] [diff] [review]? The two issues you mentioned could quite easily be fixed (by trying to get an item in moving direction first when the current is invisible in moveByOffset; and by adjusting listbox's scrollOnePage to behave like richlistbox's).

> There isn't a great deal to be gained from this

No, just some conceptual weight (i.e. less risk of forgetting to change both and less time figuring out where the actual differences are).

Comment 27

[Dave Townsend [:mossop]]

(In reply to comment #26)
> Out of curiosity, though: what don't you like about the approach of attachment
> 270955 [details]? The two issues you mentioned could quite easily be fixed (by trying to
> get an item in moving direction first when the current is invisible in
> moveByOffset; and by adjusting listbox's scrollOnePage to behave like
> richlistbox's).

Basically this patch was almost as it was before you posted yours and Neil had already suggested to me that the existing APIs shouldn't be changed.

Comment 28

[Simon Bünzli]

Attachment: ignore invisible elements on manual selection only, rev 2

(In reply to comment #27)
> Basically this patch was almost as it was before you posted yours and Neil had
> already suggested to me that the existing APIs shouldn't be changed.

That won't be necessary with the other patch, either. As I said: we can hide the need for an API change in a field (similar to _suppressOnSelect). Does a so patched listbox.xml now pass all your tests?

Comment 29

[Dave Townsend [:mossop]]

If you mean the unit tests from my patch then no.

In hiddenitems.xul it fails the following for both listbox and richlistbox:

Doesn't skip collapsed item.
Should have moved to last unhidden item.
Should have selected all but the last item (If you've changed the selection behaviour then that's to be expected).
Page down should go to the last visible item (went to first list item).

In hiddenpaging.xul it fails the following for richlistbox:

Page down should move to item14 (went to item13)
Then two failures on page up most likely caused by the previous failure.

Also there are 6 failures for the listbox there but I'll leave those since I couldn't say if they are issues with your patch or similar issues to those I found. It also exhibits the same repaint and scroll issues of course.

Simon, if you wish to take this bug and complete your patch to pass the tests and to Neil's satisfaction then please do. The bug I required this for no longer needs it and so this is no longer a priority for me so I'm not sure I will get chance to work out the issues with the listbox before gecko 1.9.

Comment 30

[Simon Bünzli]

Attachment: ignore invisible elements on manual selection only, rev 3

(In reply to comment #29)
> It also exhibits the same repaint and scroll issues of course.

I'm quite sure those are due to listScrollbox not being used to hidden items and not to this patch. Anyway, that's just a single listbox-test which fails.

> if you wish to take this bug and complete your patch to pass the tests
> and to Neil's satisfaction then please do.

Alright. Don't have much time, either, but it looks like we're almost there: apart from the single failure mentioned above, we now pass all tests and behave pretty much as I'd expect it. Neil: anything missing?

Comment 31

[Neil Deakin]

Comment on attachment 273204 [details] [diff] [review]
ignore invisible elements on manual selection only, rev 3

Index: toolkit/content/tests/widgets/test_hiddenitems.xul

+  listbox.selectedItem = listbox.getItemAtIndex(2);
+  ok(listbox.selectedItem == listbox.getItemAtIndex(2), id + ": Should have selected the hidden item");

Should probably use listbox.selectedIndex = 2, rather than setting selectedItem then immediately comparing it against what it was just set to.

+  if (id == "richlistbox")
+    is(listbox.getIndexOfFirstVisibleRow(), 3, id + ": Page up should move up a visible page");
+  else // blaming a listScrollbox bug instead of our own
+    todo_is(listbox.getIndexOfFirstVisibleRow(), 3, id + ": Page up should move up a visible page");

Seems that listbox just returns mCurrentIndex for getIndexOfFirstVisibleRow which sounds like a bug. Could one be filed?

+  sendKey("PAGE_UP", id);
+  is(listbox.selectedItem.id, id + "_item2", id + ": Page up should go to the item a visible page away");
+  is(listbox.getIndexOfFirstVisibleRow(), 0, id + ": Page up should not move beyond the start");
+  sendKey("PAGE_UP", id);
+  is(listbox.selectedItem.id, id + "_item1", id + ": Page up should go to the item a visible page away");
+  is(listbox.getIndexOfFirstVisibleRow(), 0, id + ": Page up should not move beyond the start");
+}

It would be nice if the each test in this function had a different description.

+     * @param aDirection - specifies scrolling direction, should be either -1 or 1
+     * @return the number of elements the selection should wander

"the selection scrolled"

+          var userSelecting = this._userSelecting;
+          this._userSelecting = false; // that's US automatically unselecting
           for (; currentItem; currentItem = this.getNextItem(currentItem, 1))
             this.removeItemFromSelection(currentItem);
-

Don't remove the blank line.
       <method name="scrollOnePage">
...
+              var maxTop = this.getRowCount() - this.getNumberOfVisibleRows();
+              for (i = this.getRowCount(); i >= 0 && i > maxTop; i--) {

for (i = this.getRowCount() - 1

+              var boxObject = children[index].boxObject;
+              if (boxObject.height == 0)
+                continue; // hidden children seem to have .y = 0

Yes, as hidden children aren't displayed at all, so just say 'hidden children have a y of 0'

It would also be good to extract the listbox tests from bug 368097 and run them, since we also need non-user input usage of the api tests as well, but that can come later.

Comment 32

[Simon Bünzli]

Attachment: nits fixed

(In reply to comment #31)
> Seems that listbox just returns mCurrentIndex for getIndexOfFirstVisibleRow
> which sounds like a bug. Could one be filed?

I'm not sure that's really the issue as paging up in a listbox with hidden items currently seems like a bad idea in all cases. Would you mind further investigating this before filing an appropriate bug?

> It would also be good to extract the listbox tests from bug 368097

Unfortunately, I didn't have time for this. Should any of the tests go orange, please CC me in the resulting follow-up bugs and I'll have a look at it.

Comment 33

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 274859 [details] [diff] [review]
nits fixed

This doesn't need approval at this stage.

Comment 34

[Dave Townsend [:mossop]]

Checking in toolkit/content/tests/widgets/Makefile.in;
/cvsroot/mozilla/toolkit/content/tests/widgets/Makefile.in,v  <--  Makefile.in
new revision: 1.19; previous revision: 1.18
done
RCS file: /cvsroot/mozilla/toolkit/content/tests/widgets/test_hiddenitems.xul,v
done
Checking in toolkit/content/tests/widgets/test_hiddenitems.xul;
/cvsroot/mozilla/toolkit/content/tests/widgets/test_hiddenitems.xul,v  <--  test_hiddenitems.xul
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/content/tests/widgets/test_hiddenpaging.xul,v
done
Checking in toolkit/content/tests/widgets/test_hiddenpaging.xul;
/cvsroot/mozilla/toolkit/content/tests/widgets/test_hiddenpaging.xul,v  <--  test_hiddenpaging.xul
initial revision: 1.1
done
Checking in toolkit/content/widgets/listbox.xml;
/cvsroot/mozilla/toolkit/content/widgets/listbox.xml,v  <--  listbox.xml
new revision: 1.27; previous revision: 1.26
done
Checking in toolkit/content/widgets/richlistbox.xml;
/cvsroot/mozilla/toolkit/content/widgets/richlistbox.xml,v  <--  richlistbox.xml
new revision: 1.41; previous revision: 1.40
done

Comment 35

[Dave Townsend [:mossop]]

Backed out due to test failures on Linux and Mac.

Linux (centos5-01):

*** 34773 INFO Running /tests/toolkit/content/tests/widgets/test_hiddenpaging.xul...
*** 34774 ERROR FAIL | richlistbox: Page down should go to the item one visible page away | got "richlistbox_item7", expected "richlistbox_item8"
*** 34775 ERROR FAIL | richlistbox: Page down should have scrolled down a visible page | got 6, expected 7
*** 34776 ERROR FAIL | richlistbox: Second page down should go to the item two visible pages away | got "richlistbox_item11", expected "richlistbox_item13"
*** 34777 INFO PASS | richlistbox: Second page down should not scroll beyond the end
*** 34778 INFO PASS | richlistbox: Third page down should go to the last visible item
*** 34779 INFO PASS | richlistbox: Third page down should not have scrolled at all
*** 34780 ERROR FAIL | richlistbox: Page up should go to the item one visible page away | got "richlistbox_item10", expected "richlistbox_item9"
*** 34781 ERROR FAIL | richlistbox: Page up should scroll up a visible page | got 5, expected 3
*** 34782 ERROR FAIL | richlistbox: Second page up should go to the item two visible pages away | got "richlistbox_item4", expected "richlistbox_item2"
*** 34783 INFO PASS | richlistbox: Second page up should not scroll beyond the start
*** 34784 INFO PASS | richlistbox: Third page up should return to the first visible item
*** 34785 INFO PASS | richlistbox: Third page up should not have scrolled at all
*** 34786 INFO PASS | listbox: Page down should go to the item one visible page away
*** 34787 INFO PASS | listbox: Page down should have scrolled down a visible page
*** 34788 INFO PASS | listbox: Second page down should go to the item two visible pages away
*** 34789 INFO PASS | listbox: Second page down should not scroll beyond the end
*** 34790 INFO PASS | listbox: Third page down should go to the last visible item
*** 34791 INFO PASS | listbox: Third page down should not have scrolled at all
*** 34792 INFO PASS | listbox: Page up should go to the item one visible page away
*** 34793 INFO TODO | listbox: Page up should scroll up a visible page | got 4, expected 3
*** 34794 INFO PASS | listbox: Second page up should go to the item two visible pages away
*** 34795 INFO PASS | listbox: Second page up should not scroll beyond the start
*** 34796 INFO PASS | listbox: Third page up should return to the first visible item
*** 34797 INFO PASS | listbox: Third page up should not have scrolled at all
*** 34799 INFO Running /tests/toolkit/content/tests/widgets/test_menubar.xul...

Mac (bm-xserve07):

*** 34773 INFO Running /tests/toolkit/content/tests/widgets/test_hiddenpaging.xul...
*** 34774 INFO PASS | richlistbox: Page down should go to the item one visible page away
*** 34775 INFO PASS | richlistbox: Page down should have scrolled down a visible page
*** 34776 INFO PASS | richlistbox: Second page down should go to the item two visible pages away
*** 34777 ERROR FAIL | richlistbox: Second page down should not scroll beyond the end | got 8, expected 9
*** 34778 INFO PASS | richlistbox: Third page down should go to the last visible item
*** 34779 ERROR FAIL | richlistbox: Third page down should not have scrolled at all | got 8, expected 9
*** 34780 INFO PASS | richlistbox: Page up should go to the item one visible page away
*** 34781 ERROR FAIL | richlistbox: Page up should scroll up a visible page | got 1, expected 3
*** 34782 INFO PASS | richlistbox: Second page up should go to the item two visible pages away
*** 34783 INFO PASS | richlistbox: Second page up should not scroll beyond the start
*** 34784 INFO PASS | richlistbox: Third page up should return to the first visible item
*** 34785 INFO PASS | richlistbox: Third page up should not have scrolled at all
*** 34786 INFO PASS | listbox: Page down should go to the item one visible page away
*** 34787 INFO PASS | listbox: Page down should have scrolled down a visible page
*** 34788 INFO PASS | listbox: Second page down should go to the item two visible pages away
*** 34789 INFO PASS | listbox: Second page down should not scroll beyond the end
*** 34790 INFO PASS | listbox: Third page down should go to the last visible item
*** 34791 INFO PASS | listbox: Third page down should not have scrolled at all
*** 34792 INFO PASS | listbox: Page up should go to the item one visible page away
*** 34793 INFO TODO | listbox: Page up should scroll up a visible page | got 4, expected 3
*** 34794 INFO PASS | listbox: Second page up should go to the item two visible pages away
*** 34795 INFO PASS | listbox: Second page up should not scroll beyond the start
*** 34796 INFO PASS | listbox: Third page up should return to the first visible item
*** 34797 INFO PASS | listbox: Third page up should not have scrolled at all

Comment 36

[Dave Townsend [:mossop]]

Mac qm-xserve01 gave the same failures as bm-xserve07

Comment 37

[Simon Bünzli]

My guess is that the following values aren't cross-platform (failing to take into account (native) theme specific padding, borders and margins):

+    /* This makes the richlistbox 5 rows high */
+    richlistitem {
+      height: 30px;
+    }
+    #richlistbox {
+      height: 154px;
+    }

Since I've got no access whatsoever to OS X or Linux boxen, I unfortunately can't help with fiddling this issue out.

Neil: How do you suggest to proceed from here?

Comment 38

[Dave Townsend [:mossop]]

(In reply to comment #37)
> My guess is that the following values aren't cross-platform (failing to take
> into account (native) theme specific padding, borders and margins):

That was my first thought as well, so I tested on my Mac box. The first two failures appear consistent with that however that last is not. The index that appeared at the top for me was item4 (index 3 as expected). While it's possible there was a smidgen of index 2 visible that I couldn't determine there shouldn't have been any of index 1 which is what the test failed with.

Comment 39

[Dave Townsend [:mossop]]

A thought I had late last night is that if this is just a theming issue then it's because we're trying to force the list to have an exact number of items when it's going to end up being slightly less or more than that depending on platform. I think if we aim at just less than an exact number of items then even if it varies by a few pixels it shouldn't be enough to push it up another item in length. Could be worth rejigging the numbers based on that and seeing how it turns out.

Comment 40

[Simon Bünzli]

Attachment: make richlistbox text mostly theme independent

As you guessed in comment #38, there was indeed a bug WRT richlistbox's pageup implementation (it might return an invisible item which would lead to moving one visible item too far). This patch fixes that bug and works around the theme issue by cropping the richlistbox by half a row as suggested by comment #39 (that unfortunately requires to fork the tests for listbox and richlistbox).

Dave: Could you please test this patch on Mac again (and possibly Linux as well)? Thanks.

Comment 41

[Dave Townsend [:mossop]]

Passes on Mac, can't test on linux right now.

Comment 42

[Simon Bünzli]

Sweet. Now, AFAICT the Linux test results from comment #35 are really the same I originally got when making the richlistbox smaller without adapting the tests - they should thus be fixed as well. Would you mind checking the revised patch in and let the unit tests run again?

Comment 43

[Dave Townsend [:mossop]]

Ok here goes again:

Checking in toolkit/content/tests/widgets/Makefile.in;
/cvsroot/mozilla/toolkit/content/tests/widgets/Makefile.in,v  <--  Makefile.in
new revision: 1.21; previous revision: 1.20
done
Checking in toolkit/content/tests/widgets/test_hiddenitems.xul;
/cvsroot/mozilla/toolkit/content/tests/widgets/test_hiddenitems.xul,v  <--  test_hiddenitems.xul
new revision: 1.3; previous revision: 1.2
done
Checking in toolkit/content/tests/widgets/test_hiddenpaging.xul;
/cvsroot/mozilla/toolkit/content/tests/widgets/test_hiddenpaging.xul,v  <--  test_hiddenpaging.xul
new revision: 1.3; previous revision: 1.2
done
Checking in toolkit/content/widgets/listbox.xml;
/cvsroot/mozilla/toolkit/content/widgets/listbox.xml,v  <--  listbox.xml
new revision: 1.29; previous revision: 1.28
done
Checking in toolkit/content/widgets/richlistbox.xml;
/cvsroot/mozilla/toolkit/content/widgets/richlistbox.xml,v  <--  richlistbox.xml
new revision: 1.43; previous revision: 1.42
done

Comment 44

[Dave Townsend [:mossop]]

Looking good. Tests passing on all platforms. One burning box but I'm pretty sure that's unrelated.