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

Audit keyboard shortcuts for Accel instead of Ctrl #6447

Merged
merged 6 commits into from Jun 1, 2019

Conversation

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented May 31, 2019

Fixes #5023. This changes some Ctrl shortcuts to Accel for more consistency with Mac users. I'd appreciate it if a Mac user could test the binder link to see if things seem to be working correctly.

Command Old New Reason
application:activate-next-tab Ctrl Shift ] Unchanged Avoid collision with browser shortcuts on Mac+Chrome (#1681)
application:activate-previous-tab Ctrl Shift [ Unchanged Avoid collision with browser shortcuts on Mac+Chrome (#1681)
editmenu:redo Ctrl Shift Z Accel Shift Z Better for Mac users
editmenu:undo Ctrl Z Accel Z Better for Mac users
filemenu:close-and-cleanup Ctrl Shift Q Accel Shift Q Better for Mac users ?
helpmenu:toggle Ctrl Shit H Removed Unused shortcut
console:linebreak Ctrl Enter Accel Enter Better for mac users, notebook uses Ctrl Enter to do something different
notebook:enter-command-mode Ctrl M (??) Unchanged Feature parity with classic
notebook:run-cell Ctrl Enter Unchanged Feature parity with classic
notebook:split-cell-at-cursor Ctrl Shift - Unchanged Feature parity with classic

References

#5203
#1681

Code changes

  • Some new keyboard shortcuts
  • Terminals now have a closeAndCleaner menu item
  • The closeAndCleaner delegator delegates to the application close if there is no more specific action to be taken.

User-facing changes

"Close and Cleanup" now applies more uniformly to widgets.
Some Mac users will see their shortcuts change to Cmd.

Backwards-incompatible changes

None.

@ian-r-rose ian-r-rose added this to the 1.0 milestone May 31, 2019
@jupyterlab-dev-mode
Copy link

@jupyterlab-dev-mode jupyterlab-dev-mode bot commented May 31, 2019

Thanks for making a pull request to JupyterLab!

To try out this branch on binder, follow this link: Binder

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jun 1, 2019

Testing this, I noticed that the redo event was never getting triggered on mac. After testing a lot of things, I realized that on mac/firefox, pressing command, shift, and a key was triggering a keydown event with shiftKey set to false. To test, you can go to a blank page, open the console, and do document.addEventListener('keydown', console.log); and then press, say Command Shift \. You can notice that the console says it has a Meta Shift keyboard event, but the shiftKey attribute is false. You can also see this by going to http://nonan.jp/keyevent/input.html and pressing the Command Shift \ combination and note that shiftKey is set correctly for the meta keydown event, but it is false for \ keydown event. In fact, in testing this, I'm seeing that for a keydown event for shift and \ and any combination of control, alt, and command, every event shows event.shiftKey as true except Command Shift \

I also tested firefox on windows, and it correctly reported the shiftKey status (though there command is mapped to the OS modifier on that test page).

The end result is that the Command Shift Z shortcut doesn't work for me on firefox on mac, but does on Chrome on mac. But this looks like a firefox bug, not a JLab bug.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jun 1, 2019

filemenu:close-and-cleanup Ctrl Shift Q Accel Shift Q Better for Mac users ?

Testing this, I noticed that Cmd Shift Q is the system-wide Logout command, and Cmd Ctrl Q is the system-wide lock screen. So I think this should be reverted to Ctrl Shift Q.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jun 1, 2019

filemenu:close-and-cleanup Ctrl Shift Q Accel Shift Q Better for Mac users ?

Testing this, I noticed that Cmd Shift Q is the system-wide Logout command, and Cmd Ctrl Q is the system-wide lock screen. So I think this should be reverted to Ctrl Shift Q.

Actually, for a mac, Cmd W is close tab and Cmd Shift W is close window, whereas Q is associated with closing an application or the system (locking or logging out), so from that perspective, Ctrl Shift W or even Ctrl W probably makes more sense on a mac. However, I can also see the argument for Ctrl Shift Q for compatibility for JLab across operating systems.

Copy link
Contributor

@jasongrout jasongrout left a comment

I think reverting Accel Shift Q back to Ctrl Shift Q, and fixing the small typo Steve pointed out, are all that is needed before a positive review from me.

(and apologies for not catching that Accel Shift Q didn't work on the issue...)

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Jun 1, 2019

Sounds good. @jasongrout Do think the Cmd-Shift issue you saw is bad enough to merit some additional changes of Cmd to Ctrl on macs?

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jun 1, 2019

Sounds good. @jasongrout Do think the Cmd-Shift issue you saw is bad enough to merit some additional changes of Cmd to Ctrl on macs?

Possibly. We can do that on another issue. I'd love if someone on a mac can confirm too to make sure I'm not crazy (or my browser).

@jasongrout jasongrout merged commit 85c872e into jupyterlab:master Jun 1, 2019
9 checks passed
@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jun 1, 2019

Thanks!

@jasongrout jasongrout changed the title Audit accel Audit keyboard shortcuts for Accel instead of Ctrl Jun 1, 2019
@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jun 5, 2019

Sounds good. @jasongrout Do think the Cmd-Shift issue you saw is bad enough to merit some additional changes of Cmd to Ctrl on macs?

Turns out that creating a new firefox profile made things start working for me, and things worked fine on two other macs we were testing, so I think this was just my issue.

@lock
Copy link

@lock lock bot commented Aug 6, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related discussion.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants