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

Fix saving shortcut not preventing default #15913

Closed
wants to merge 4 commits into from

Conversation

krassowski
Copy link
Member

@krassowski krassowski commented Mar 5, 2024

References

Code changes

Immediately process keydown event for ctrl + s and on command + shift + c.

User-facing changes

Saving with ctrl + s will no longer show spurious browser dialog

Backwards-incompatible changes

None

@krassowski krassowski added the bug label Mar 5, 2024
@krassowski krassowski added this to the 4.1.x milestone Mar 5, 2024
Copy link

Thanks for making a pull request to jupyterlab!
To try out this branch on binder, follow this link: Binder

((event.ctrlKey || event.metaKey) && event.key.toLowerCase() == 's') ||
// Command + Shift + C on Mac should only open the Command Palette
// (rather than also opening the Dev Tools)
(event.metaKey && event.shiftKey && event.key.toLowerCase() == 'c')
Copy link
Member

@jtpio jtpio Mar 7, 2024

Choose a reason for hiding this comment

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

Should this condition also be valid on non-mac devices?

Copy link
Member

@jtpio jtpio Mar 7, 2024

Choose a reason for hiding this comment

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

So this would also fix #15936

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, though honestly I'm also considering just reverting this handler logic altogether until we get the the improved handling in lumino.

Copy link
Member

Choose a reason for hiding this comment

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

Right, but I guess we'll still want this as a hot fix for now?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, I mean closing this one and merging revert PR #15938. This is mostly because I think we should not be breaking shortcuts contributed by extensions for which we do not have ability to provide a set of workaround like in this PR.

Copy link
Member

Choose a reason for hiding this comment

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

Ah ok got it 👍

Copy link
Member

@jtpio jtpio left a comment

Choose a reason for hiding this comment

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

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants