74553 - Shift+Page Down/Up not working in a special case

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Ninoschka Baca]

This bug was found as a result of bug# 64387.

Build 2001-03-19-12: NT4
Build 2001-03-19-11: Linux RH 6.2
Build 2001-03-19-10: Mac 9.04
The basics of "Shift+Page Down" and "Shift+Page Up" are working but not in the 
following case: 

Steps to reproduce :
1. Go to a folder with 10 messages (making it easier to view the problem)
2. Select the first message
3. Select Shift+Page Down, all the messages are highlighted
4. Select the last message and only the last message is highlighted
5. Select Shift+Page Up

Results: The first message has a dotted line surrounding it, instead of all the
messages being highlighted. Select "Shift+Page Down" and all the messages are
highlighted except for the first message.

Expected Results: I would expect the first and all messages to be highlighted.

Comment 1

[Sander]

This bug isn't MailNews specific; all trees show it.

Comment 2

[Sander]

Attachment: patch

The problem also shows with just regular shift-up/down.
A related case is:
start with msg 0 selected
shift-click msg 2 to select msg 0, 1 and 2
shift-down to select msg 0, 1, 2 and 3 (expected) - instead selection shifts to
msg 2 and 3 (actual)

The patch is pretty self-evident once you know the problem (changes in the
selection by clicking didn't update the variables used when selecting using the
keyboard), though it took me an inordinate amount of time to track down.

Unfortunately testing further, I came across a few more testcases, such as:
start with msg 0 selected
shift-click msg 1 to select msg 0 and 1
shift-down to select msg 0, 1 and 2
shift-click msg 1 to select msg 0 and 1 (expected) - instead selection shifts
to msg 1 and 2 (actual)

This patch should fix all that (changing obviously faulty behaviour into a mix
of the 'best' of the behaviour of windows explorer and nautilus, as far as
applicable), as well as extending keyboard access for selecting with the
ctrl-shift case (which selects without invalidating the previous selection if
the current message isn't part of that previous selection).

Comment 3

[Sander]

Attachment: patch after tree widget refactoring

patch for after bug 221619 has landed - I _haven't_ been able to test this, as
the  patches for said bug didn't apply cleanly to my tree (as I'm hardly ever
able to be online, I feared so), but the changes between this and the previous
patch are trivial, so I _shouldn't_ have messed up anything. :)

Keeping the review? request on the previous patch while bug 221619 hasn't
landed yet.

Comment 4

[Sander]

2nd patched tested; will work correctly after bug 221619 has landed. 
(Jan: can I assume you will have time to review said patch after that landing?
Only just getting started with seriously contributing patches, I find I have
some trouble scaring up reviews [hardly ever being around on account of the
traveling doesn't help much either I guess]) :)

Comment 5

[Sander]

Comment on attachment 143160 [details] [diff] [review]
patch after tree widget refactoring

Toggling review request to updated patch since bug 221619 has landed.

Comment 6

[Jan Varga [:janv]]

What about merging this patch with Neil's big refactoring in bug 97434?

Comment 7

[nrlz]

> The patch is pretty self-evident once you know the problem (changes in the
> selection by clicking didn't update the variables used when selecting using the
> keyboard), though it took me an inordinate amount of time to track down.

I've observed a similar situation (tested FF 0.9.2). If you shift+Up/Dn on a
list box and then decide to use the mouse to finish the selections (with shift
key still depressed), the list values change.

I first observed this with the cookies list when I tried to select a few cookies
with shift+Dn. Then I decided to use the mouse to take over so I kept the shift
key down and started clicking. The previous selections were gone.

Comment 8

[[e-mail]]

Attachment 149207 [details] [diff] (bug 97434) fixed this on trunk, right?

Comment 9

[Sander]

*after testing* (Sorry for being so slow; busy life.) :)
yes, it does.