Closed
Bug 159477
Opened 23 years ago
Closed 22 years ago
Arrow key selection in tree has absurd delay
Categories
(Core :: XUL, defect)
Tracking
()
VERIFIED
FIXED
mozilla1.2alpha
People
(Reporter: bugzilla, Assigned: bugzilla)
References
Details
(Whiteboard: [adt2])
Attachments
(1 file, 1 obsolete file)
3.06 KB,
patch
|
brendan
:
review+
brendan
:
superreview+
blizzard
:
approval+
|
Details | Diff | Splinter Review |
Patch coming up.
Assignee | ||
Comment 1•23 years ago
|
||
Comment 2•23 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•23 years ago
|
||
Let's get this into the 1.1 branch. This is a must-have. Unbelievable. :)
Comment 4•23 years ago
|
||
Let's fix this for 1.1 and for the 1.0 branch. It's safe and easy. /be
Comment 6•23 years ago
|
||
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 7•23 years ago
|
||
Comment on attachment 92833 [details] [diff] [review] patch Or a <field>, blake said (no const fields in xbl). /be
Assignee | ||
Comment 8•23 years ago
|
||
Attachment #92833 -
Attachment is obsolete: true
Comment 9•23 years ago
|
||
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+
Comment 10•23 years ago
|
||
*** Bug 159563 has been marked as a duplicate of this bug. ***
Comment 11•23 years ago
|
||
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•23 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•23 years ago
|
||
I have a 2ghz machine with 512 MB of ram and it works perfectly for me.
Comment 14•23 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?
Comment 15•23 years ago
|
||
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•23 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•23 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•23 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 19•23 years ago
|
||
Comment on attachment 92835 [details] [diff] [review] patch a=blizzard for 1.1
Attachment #92835 -
Flags: approval+
Comment 21•22 years ago
|
||
*** Bug 134566 has been marked as a duplicate of this bug. ***
Comment 22•22 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.
Assignee | ||
Comment 23•22 years ago
|
||
This has been landed on the trunk; John, can you verify?
QA Contact: shrir → jrgm
Comment 24•22 years ago
|
||
marking fixed (for trunk)
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Comment 25•22 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
Comment 26•22 years ago
|
||
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•22 years ago
|
||
bug 163946 filed on the mail thread pane issue.
Comment 28•22 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 30•22 years ago
|
||
mozilla1.0.1 has passed. carrying over nomination to mozilla1.0.2.
Whiteboard: [adt2 RTM] [ETA 08/08] → [adt2]
Comment 31•22 years ago
|
||
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•22 years ago
|
||
adt-, not clear why we want this on the branch, and the regression fix noted in comment 31 is not nominated`.
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.
Description
•