490178 - Need a way to set the direction on a richlistbox

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect)

Description

[Nochum Sossonko [:Natch]]

Attachment: patch

In order to specify a direction on the richlistbox the scrollbox needs to inherit the order and pack.

Comment 1

[Dão Gottwald [:dao]]

dir="reverse" seems to break the scrollOnePage method, and possibly others.

Comment 2

[Nochum Sossonko [:Natch]]

(In reply to comment #1)
> dir="reverse" seems to break the scrollOnePage method, and possibly others.

Good point, I'll have to update the methods.

Comment 3

[Nochum Sossonko [:Natch]]

Listbox doesn't seem to support dir="reverse" at all, gonna have to make a localized patch just for richlistbox :(

Comment 4

[Nochum Sossonko [:Natch]]

So I'm thinking of just adding a method "setDirection" to richlistbox.xml and fixing up all of the functions to work with it. The method will just set this._scrollbox.dir=aDirection. How does that sound?

Comment 5

[Nochum Sossonko [:Natch]]

Attachment: patch + tests

So, this isn't the most optimal solution, but it is a temporary solution of sorts. IMHO dir and pack should apply to the non-anon descendants, but that would be a Core bug, and the richlistbox impl. would need to be fixed anyhow.

Let me know what you think of this approach.

Comment 6

[Dão Gottwald [:dao]]

Comment on attachment 377915 [details] [diff] [review]
patch + tests

Just use "dir" rather than inventing "direction". This way the scrollbox can inherit the direction from the richlistbox.

Why does dir=reverse imply pack=end?

> this.dir == "reverse" ? aStartItem.nextSibling : aStartItem.previousSibling
would probably be faster than
> this.getItemAtIndex(this.getIndexOfItem(aStartItem) - 1)

Comment 7

[Nochum Sossonko [:Natch]]

Attachment: patch ver.2 + tests

Yes on all accounts. This makes a lot more sense, I was going through a bit of tunnel vision :(

Comment 8

[Dão Gottwald [:dao]]

Could you please include more context? See the last paragraph of this section: https://developer.mozilla.org/en/Mercurial_FAQ#How_can_I_diff_and_patch_files.3F

Comment 9

[Nochum Sossonko [:Natch]]

Attachment: patch ver. 2.1 (10 lines context)

I also changed the check so that it is cached instead of rechecking this.dir on every iteration. One thing I worry is in the case of a listbox (which does not really support dir yet), if someone sets dir the "getNextItem" and "getPreviousItem" will respect the dir setting. Should I redefine those functions in richlistbox with a comment to remove them once listbox is fixed up, or not worry about this at all?

Comment 10

[Nochum Sossonko [:Natch]]

Spun-off the listbox issue (not supporting dir) as bug 493461.

Comment 11

[Nochum Sossonko [:Natch]]

Attachment: patch ver. 2.1 (10 lines context)

Same as before minus the line endings badness, sorry for the spam.

Comment 12

[Dão Gottwald [:dao]]

Comment on attachment 377948 [details] [diff] [review]
patch ver. 2.1 (10 lines context)

>+          var isReverse = this.dir == "reverse";
>           while (aStartItem) {
>-            aStartItem = aStartItem.nextSibling;
>+            aStartItem = isReverse ? aStartItem.previousSibling :
>+                                     aStartItem.nextSibling;
>             if (aStartItem && aStartItem instanceof

You could be more aggressive and do
> var propNext = this.dir == "reverse" ? "previousSibling" : "nextSibling";
and
> aStartItem = aStartItem[propNext];

I don't really care about that, though.

>+                else { // XXX hack around a bug in ensureElementIsVisible

Which bug?

Comment 13

[Dão Gottwald [:dao]]

(In reply to comment #9)
> One thing I worry is in the case of a listbox (which does not
> really support dir yet), if someone sets dir the "getNextItem" and
> "getPreviousItem" will respect the dir setting. Should I redefine those
> functions in richlistbox with a comment to remove them once listbox is fixed
> up, or not worry about this at all?

listboxes aren't part of richlistboxes, so why do you touch the former at all?

Comment 14

[Nochum Sossonko [:Natch]]

(In reply to comment #12)
> >+                else { // XXX hack around a bug in ensureElementIsVisible
> 
> Which bug?

That's not my comment, the comment was there beforehand, being that it is so
uninformative I'm not really sure. In my experience, though, there is a small
jump under certain circumstances without this. I'm not 100% sure, I'll try to
dig through bonsai and see what I can come up with. Regardless, this change has
to be there (in fact anywhere that [first|last]Child or [previous|next]Sibling
is used has to be changed).

I thought I had posted this before, guess I didn't, the boxObject navigation
(its' [next|previous]Sibling etc.) should work for me here, although in my
experience it hasn't.

(In reply to comment #13)
> listboxes aren't part of richlistboxes, so why do you touch the former at all?

richlistboxes inherit from listbox-base as do listboxes and they both use the generic get[Previous|Next]Item function from listbox-base, do you want me to override the function instead of modifying listbox-base generic one (alternatively I can make a "dir" property that returns "normal" on listboxes and is unsettable, that seems a bit hack-ish though)?

Comment 15

[Nochum Sossonko [:Natch]]

(In reply to comment #12)
> >+                else { // XXX hack around a bug in ensureElementIsVisible
> 
> Which bug?

Bug 298371 comment 27. From Simon, cc'ing him. Since I'm there anyhow, I'll update the comment, Simon, can you give me a short description of the problem. Otherwise I'll just take your comment from that bug, it seems a bit vague though.

Comment 16

[Simon Bünzli]

(In reply to comment #15)
This was an issue I had with the Firefox 1.5 Extension Manager, and bug 298371 comment 27 is actually a pretty accurate description.

If you try to reproduce, you might want to make sure that all richlistitems are the same height, and - of course - that you've got more than enough items to fill your list and that one of the first items is actually selected.

Comment 17

[Nochum Sossonko [:Natch]]

Thanks Simon, I see now what it does, I'll file a bug and reference that in the comment.

Dao: do you have a preference for comment 14 at the end:

> richlistboxes inherit from listbox-base as do listboxes and they both use the
> generic get[Previous|Next]Item function from listbox-base, do you want me to
> override the function instead of modifying listbox-base generic one
> (alternatively I can make a "dir" property that returns "normal" on listboxes
> and is unsettable, that seems a bit hack-ish though)?

Comment 18

[Dão Gottwald [:dao]]

Overriding seems like the right solution.

Comment 19

[Nochum Sossonko [:Natch]]

Attachment: patch ver. 2.3 nits addressed

Comment 20

[Neil Deakin]

What reason do we have for needing this?

Comment 21

[Nochum Sossonko [:Natch]]

(In reply to comment #20)
> What reason do we have for needing this?

1) So that the attribute actually works.
2) For the blocked bug. The error console wants to be switched to a richlistbox and needs this so that it can easily switch directions.

Comment 22

[Nochum Sossonko [:Natch]]

Neil: do the answers in comment 21 suffice? This is sort of in the way of other bugs, if you could review this I'd appreciate it.

Comment 23

[Neil Deakin]

Comment on attachment 378247 [details] [diff] [review]
patch ver. 2.3 nits addressed

This seems to imply that when dir="reverse", richlistbox.getItemAtIndex(0) would return the last child. That's not correct, as richlistbox.getItemAtIndex(0) should always return the first child. The dir attribute should only affect where it is displayed and have no effect on the order used for retrieving, selecting or iterating items.

Sorry for the delay. I was trying to determine whether this would conflict with attempts to add multicoloumn support to richlistbox.

Comment 24

[Nochum Sossonko [:Natch]]

Ok. So, I have to override nearly every function on listbox-base to implement this. Not sure how useful it is for scrollbox to inherit from listbox-base if so. Any other suggestions?

Maybe I can implement this in listbox-base but include a check for supportsDir, then set it to false on listbox-base and true on richlistbox...

Although, there would have to be new functions for get[Next|Previous]Item to retrieve the displayed next/previous item, then add a check where they're called in response to user interactions...

Comment 25

[Nochum Sossonko [:Natch]]

Attachment: patch v.3

This is what seems to be the alternative. Only reverse direction if a user initiated the selection. Fixed the test as well to correspond to this change.

Comment 26

[Neil Deakin]

Comment on attachment 384569 [details] [diff] [review]
patch v.3

I haven't tested this yet, but wouldn't it be easier to just override _moveByOffsetFromUserEvent?

_shouldReverse should be called _mayReverse

Why do you need to reverse during the click handler?

Comment 27

[Nochum Sossonko [:Natch]]

(In reply to comment #26)
> (From update of attachment 384569 [details] [diff] [review])
> I haven't tested this yet, but wouldn't it be easier to just override
> _moveByOffsetFromUserEvent?

I wanted to do that at first, but I got stuck with the VK_PAGE_{UP|DOWN} and VK_{END|HOME} handlers that call scrollOnePage or get this.currentIndex before _moveByOffsetFromUserEvent is called.

> _shouldReverse should be called _mayReverse

Ok, I'll fix that.

> Why do you need to reverse during the click handler?

Because when used in conjunction with the {meta|shift|ctrl} key it messes with the selection, and calls the methods on the [rich]listbox that need the flag. E.g. http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/listbox.xml?mark=1011-1018#1000 .

Comment 28

[Neil Deakin]

> Because when used in conjunction with the {meta|shift|ctrl} key it messes with
> the selection, and calls the methods on the [rich]listbox that need the flag.
> E.g.
> http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/listbox.xml?mark=1011-1018#1000

I didn't see anything negative if I remove it though. What problem does it cause?

Comment 29

[Nochum Sossonko [:Natch]]

Attachment: patch ver. 3.1

Ok, I just re-tested the click handler and it seems that there are no differences. I put it in originally because I just wanted to make sure the richlist gets the correct items _any_time_ it's reversed. I guess this doesn't matter.

I also changed the variable name to _mayReverse.

Comment 30

[Neil Deakin]

Comment on attachment 389545 [details] [diff] [review]
patch ver. 3.1

>+  sendKey("END", richListBox);
>+  is(richListBox.currentIndex, 0, "Selection should move to the last item");
>+  sendKey("HOME", richListBox);
>+  is(richListBox.currentIndex, count - 1, "Selection should move to the first item");

You might clarify that you mean the last/first item in display order.

>+  var currentIndex = richListBox.currentIndex;
>+  richListBox._mayReverse = true; // hack to get a proper index

No code outside of listbox.xml should be adjusting its 'private' fields. What is this trying to accomplish?

Did you mean to remove the code in the click handler?

Comment 31

[Nochum Sossonko [:Natch]]

(In reply to comment #30)
> No code outside of listbox.xml should be adjusting its 'private' fields. What
> is this trying to accomplish?

This is a test for the richlistbox, so I figured it wouldn't be as bad. The problem is I need the correct index when I call scrollOnePage match, this was the only way I could really do it.

> Did you mean to remove the code in the click handler?

No. Not sure how that even got there, I'll add a test for click handling as well...

Comment 32

[Nochum Sossonko [:Natch]]

Attachment: patch

Fixed the click handler mistake and added tests for clicking and keying with the shift key held.

Comment 33

[Nochum Sossonko [:Natch]]

Neil: any chance of getting this reviewed soon?

thanks.

Comment 34

[Neil Deakin]

(In reply to comment #31)
> (In reply to comment #30)
> > No code outside of listbox.xml should be adjusting its 'private' fields. What
> > is this trying to accomplish?
> 
> This is a test for the richlistbox, so I figured it wouldn't be as bad. The
> problem is I need the correct index when I call scrollOnePage match, this was
> the only way I could really do it.

I don't understand this comment. What value are you trying to retrieve? If the richlistbox and items have a known height, or are given one within the test, the various scroll positions and sizes should already be known.

Comment 35

[Nochum Sossonko [:Natch]]

(In reply to comment #34)
> I don't understand this comment. What value are you trying to retrieve? If the
> richlistbox and items have a known height, or are given one within the test,
> the various scroll positions and sizes should already be known.

While that is true, and I *could* figure out the scroll position by hand then calculate the index of the element from that position, the calculation in scrollOnePage isn't a very simple one, plus it does some scrolling, if necessary, on its own. I could just copy the calculation from that function, but then what happens when that function changes? I figured it was a more error-prone approach vs. just setting _mayReverse. If you disagree, I can certainly change it.

For currentIndex, though, I'm not sure how I can get the "correct" currentIndex without setting _mayReverse, which I need in order to check the results.

Comment 36

[Neil Deakin]

(In reply to comment #35)
> While that is true, and I *could* figure out the scroll position by hand then
> calculate the index of the element from that position, the calculation in
> scrollOnePage isn't a very simple one, plus it does some scrolling, if
> necessary, on its own. I could just copy the calculation from that function,
> but then what happens when that function changes? I figured it was a more
> error-prone approach vs. just setting _mayReverse. If you disagree, I can
> certainly change it.

I don't understand. If the richlistbox has a height of 80 exactly pixels and each item is given a height of exactly 20 pixels, one page is 4 items, and no calculation is needed.

> For currentIndex, though, I'm not sure how I can get the "correct" currentIndex
> without setting _mayReverse, which I need in order to check the results.

You could make sure that the item at that index has the right coordinates.

Comment 37

[Nochum Sossonko [:Natch]]

Attachment: patch ver. 3.2

Ok, how about this? No more _mayReverse changes in the test, the test now ensures that there are 4, and only 4, richlistitems per page, then it checks the values backwards like the rest of the dir="reverse" tests.

Comment 38

[Neil Deakin]

Comment on attachment 393863 [details] [diff] [review]
patch ver. 3.2

>+  var height = richListBox.scrollBoxObject.height;
>+  do {
>+    richListBox.appendItem("Test", "");
>+    richListBox.lastChild.height = richListBox.lastChild.maxHeight = Math.floor(height / 4);
>+  } while (richListBox.children[richListBox.children.length - 1].getBoundingClientRect().bottom < (height * 2))

Use richListBox.lastChild in the while loop as well. Or, appendItem will return this item so just use that in all three cases.

>+  var count = richListBox.children.length;

Use richListBox.itemCount

+  is(richListBox.currentIndex, richListBox.children.length - 1, "Selection should move to the last item");

And here

Comment 39

[Nochum Sossonko [:Natch]]

Attachment: patch ver. 3.2 + comments

Thanks Neil.

Comment 40

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/8f77197a8f99

Comment 41

[[e-mail]]

So, a) what's the difference between _mayReverse and _userSelecting, and b) why do you set _mayReverse in the callers to _moveByOffsetFromUserEvent when (unless the event has already been cancelled!) the latter already sets and unsets it?

Comment 42

[Nochum Sossonko [:Natch]]

(In reply to comment #41)
> So, a) what's the difference between _mayReverse and _userSelecting,

_userSelecting is unset in selectItemrange, which then proceeds to use getNextItem which depends on _mayReverse.

> and b) why
> do you set _mayReverse in the callers to _moveByOffsetFromUserEvent when
> (unless the event has already been cancelled!) the latter already sets and
> unsets it?

Needed to get the correct item when calling scrollOnePage and .currentItem in _moveByOffsetFromUserEvent's arguments.

Comment 43

[Dão Gottwald [:dao]]

> 87903 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/content/tests/widgets/test_hiddenitems.xul | richlistbox: Up didn't skip collapsed item - got "richlistbox_item6", expected "richlistbox_item4"
> 87904 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/content/tests/widgets/test_hiddenitems.xul | richlistbox: Up didn't skip hidden item - got "richlistbox_item6", expected "richlistbox_item2"
> 87908 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/content/tests/widgets/test_hiddenitems.xul | richlistbox: Should have moved to the last unhidden item - got "richlistbox_item1", expected "richlistbox_item6"
> 89579 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/content/tests/widgets/test_richlist_direction.xul | Selection should move to one page down - got 0, expected -3
> 89581 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/content/tests/widgets/test_richlist_direction.xul | Selection should move to one page up - got 1, expected 4

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1250287939.1250289486.7380.gz

backed out

Comment 44

[Nochum Sossonko [:Natch]]

Attachment: patch ver. 3.3

The hiddenitems test failures were due to a typo in the getPreviousItem function in richlistbox.xml, it was essentially returning the same thing as getNextItem.

The failures in richlist_direction were due to the fact that a minheight wasn't being specified, and when the whole suite is run (as opposed to when just running individual tests), the richlistbox wasn't getting any height.

I'm re-requesting review to address an issue Neil brought up, and I've added a test for this. If someone attaches an event listener to the richlistbox and calls preventDefault, _moveByOffsetFromUserEvent returns without doing anything (correctly). However, _mayReverse would still be true since it was never being reset.

Comment 45

[Neil Deakin]

Comment on attachment 394752 [details] [diff] [review]
patch ver. 3.3

>+  richListBox.addEventListener("keypress", function(aEvent) {
>+    if (aEvent.keyCode != KeyEvent.DOM_VK_HOME)
>+      return;

No need to check this.

Comment 46

[Nochum Sossonko [:Natch]]

Attachment: patch ver. 3.3, nit addressed [backout: Comment 50]

Comment 47

[Nochum Sossonko [:Natch]]

I verified that all tests in toolkit/content/tests/widgets pass (besides bug 482430 and other random failures completely unrelated to this change).

Comment 48

[Nochum Sossonko [:Natch]]

Err, bug 510811...

Comment 49

[Serge Gautherie (:sgautherie)]

Comment on attachment 394831 [details] [diff] [review]
patch ver. 3.3, nit addressed
[backout: Comment 50]


http://hg.mozilla.org/mozilla-central/rev/4d1a92babc49

Comment 50

[Johnathan Nightingale [:johnath]]

Backed this out because it was causing oranges on Linux (e.g. http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1250620586.1250623015.29097.gz )

http://hg.mozilla.org/mozilla-central/rev/8bb50c6982c4

I would have liked Natch or sgautherie to have been around to answer questions about it, after landing it, but neither of you guys were in #developers an hour after the patch landed. I trust there was a good reason, and that you had asked some delegate to watch the tree on your behalf?

Did these failures not show up on TryServer?

Comment 51

[Johnathan Nightingale [:johnath]]

Sorry Natch - dao points out that this bug was checkin-needed, so you wouldn't have known when it would hit the tree, in order to watch for it. Still, I'd like to understand what went wrong here, because things should never land without someone around to watch them - and tests shouldn't land without a clean tryserver bill of health.

Being backed out by the sheriff is a last-ditch failure case that we should strive quite aggressively to avoid.

Comment 52

[Nochum Sossonko [:Natch]]

Johnathan: sorry about that, I see your point. The problem is, I tested this rigorously on my machine locally which runs a Windows Vista os, and I don't have access to any other oses atm. TryServer would be nice, it's just hard to find volunteers for that.

This change and test shouldn't be platform specific, so I suspect some other latent bug on Linux being the issue here.

Enn: I think the only option here is to revert to the _mayReverse flipping so that scrollOnePage accurately returns the value that will be scrolled on all platforms. Is that ok with you?

Comment 53

[Serge Gautherie (:sgautherie)]

(In reply to comment #50)
> Backed this out

Thanks Johnathan.

> I would have liked Natch or sgautherie to have been around to answer questions
> about it, after landing it, but neither of you guys were in #developers an hour
> after the patch landed. I trust there was a good reason, and that you had asked
> some delegate to watch the tree on your behalf?

Short answer: I was eating for a while :-|


(In reply to comment #52)
> TryServer would be nice, it's just hard to find volunteers for that.

This is no excuse as this was already landed and backed out once for tests failure: meaning this is the second time it was asked to be landed without proper testing :-(

Comment 54

[Nochum Sossonko [:Natch]]

Comment on attachment 394831 [details] [diff] [review]
patch ver. 3.3, nit addressed
[backout: Comment 50]

I have an idea on how to fix this issue, gonna push to try (it works on my system, but I'd like to check Linux as well).

Comment 55

[Nochum Sossonko [:Natch]]

Attachment: patch ver. 3.4

Ok, so the problem was that although we specified a size of 80 for the richlistbox the scrollBoxObject was only 78 (or whatever on different platforms). I updated the code to check that scrollBoxObject is 80, and if not to add the necessary amount to the richlistbox height so that it's right.

Tryserver passed as well.

Comment 56

[Nochum Sossonko [:Natch]]

Attachment: patch ver. 3.4 [Checkin: Comment 58]

Ugh, qrefresh fail.

Comment 57

[Nochum Sossonko [:Natch]]

Hope I'm not on the checkin-needed blacklist now...

Thanks Enn.

Comment 58

[Serge Gautherie (:sgautherie)]

Comment on attachment 395249 [details] [diff] [review]
patch ver. 3.4
[Checkin: Comment 58]


http://hg.mozilla.org/mozilla-central/rev/f8f1a9113a1c