fix: reconnect terminal on non-modified key presses #9686
Conversation
Instead of bubbling. I think maybe what happens here is that xterm is capturing key presses and preventing the event from bubbling? So setting the listener on the capture phase instead works around this. Probably would also work to dipsose the terminal.
I am not sure this actually causes any issues, but might as well.
0eb56e6 to
85ffce8
Compare
| if (isModifier(event)) { | ||
| modifierKeyState[event.key] = true; | ||
| } else if (!isModified()) { | ||
| setStatus("reloading"); |
There was a problem hiding this comment.
kind of a nit: this call won't really do anything. window.location.reload() will block/kill the execution context before the event loop circles back to this.
There was a problem hiding this comment.
Yeah I thought the same thing, it could depend on the browser or something, but in Chromium I tested via a console.log statement and it will happily run multiple times until the reload "kicks in".
There was a problem hiding this comment.
That said, running window.location.reload() multiple times is probably harmless.
There was a problem hiding this comment.
One more thing, setting the status here to reloading causes the overlay to display "Reloading...", so whatever window.location.reload() is doing, it appears to not quite be instant, or there is some other trick at play here.
There was a problem hiding this comment.
wow, ok 😅 I thought I'd even played with this before and touching location instantly killed your code
There was a problem hiding this comment.
that said, it seems like this would benefit from being instant, rather than waiting for react to do an update cycle. maybe we should use useRef for this?
although I guess the ui uses this... maybe it needs to be both 🙃
There was a problem hiding this comment.
lol I know what you mean, I could have sworn I did and saw the same!
There was a problem hiding this comment.
Oh right, good point. Maybe it would be nice if the UI could update immediately as well since if the reload happens too fast they might never see the Reloading... text? Wonder if we can force an update or something.
There was a problem hiding this comment.
I experimented a bit to see if I could get the keydown handler firing twice before React updates and the listener is removed but apparently I am not able to mash a key fast enough lol
I did find a bug though, if you ctrl+tab away then come back via click, the page still thinks ctrl is being pressed and nothing will reload until you press and release ctrl again. Not sure if this can be solved with keydown...too bad it is not like the deprecated keypress event that would tell you if modifier keys were being held down.
There was a problem hiding this comment.
Wait what no keydown does have that information. What am I even doing?? Will refactor. It does not cover all the keys listed in documentation as modifiers, but all the ones that really matter imo.
Co-authored-by: Kayla Washburn <mckayla@hey.com>
Instead of manually tracking modifiers with keydown/keyup, as this can miss keys like in the case of ctrl+tab, leaving ctrl in a permanent "on" state. The other modifiers do not appear to be used in conjunction with other keys, except possibly Fn? So this should be more or less the same functionality.
e86fdfb to
9cab143
Compare
Fixes #8547
Fixes #6732