Bug 12566 – [Drosera] Console history fixups

• Status: CLOSED
• Resolution: FIXED
• Component: New Bugs

[David Smith]

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.

[David Smith]

Created attachment 12911 [details]
History fixes

The aforementioned patch.

[Darin Adler]

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.

[David Smith]

(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.

[David Smith]

Created attachment 12954 [details]
This addresses all of Darin's comments except the first one.

[Darin Adler]

Comment on attachment 12954 [details]
This addresses all of Darin's comments except the first one.

Looks fine, r=me.

[Sam Weinig]

Landed in r19447.

[Timothy Hatcher]

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.