Bug 12566 - [Drosera] Console history fixups
Summary: [Drosera] Console history fixups
Status: CLOSED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-02-04 02:48 PST by David Smith
Modified: 2008-05-17 09:56 PDT (History)
0 users

See Also:


Attachments
History fixes (4.52 KB, patch)
2007-02-04 02:56 PST, David Smith
darin: review-
Details | Formatted Diff | Diff
This addresses all of Darin's comments except the first one. (4.47 KB, patch)
2007-02-05 15:25 PST, David Smith
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Smith 2007-02-04 02:48:56 PST
The current console history implementation in Drosera is somewhat clunky, and doesn't correctly handle a number of cases (for example: hit up, enter a command, and then see where the history index is. It should be reset to the bottom, but it's not). The soon-to-be-attached patch should make it match bash's implementation.
Comment 1 David Smith 2007-02-04 02:56:10 PST
Created attachment 12911 [details]
History fixes

The aforementioned patch.
Comment 2 Darin Adler 2007-02-04 18:17:14 PST
Comment on attachment 12911 [details]
History fixes

Seems to me we should be looking at keypress events, not keydown or keyup, since it's keypress events that actually change the contents of an input element. To process after the key is hit, we could perhaps listen for the input element's "change" event and keep a global variable to indicate what keypress is being processed.

+    if(event.keyCode != 38 && event.keyCode != 40 && event.keyCode != 13) {

We put spaces between if and ( characters.

+    historyDisplay.scrollTop = history.scrollHeight;

This looks wrong to me. I think you want historyDisplay.scrollHeight here.

review- just because of the scrollHeight mistake.
Comment 3 David Smith 2007-02-05 14:04:47 PST
(In reply to comment #2)
> (From update of attachment 12911 [details] [edit])
> Seems to me we should be looking at keypress events, not keydown or keyup,
> since it's keypress events that actually change the contents of an input
> element. To process after the key is hit, we could perhaps listen for the input
> element's "change" event and keep a global variable to indicate what keypress
> is being processed.

I've fixed all the issues except this one, but I'm not so sure on this; it seems to me that the behavior in the original patch is somewhat cleaner than keeping events around in a global just so we can ignore changes due to up/down/return.
Comment 4 David Smith 2007-02-05 15:25:25 PST
Created attachment 12954 [details]
This addresses all of Darin's comments except the first one.
Comment 5 Darin Adler 2007-02-06 14:29:45 PST
Comment on attachment 12954 [details]
This addresses all of Darin's comments except the first one.

Looks fine, r=me.
Comment 6 Sam Weinig 2007-02-06 15:47:39 PST
Landed in r19447.
Comment 7 Timothy Hatcher 2008-05-17 09:56:03 PDT
Closing since Drosera has been replaced by the new Web Inspector debugger. Moving to the New Bugs component so the Drosera component can be closed and removed.