Add keyboard shortcuts to menu dropdowns

[aaronmyatt]

@gnestor

This PR is intended to improve keyboard shortcut discoverability by rendering keyboard shortcuts next to their respective dropdown menu item.

Closes #2759

[aaronmyatt]

@gnestor here's my rudimentry PR. You'll immediately notice my rather direct approach to solving this problem. I wonder if we would be better served by adding an independent this.display_keybindings() step in the constructor, e.g:

        if (this.selector !== undefined) {
            this.element = $(selector);
            this.style();
            this.add_bundler_items();
            this.bind_events();
            this.render_keybindings();
        }

So as to keep the intent clear. Very pleased to hear thoughts and critique!

[gnestor]

Thanks @aaronmyatt! I like your thought of adding a render_keybindings method to MenuBar because it sounds more elegant, but your current implementation works and it conveniently has access to the actions list and the menu item index, whereas something like this.render_keybindings() would require a second iteration through the menu items and a move of the actions list out of this.bind_events.

I cleaned up the styles and I can push to your branch, but it looks like I don't permission. Can you check the "Allow maintainers to edit" checkbox in the right column of this PR on Github?

[aaronmyatt]

@gnestor

[aaronmyatt]

@gnestor any direction on this would be appreciated. I'm trying to think of any way we could extract the id_actions_dict object, iterate over it only once, and essentially 'hook' into that iteration whenever we want to add something, such as events or keybindings in this case.

Just to brain dump everything. My very first idea was to build a menu generator, so we could dynamically build the menu HTML and conveniently render the keybindings next to the menu items as we built them.

[gnestor]

I'm not sure what the issuer is, but when I try to push commits to your branch I get:

$ git push aaronmyatt add-keyboard-shortcuts-to-menu-dropdowns:add-keyboard-shortcuts-to-menu-dropdowns --force
ERROR: Authentication error: Authentication required: You must have push access to verify locks
error: failed to push some refs to 'https://github.com/aaronmyatt/notebook.git'

It would be nice to clean up this logic a bit. I didn't write originally write this so I don't have any particular insights. I say try the menu generator idea and I will review and help. We can also merge this PR once the styling is cleaned up and we can iterate on the menu generator concept in a new PR.

[aaronmyatt]

@gnestor do I need to make you a contributor on my fork? =/

☝️ I've gone ahead and done this anyway

[gnestor]

You shouldn't need to but that did the trick! I cleaned up the first commit so that it doesn't have diffs on added/removed whitespace and pushed a commit to clean up the CSS.

[aaronmyatt]

@gnestor fantastic! Thank you, what's next for this?

I think we need to be sure I'm getting all the possible keybindings. I'll do a few eye ball comparisons between the menu items and the keybinding modal.

Should I chase a cleaner implementation of my current logic?

[gnestor]

Sure, but what you have here is ok in my opinion.

[ellisonbg]

Great to see this!

[aaronmyatt]

@gnestor

I've been reviewing the keybindings attempting to find any that are missing or wrong. Here are my findings:

Action - Keybinding - Note
Save and Checkpoint - meta-s - On a mac the keybinding is cmd-s
Paste Cells Above/Below - - Missing
Find and Replace - - Missing
Toggle Scrolling - - Missing
Toggle Output - - Missing

Though, for example, looking up find-and-replace on the global IPython object, the keyboard shortcut is successfully found:
IPython.keyboard_manager.command_shortcuts.get_action_shortcut('jupyter-notebook:find-and-replace')

Any suggestions what needs to done to ensure the keyboard_manager is fully populated with the shortcuts?

[gnestor]

Save and Checkpoint - meta-s - On a mac the keybinding is cmd-s

I was wondering about that, too. Fortunately, there is a a helper function in quickhelp.js that humanizes shortcuts, so I added a commit that does that for us.

Paste Cells Above/Below - - Missing
Toggle Scrolling - - Missing
Toggle Output - - Missing

The issue here is that this action is not listed in id_actions_dict and we are only iterating through the items in id_actions_dict: https://github.com/jupyter/notebook/blob/master/notebook/static/notebook/js/menubar.js#L252-L296. We can definitely expand this dict to include more actions 👍

Find and Replace - - Missing

The issue here is that this action is not registered until after the menu is created: https://github.com/jupyter/notebook/blob/master/notebook/static/notebook/js/main.js#L163. I added a commit that changes the order since it doesn't seem like it should break any dependencies involved.

[gnestor]

Ok, I just filled in some missing menu item/action pairs in id_actions_dict. It's looking pretty good.

@aaronmyatt I think this is ready to go. If you would like to clean up the implementation, you can always open a new PR.

[aaronmyatt]

@gnestor I only want to read through your commits to understand the full scope of the changes required to fulfill the implementation. Apologies I was not able to keep up with your pace!

Also, let me sincerely say thank you for making this a truly pleasant open source experience (and my first meaningful contribution!). I look forward to the next!

[ghost]

Hello!
We are a group of undergrads from India who are looking to solve issues in open source repositories as a course project. Some of us(including me), being avid users of Jupyter Notebook suggested that we choose an issue in this repository.

Being new contributors we searched for issues labelled as "good first issue" and came across this issue and it really piqued our interest. We are third year CSE students with technical background in fields related to the tech stack of this repo.

We'd like to work on this issue at and also, some advice regarding which issue to work on.We got to this PR from #2759 and after following the thread, hope that we can still work on this. We are really hyped to get this work done and will sincerely appreciate your guidance. Please tell us how to proceed and what we can do in this issue.

[gnestor]

Hi @prof-lupin! Welcome to the Jupyter community!! We would really appreciate your help on this PR. It appears that the current status is that some tasks are failing. I will be happy to mentor you through this (although I will be offline for the next 12 days).

[ghost]

Hey @gnestor ! Thanks for the reply. I wanted to inform you that since our project needs to be semester-long so, we decided to go with a bigger issue in this repo itself. We are going with Issue #2653 and have started the planning and design stuff. One of my team members had dropped a comment on the thread two weeks ago and we still haven't got a reply from anyone. Hoping you could look into it although, personally, I'd like if you could mentor us in that one too.

[akdor1154]

I'm having a peek at this.. after rebasing on master, the (ported to selenium) test failure is valid and shows a bug introduced by this PR. Commenting out the added #paste_cell_replace in the id_actions_dict fixes the behaviour (but probably breaks the shortcut for that item). Gonna look further but I'm new to the codebase, someone else might have better luck.

EDIT: my changes are in https://github.com/akdor1154/jupyter-notebook/tree/add-keyboard-shortcuts-to-menu-dropdowns for anyone interested.

[akdor1154]

Problem found - Notebook.enable_paste already handle adding/removing click events, so with the paste_cell_replace addition to id_actions_dict, it gets duplicate click events created.

I removed the click event manipulation from enable_paste and disable_paste to fix this, now the elements just get set disabled and their handler functions are invoked but return early. Hope this approach is ok (it's how e.g. Insert Image seems to work so there is precedent for this).

EDIT: Browser tests now pass 😎 (see mentioned PR above) . services tests still fail, is there known flakiness here? Or does this still need to be looked into?

EDIT 2: reverted the Casper and phantom updates added earlier in this pr and now everything is green, let's get this baby (well at this age probably more like toddler) merged! @aaronmyatt if you give me push access I'll push my changes to this pr.

[gnestor]

@akdor1154 It looks like CI is passing! 👏

[akdor1154]

ah if a contributor force-pushes my branch to aaron's branch as well that would also work. (force push is needed as it's a rebase, sorry)

[takluyver]

Thanks @akdor1154! Is there a reason not to work from your PR #5096? I can probably do it if necessary, but force-pushing into someone else's repo is something I'd rather not do, and the same commits would get merged whether from @aaronmyatt's repository or from yours.

[akdor1154]

Hmm ok, it looked like Aaron's first contribution to an open source project so he would probably appreciate getting his pr merged directly. Give him a little while to respond?

[aaronmyatt]

@akdor1154 @gnestor @takluyver let me catch up on this and figure out what to do

[aaronmyatt]

@akdor1154 you can now push to my branch

[akdor1154]

@aaronmyatt don't think I have access still, unless I am doing something dumb (possible :) )

[takluyver]

I've pushed the changes to this PR now; thanks both.

Some of the longer items are getting split over two lines, which looks awkward:

Maybe we need to allow the menus to be wider, or even hide the shortcuts if there isn't enough space to display them on one line?

There's also at least 1 shortcut which only works in edit mode (Ctrl-Shift-Minus to split a cell). I wonder if this should be marked somehow to differentiate it from the normal command-mode shortcuts? I don't have any great ideas, though.

[blink1073]

Closing this PR as stale as part of housekeeping. Please feel free to re-open if you'd like to continue work on it @aaronmyatt.