Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes to Console and Variables keybindings #2864

Merged
merged 4 commits into from
Apr 25, 2024
Merged

Conversation

lionel-
Copy link
Contributor

@lionel- lionel- commented Apr 23, 2024

These fixes depend on each other so I gathered them in a single PR. Each commit can be cleanly reviewed separately though. Happy to split in multiple PRs if that would be preferred.

Fix shadowing of user keybindings

Addresses #2857.

The first commit improves the detection of extra modifiers in our keydown handlers for Console and Variables viewpanes. This allows unrelated commands to dispatch correctly, e.g. our Cmd+C handler will not prevent a user keybinding Shift+Cmd+C from running.

QA notes: The keys handled in these locations should still work as expected:

In particular, typing text when the console is scrolled up should scroll all the way to the bottom, there should not be some pixels left to scroll.

Some of the keybindings such as for Copy, Cut, Paste, and Select all differ depending on the platform. MacOs uses the Cmd prefix key whereas other platforms use Ctrl.

Fix keybindings in foreign keyboard layouts

Addresses #2724.

The second commit changes our handlers to use the key field of keyboard events instead of code fields. The former represents the character typed according to the user's keyboard layout. The latter represents the physical location on the keyboard, which doesn't match the layout.

Restore cut command in console

Addresses #2549.

Third commit removes a registered keybinding for Cmd/Ctrl+X that was bound to an empty command. It was added in #1118, I assume to pair it with the context menu action for Cut. However removing it does not affect the context menu. And it turns out that the default Cut implementation pretty much does what we want:

  • It's disabled when the selection is read-only or overlaps with a read-only section.
  • It's enabled when the selection is writable, as in our input code editor.

Add paste handler to readline prompts

Addresses #2863.

Fourth commit implements select-all and paste in readline prompts. Paste is a quick stopgap implementation on top of the <input> HTML element. The direct HTML manipulation confuses undo/redo. I think we'd be better off using a VS Code code text editor that accepts text edits so that undo/redo work out of the box.

Copy link
Collaborator

@jmcphers jmcphers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, and was able to verify these fixes with a locally built copy of the branch! Feels nice.


// Paste handler
case 'v': {
// This is a stopgap implementation. Because we are
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine as a stopgap, but it is not clear to me why we need to reimplement paste here -- shouldn't we get that for free as part of the HTML input control?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering too, given that Copy and Cut are working as expected. Perhaps relatedly, the onPaste= handler is not run if I pass one to the <input> element. VS Code's keyboard troubleshooter does not react to the event so it's not some dispatch that takes over.

@lionel- lionel- force-pushed the bugfix/console-keybindings branch from fc54e95 to 4469cab Compare April 25, 2024 10:04
@lionel- lionel- merged commit cf953db into main Apr 25, 2024
1 check passed
@lionel- lionel- deleted the bugfix/console-keybindings branch April 25, 2024 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants