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

Revert "fix(runtime): Make native modal keyboard interaction consistent with browsers" #21739

Merged

Conversation

littledivy
Copy link
Member

Reverts #18453

Fixes #21602 #21631 #21641

Reasons for revert:

  • alert() and confirm() swallowed ^C with raw mode.
  • prompt() did not re-raise the interrupt signal from rustyline.
  • Default 'Y' on confirm() is a bad default and breaking change.

cc @lionel-rowe

Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

LGTM. It's unfortunate that we have to revert it, but given all the issues raised I think it makes sense.

@lionel-rowe we'll be more than happy to give it another go, but we probably should improve it using low level APIs and not relying on rustyline crate.

@littledivy littledivy merged commit b214623 into main Jan 2, 2024
14 checks passed
@littledivy littledivy deleted the revert-18453-alert-prompt-confirm-keyboard-interaction branch January 2, 2024 04:06
@lionel-rowe
Copy link
Contributor

Default 'Y' on confirm() is a bad default and breaking change.

@littledivy I don't see anything about defaulting to Y in the linked issues. I wouldn't conceptualize the reverted behavior as "defaulting" to Y (the default if !isatty is still false); rather, it's mirroring in-browser behavior by registering an Enter keypress as true.

@lionel-rowe we'll be more than happy to give it another go, but we probably should improve it using low level APIs and not relying on rustyline crate.

@bartlomieju sure, I'll probably take another crack at it sometime, but not sure when. Possibly rustyline can be customized sufficiently; if not, using low-level APIs for prompt would likely require re-implementing a decent chunk of rustyline's functionality (consider line wrapping, including double/zero-width characters, cross-platform compat, handling of copied/pasted text, navigation with arrow keys, various modifier keys, etc.)

@littledivy
Copy link
Member Author

I wouldn't conceptualize the reverted behavior as "defaulting" to Y

I agree with that after seeing how other CLIs handle confirm. It was a breaking change nonetheless, It changed behaviour of a project creation tool I was using where I usually hold enter all the way to respond "n". I wouldn't block any work on this for this reason though.

bartlomieju pushed a commit that referenced this pull request Jan 4, 2024
…nt with browsers" (#21739)

Reverts #18453

Fixes #21602
#21631
#21641

Reasons for revert:
- alert() and confirm() swallowed ^C with raw mode.
- prompt() did not re-raise the interrupt signal from rustyline. 
- Default 'Y' on confirm() is a bad default and breaking change.

cc @lionel-rowe
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.

confirm, alert and prompt no longer respect quit signals since updating to Deno v1.39.0
3 participants