If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Arrow key selection in tree has absurd delay

VERIFIED FIXED in mozilla1.2alpha

Status

()

Core
XUL
VERIFIED FIXED
15 years ago
6 years ago

People

(Reporter: Blake Ross, Assigned: Blake Ross)

Tracking

Trunk
mozilla1.2alpha
x86
Windows XP
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [adt2])

Attachments

(1 attachment, 1 obsolete attachment)

3.06 KB, patch
brendan
: review+
brendan
: superreview+
blizzard
: approval+
Details | Diff | Splinter Review
(Assignee)

Description

15 years ago
Patch coming up.
(Assignee)

Comment 1

15 years ago
Created attachment 92833 [details] [diff] [review]
patch

Comment 2

15 years ago
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.
Attachment #92833 - Flags: superreview+

Comment 3

15 years ago
Let's get this into the 1.1 branch.  This is a must-have.  Unbelievable. :)
Let's fix this for 1.1 and for the 1.0 branch.  It's safe and easy.

/be
Nominating and stuff.

/be
Blocks: 158377
Keywords: mozilla1.0.1, mozilla1.1
Comment on attachment 92833 [details] [diff] [review]
patch

r=brendan@mozilla.org if you use a single const SELECT_DELAY = 50; in a scope
that works for all uses.

/be
Attachment #92833 - Flags: review+
Comment on attachment 92833 [details] [diff] [review]
patch

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

/be
(Assignee)

Comment 8

15 years ago
Created attachment 92835 [details] [diff] [review]
patch
Attachment #92833 - Attachment is obsolete: true
Comment on attachment 92835 [details] [diff] [review]
patch

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

/be
Attachment #92835 - Flags: superreview+
Attachment #92835 - Flags: review+
*** Bug 159563 has been marked as a duplicate of this bug. ***

Updated

15 years ago
Keywords: adt1.0.1
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

15 years ago
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?
(Assignee)

Comment 13

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

Comment 14

15 years ago
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?
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

15 years ago
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.
(Assignee)

Comment 17

15 years ago
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

15 years ago
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 on attachment 92835 [details] [diff] [review]
patch

a=blizzard for 1.1
Attachment #92835 - Flags: approval+
(Assignee)

Comment 20

15 years ago
-> me
Assignee: hewitt → blaker

Comment 21

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

Comment 22

15 years ago
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.
Blocks: 143047
Whiteboard: [adt2 RTM] [ETA 08/08]
Target Milestone: --- → mozilla1.2alpha
(Assignee)

Comment 23

15 years ago
This has been landed on the trunk; John, can you verify?
QA Contact: shrir → jrgm

Comment 24

15 years ago
marking fixed (for trunk)
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED

Comment 25

15 years ago
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]
Status: RESOLVED → VERIFIED
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

15 years ago
bug 163946 filed on the mail thread pane issue.

Comment 28

15 years ago
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

15 years ago
mass changing adt1.0.1 to adt1.0.2 nominations.
Keywords: adt1.0.2

Updated

15 years ago
Keywords: adt1.0.1

Comment 30

15 years ago
mozilla1.0.1 has passed. carrying over nomination to mozilla1.0.2.
Keywords: mozilla1.0.1 → approval, mozilla1.0.2, nsbeta1
Whiteboard: [adt2 RTM] [ETA 08/08] → [adt2]
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.
adt-, not clear why we want this on the branch, and the regression fix noted in
comment 31 is not nominated`.
Keywords: adt1.0.2 → adt1.0.2-

Updated

9 years ago
Component: XP Toolkit/Widgets: Trees → XUL
QA Contact: jrgmorrison → xptoolkit.widgets
You need to log in before you can comment on or make changes to this bug.