159477 - Arrow key selection in tree has absurd delay

Status: VERIFIED FIXED

(Core :: XUL, defect)

Description

[Blake Ross]

Patch coming up.

Comment 1

[Blake Ross]

Attachment: patch

Comment 2

[David Hyatt]

Comment on attachment 92833 [details] [diff] [review]
patch

sr=hyatt

This has to have been a typo.  When I did this in C++ originally, it was 50ms. 
Sigh.  Good catch, blake.

Comment 3

[David Hyatt]

Let's get this into the 1.1 branch.  This is a must-have.  Unbelievable. :)

Comment 4

[Brendan Eich [:brendan]]

Let's fix this for 1.1 and for the 1.0 branch.  It's safe and easy.

/be

Comment 5

[Brendan Eich [:brendan]]

Nominating and stuff.

/be

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 92833 [details] [diff] [review]
patch

r=[e-mail] if you use a single const SELECT_DELAY = 50; in a scope
that works for all uses.

/be

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 92833 [details] [diff] [review]
patch

Or a <field>, blake said (no const fields in xbl).

/be

Comment 8

[Blake Ross]

Attachment: patch

Comment 9

[Brendan Eich [:brendan]]

Comment on attachment 92835 [details] [diff] [review]
patch

Beauty -- we love you, blake! Carrying stamps over from last patch.

/be

Comment 10

[Brendan Eich [:brendan]]

*** Bug 159563 has been marked as a duplicate of this bug. ***

Comment 11

[(not reading, please use [e-mail] instead)]

very cool.  thanks blake!

one idea a dup of this has:

should we default to 50ms, but allow a given tree to say:

<tree selectTimeout="100" ...>

I guess if 50ms works for everyone, no need to add that until someone needs it.

Comment 12

[Sean Su]

I've done some tests in the mailnews thread pane, and have noticed that 50ms
might be a little too short.  Here's the problem that I'm seeing:

In the mailnews threadpane, if you hold down the arrow key to scroll thru the
summaries or messages, about 1/3 of the time it will load all of the (or every
other) messages it hilights.  While the other 2/3 of the time, it just scrolls
thru without loading any of the messages in the message pane.

This could be a problem because if a message takes time to load (contains lots
of tables or there are network problems for the html messages),  the user will
get stuck in this message as they're trying to scroll down the list.  This
actually hangs the entire browser until it finishes loading the message.

I'm on Win2k 1Ghz system with 512MB ram.  I think the faster machines might run
into this problem more often.  However, 75ms seems to be better with my system.

Can someone who has a desktop machine that's faster than what I have test both
50ms and 75ms?

Comment 13

[Blake Ross]

I have a 2ghz machine with 512 MB of ram and it works perfectly for me.

Comment 14

[Sean Su]

sorry, it's actually for slower machines that we'll have that problem.  Turns
out that the key repeat rate can be changed to be very slow, thus still
exhibiting the problem under mailnews.  One way to fix this is to detect when
the key is repeating and not load the message in the message pane, but Seth said
that he doesn't think there's a way to check for that.

I talked to putterman, and we agreed that this patch is okay to land with 50ms.
 We might have to set it differently for mailnews.

Blake, could you make it so that there's way to change this value?

Comment 15

[Brendan Eich [:brendan]]

I was arguing the other day with hyatt about how time is not relevant here, and
that the question of whether to load the message being "selected" as an arrow
key is pressed should depend on whether the arrow key is released -- on a key-up
event, or something akin.  Hyatt pointed out that other apps' tree widgets use
time as XUL's does, to handle the edge case where you reach the first message
when holding up, or the last when holding down.  In either of those cases, we
want to show the message even though the key is still held (even though there is
no key-up event).

But couldn't those edge cases be handled with logic, not with a timer?

The other reason to use a timer is to simplify the model for tree-based code, so
it has onselect only and not onselect and onselectrepeat or whatever.  XUL is
frozen as of mozilla1.0, so we can't elaborate the model now, except in
backward-compatible ways.

Anyway, 500ms is way too long.  ssu, if 50 ms is too short, what value would you
use?  Why is mailnews' use of tree any different from other XUL apps'? Should
the value be a function of CPU clock?  I hope not.

/be

Comment 16

[Sean Su]

There is a problem with loading certain messages in the message pane that make
it "appears" the browser is hung (ie. messages with large tables) until the
message has been fully loaded (or parsed).  This happens regardless if how the
message is triggered to be loaded (via mouse clicks or arrow keys).

The situation in which we were trying to avoid this "partial hang" is when the
key is in the repeat mode (either the arrow up or arrow down key).  Having the
timer be 500ms provided enough time for the selection (hilight) to move to the
next summary in the thread pane without loading messages in the message pane.

Changing the timer value from 500ms to 50ms causes messages to load as the
selection is scrolling thru the summaries.  It looks like on fast machines, the
scrolling is quick enough that having 50ms for the timer value is still slow
enough that it won't cause messages to load while scrolling.

You are correct in that this problem should be solved with logic instead of with
the timer.  It was initially thought that a timer value could be found that
would not cause the loading of each message hilighted while holding the up/down
arrow key (there by possibly running into a message that caused the temporary
browser hang).  It might be that a timer value of 500ms could be the only way
(as it currently is set to) to avoid this problem, but obivously that is not
acceptable for either mailnews or any other xul app.

Basically, we want the up/down arrow keys to load messages in the message pane
as quickly as possible (50ms is perfect), but not run into the temporary hang
inadvertantly.

Putterman was arguing that the chances of running into a message that causes a
temporary hang (however small) was not good UE (Scott please correct me if I
misunderstood you).  I'm now trying to find a way to avoid running into this
problem while holding the up/down arrow key to scroll thru the thread pane. 
There might not be a simple solution.

Brendan, let me know if I still have not answered your questions or this doesn't
make any sense.

Comment 17

[Blake Ross]

Why don't we check this in and get it tested on a wide variety of machines
before speculating?  Just waiting on approval.

Comment 18

[Sean Su]

I've already test your patch on several systems (1ghz win2k, 450mhz win2k, and
266mhz win98) using 50ms, 60ms, 65ms, 75ms, and 100ms.

We definitely want your patch as it is.  Fixing the mailnews problem will
require a different patch in addition to yours.  We may not fix the mailnews
problem depending on how often people run into it and how risky the fix might be
(for branch that is).

Comment 19

[Christopher Blizzard (:blizzard)]

Comment on attachment 92835 [details] [diff] [review]
patch

a=blizzard for 1.1

Comment 20

[Blake Ross]

-> me

Comment 21

[Jesse Ruderman]

*** Bug 134566 has been marked as a duplicate of this bug. ***

Comment 22

[Jaime Rodriguez, Jr.]

let's get this one landed on the trunk, and have it verified by QA asap, so that
we can evaluate its inclusion in the 1.0 branch.

Comment 23

[Blake Ross]

This has been landed on the trunk; John, can you verify?

Comment 24

[John Morrison]

marking fixed (for trunk)

Comment 25

[John Morrison]

verified for trunk 2002-08-05-08-trunk in bookmarks/history/threadPane trees
and the checkin.

[I'll note that while 50msec is appropriate for most trees, it's not quite 
right for the threadPane (or at least it's different). I do (sometimes) like 
being able to scroll over unread messages without marking read, and on a 500MHz 
system, I now can't quite do that. On the other hand, I usually use the mouse.
And, since mailnews has already signed off on this, then I'll stop picking 
nits]

Comment 26

[Johnny Stenback (:jst)]

Argh, so this is what caused keyboard navigation in the thread pane in mailnews
to suck a lot! This has been bugging the hell out of me since this broke, I use
the keyboard to navigate in mailnews *all the time*.

/me whishes this hadn't been checked in w/o making the thread pane in mailnews suck.

Comment 27

[David :Bienvenu]

bug 163946 filed on the mail thread pane issue.

Comment 28

[David :Bienvenu]

Mailnews is obviously different because we actually do something on onselect -
none of the other tree widgets I can think of actually do anything much. And our
"something" can be quite network intensive. Once you start loading an imap
message, you can't simply stop immediately (remember, this is not a wonderfully
stateless protocol like HTTP), and you can't load another message until you've
finished with the previous message, so you have to be very careful.

Comment 29

[Jaime Rodriguez, Jr.]

mass changing adt1.0.1 to adt1.0.2 nominations.

Comment 30

[Jaime Rodriguez, Jr.]

mozilla1.0.1 has passed. carrying over nomination to mozilla1.0.2.

Comment 31

[Johnny Stenback (:jst)]

Beware that this regressed keyboard navigation in mailnews to the point where
it's not usable any more (bug 163946), if this is checked in on the branch we'll
need the fix for the regression as well.

Comment 32

[Daniel Veditz [:dveditz]]

adt-, not clear why we want this on the branch, and the regression fix noted in
comment 31 is not nominated`.