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

Add keyboard navigation #1095

Merged
merged 2 commits into from Oct 3, 2017

Conversation

Projects
None yet
3 participants
@mreinhardt
Contributor

mreinhardt commented Nov 17, 2016

Right arrow or n for next page. Left arrow or p for previous page. Disabled when cursor is in an input element.

@waylan waylan added this to the 1.0.0 milestone Nov 17, 2016

@waylan waylan added the Enhancement label Nov 17, 2016

@waylan

This comment has been minimized.

Member

waylan commented Nov 17, 2016

This is a nice feature to have. Thanks for the contribution.

I noticed that both the n/p and the right and left arrow keys are used for navigation. Just did a quick search and the discussion at sphinx-doc/sphinx#2064 is interesting. Sphinx appears to only be using the arrow keys. Is there any president to also using n/p? And their discussion includes a concern that using the arrow keys might break normal left/right scrolling in the browser. They decided to go forward anyway and neither their themes nor ours should have any left/right scrolling anyway. Just wanted to make sure we are aware of the issues involved.

Finally, I appears that the pypy3 tests failed (on Travis) due to an error creating the envs. I restarted them, but the outcome was the same. In any even, the problem does not appear to be related to this PR. Even more curious is that the appveyor tests were never even triggered. Again, it does not appear to be an issue with this PR, but we may have some new issues with our automation setup. Sigh.

@mreinhardt

This comment has been minimized.

Contributor

mreinhardt commented Nov 17, 2016

I can remove the n/p. I thought about [/] also. We could mitigate the arrow/horiz-scroll issue by using either of these instead, but like you said it doesn't seem to matter in our case unless someone made a wacky theme.

No idea about the Travis failure, it's only a little JS!

@waylan

This comment has been minimized.

Member

waylan commented Nov 17, 2016

I can remove the n/p. I thought about [/] also.

I would prefer we use whatever is common. Other than my quick search which tuned up what Sphinx does, I have no idea what other projects use for this use case.

In my experience, many apps us j/k (from vim for next/previous line), but they correspond with the up/down arrow keys rather than right/left (for example navigating to the next/previous email which are usually in a vertical list). Still might be the least surprising to people who actually use keyboard shortcuts regularly. There is a pretty comprehensive list of keyboard shortcuts across multiple platforms on Wikipedia, but nothing else stands out to me as a good fit.

@waylan

This comment has been minimized.

Member

waylan commented Nov 17, 2016

I should add that n/p is very English specific. They could be different for every language.

In contrast, the vim shortcuts apparently come from the ADM-3A keyboard layout, where the ←↓↑→ keys corresponded with the HJKL keys (in that order) not because of any meaning implied by the letters, but because if their location on the keyboard (see image). By that metric, we should use h/l for previous/next respecitively. And the proper way to code that would be to use the key codes for those specific keys even if the user has their keyboard mapped to different letters (ex: Dvorak or non-English).

Regardless, I think it makes sense to leave the arrow keys support as it is now.

@mreinhardt

This comment has been minimized.

Contributor

mreinhardt commented Nov 17, 2016

Another option is that arrows could be default with a setting in mkdocs.yml to override this behavior, and even disable keyboard shortcuts entirely. Would take a lot more code for this but I can tackle it if you think it's necessary.

@mreinhardt

This comment has been minimized.

Contributor

mreinhardt commented Nov 17, 2016

BTW - Gmail uses n/p for "Read previous/next message", but I understand why that's not the best option here.

@waylan

This comment has been minimized.

Member

waylan commented Nov 17, 2016

Let's not add any more settings.

Really, Gmail uses n/p? Way back when they first introduced keyboard shortcuts I thought they used j/k. But I haven't used Gmail in years. Maybe that's too nerdy/geeky/[insert appropriate adjective here] for the average user?

@mreinhardt

This comment has been minimized.

Contributor

mreinhardt commented Nov 17, 2016

Yes, vimmy j/k for next/prev email thread, n/p is for next/prev message in a thread.

@waylan

This comment has been minimized.

Member

waylan commented Nov 17, 2016

Okay, I see three options:

  1. Follow Sphinx's lead and use arrows only.
  2. Use a bunch of shortcuts for each action. Perhaps /h/p for "previous" and /l/n for "next". That way, whatever the user tries will be likely to work. We could even throw j/k in there.
  3. Pick a sensible single set of shortcuts and create a popup modal (using the existing bootswatch tools) which documents them. Maybe the model displays on the ? shortcut. We could then add a few others to focus the search box, etc. This would only work for bootswatch/bootstrap themes though, so readthedocs is out.

I'm partial to 1 or 3 but would be willing to accept whichever one gets built.

@mreinhardt

This comment has been minimized.

Contributor

mreinhardt commented Nov 17, 2016

3 seems best to me. I don't understand why it wouldn't work on readthedocs though, easy enough to make a little modal with some basic vanilla CSS/JS.

@d0ugal

This comment has been minimized.

Member

d0ugal commented Nov 17, 2016

1 or 3 is fine. Maybe even 1 now as it is easy then 3 later.

@mreinhardt mreinhardt force-pushed the mreinhardt:keyboard-nav branch from 59182e5 to 88de372 Nov 18, 2016

@mreinhardt

This comment has been minimized.

Contributor

mreinhardt commented Nov 18, 2016

@waylan @d0ugal I amended the commit with the changes discussed.

@mreinhardt mreinhardt force-pushed the mreinhardt:keyboard-nav branch 2 times, most recently from abadc03 to 8f78c0a Nov 18, 2016

@waylan

This comment has been minimized.

Member

waylan commented Nov 18, 2016

@mreinhardt Wow. Looks good at a glance. I'll take a closer look later.

However, you are getting a few minor csslint failures. You can ignore the other (pypy3) failures as they are not related to this PR.

@mreinhardt mreinhardt force-pushed the mreinhardt:keyboard-nav branch from 8f78c0a to 8682d8a Nov 18, 2016

@mreinhardt

This comment has been minimized.

Contributor

mreinhardt commented Nov 18, 2016

@waylan Installed tox and csslint and fixed issues.

@mreinhardt

This comment has been minimized.

Contributor

mreinhardt commented Nov 21, 2016

I know it's unrelated, but is there anything I can do to help fix the other tests?

@waylan

This comment has been minimized.

Member

waylan commented Nov 22, 2016

The other failures appears to be an incompatibility between the latest version of pip (or one of its dependencies) and pypy3. The failure is happening in environment creation, long before our tests are run. Not sure what can be done about that as we don't get to pick which version of pip gets installed in each environment (at least I'm not aware of a way). Hopefully either pip or pypy3 will get a quick update which resolves the problem. Otherwise, I'm not sure what the best approach is here.

In any event, sorry but I just haven't had time to look closely at your changes in this PR yet. Hopefully I'll get to it sometime this week.

@waylan

This comment has been minimized.

Member

waylan commented Nov 23, 2016

I took a closer look and overall I think this looks good. However, there are a couple minor issues.

First of all the table in the modal needs adjusted in the readthedocs theme (its fine in the mkdocs theme). It should probably stretch to fill the modal or the modal should shrink to the size of the table. This is a nitpick, but we might as well get it right.

The other issue is a little more of a problem. The modal works fine except when you use one of the shortcuts before closing the modal (yes, the normal ways of closing the modal works such as the esc key, clicking the close button or clicking elsewhere on the page). For example, if you open the modal, then press the search shortcut (Alt-s), the modal does not close, but the search shortcut is followed. Here is an example in the readthedocs theme:

rtd

As you can see, I've started to type a search term and the browser's history feature also shows a dropdown of past search terms. It would be preferable if the modal had closed when I hit the shortcut.

However, even more concerning is the mkdocs theme:

mkdocs

In that case, the search modal appears behind the shortcut modal (the darkened background actually doubles in darkness). And even stranger, the search box has focus and you can type a search term and get results (and the browser shows search history), all while the search modal is hidden behind the shortcut modal. If you use the normal methods for closing the shortcut modal, then the search modal is properly visible. This is clearly a broken user interface. I could live with the other issues, but this needs to be fixed.

I will add that the other shortcuts are not an issue as the navigation shortcuts load another page and the modal gets reset anyway.

I see a few possible solutions:

  1. Close the modal on any key-press. However, we only need to be listening for key-presses when it is open, so I'm not sure this is the right solution.
  2. Close the modal (if it is open) as the first action of any keyboard shortcut. Of course, this would mean that only the shortcut keys would close it, not any key.
  3. Disable the shortcuts when any modal is open, which more closely matches the behavior of the modal in other respects (scrolling is disabled, you can only tab through the modal elements, etc). The user would need to close the modal before using a shortcut.

I think option 3 probably makes the most sense. AFAICT, this is the behavior used by GMail. And perhaps we should list esc as a shortcut to close the modal.

Finally, the up/down arrow keys do nothing in the mkdocs theme when a modal is open (which is normal behavior) but still scroll the page behind the modal in the readthedocs theme. Also in the readthedocs theme you can tab through the entire page, but only through the modal elements in the mkdocs theme. This inconsistency should be fixed as well.

@waylan

This comment has been minimized.

Member

waylan commented Nov 23, 2016

Something keeps bothering me about this. The readthedocs theme is intended to be a clone of the Sphinx theme by the same name. Thus far, the only differences are due to low level differences between MkDocs and Sphinx. Adding a modal feels like a little too much of a departure from that. Perhaps we should only use the modal in the mkdocs theme and in the readthedocs theme simply clone Sphinx shortcuts (right and left arrows), but offer nothing more.

A bonus is that we would not need to develop and maintain a complete modal system for the readthedocs theme. I, for one, am not really interested in maintaining that long-term. Disabling scrolling/tab etc, and other work to match the bootswatch modal would not be needed then.

@mreinhardt

This comment has been minimized.

Contributor

mreinhardt commented Nov 23, 2016

@waylan Sounds good. Will only use / on readthedocs theme, and will implement "Close the modal (if it is open) as the first action of any keyboard shortcut. Of course, this would mean that only the shortcut keys would close it, not any key." on the mkdocs theme. Coming right up.

@mreinhardt mreinhardt force-pushed the mreinhardt:keyboard-nav branch from 8682d8a to 12a65f1 Nov 23, 2016

@mreinhardt

This comment has been minimized.

Contributor

mreinhardt commented Nov 23, 2016

@waylan Amended commit with requested fixes.

@waylan

Looks good. The search/help modal weirdness is resolved sufficiently. I still wonder if all keyboard shortcuts should be disabled when any modal is open, but this is a clean alternative and probably easier to implement.

I really liked being able to completely navigate through the entire site only from the keyboard. Address the few minor issues I highlighted inline and this should be ready to go.

Thanks for doing the work on this.

page = $('[role="navigation"] a:contains(Previous):first').prop('href');
break;
case 83: // s
if (e.altKey) {

This comment has been minimized.

@waylan

waylan Nov 24, 2016

Member

Curious why you saw the need to require the Alt key here? I'm fine with it, just wondering why we can't use an s alone.

This comment has been minimized.

@mreinhardt

mreinhardt Nov 26, 2016

Contributor

Umm, good question! I don't remember. Just s seems fine, will change.

<td>Open this help</td>
</tr>
<tr>
<td><kbd>←</kbd></td>

This comment has been minimized.

@waylan

waylan Nov 24, 2016

Member

Perhaps we should use the HTML entity (&larr;) rather than the Unicode character in the template. With the proper encoding the Unicode character should work, but the HTML entity is less likely to break across various platforms, browsers, servers, etc.

This comment has been minimized.

@mreinhardt

mreinhardt Nov 26, 2016

Contributor

You got it.

<td>Previous page</td>
</tr>
<tr>
<td><kbd>→</kbd></td>

This comment has been minimized.

@waylan

waylan Nov 24, 2016

Member

And this should be the HTML entity &rarr;.

This comment has been minimized.

@mreinhardt

mreinhardt Nov 26, 2016

Contributor

🆗

<td>Next page</td>
</tr>
<tr>
<td><kbd>Alt</kbd>+<kbd>s</kbd></td>

This comment has been minimized.

@waylan

waylan Nov 24, 2016

Member

I noticed that the plus sign looks out-of-line (vertically) with the Alt and s keys. Its a nitpick, but it would be nice to get it right.

screencapture-127-0-0-1-8000-1480009014255

Actually, as per this, you should have:

<td><kbd><kbd>Alt</kbd>+<kbd>s</kbd></kbd></td>

That way, the plus sign is still in the outer kbd element, but separate from the actual keys in the inner kdb elements. Then just apply the key styling to the inner elements. That may resolve the issue itself. If not, then it at least provides an extra styling hook.

This comment has been minimized.

@mreinhardt

mreinhardt Nov 26, 2016

Contributor

Removing Alt in shortcut so this will go away, but good to know.

@waylan

This comment has been minimized.

Member

waylan commented Nov 24, 2016

Oh, one more thing, if you rebase against the upstream master branch, then the failing tests will go away as we've addressed that issue.

@mreinhardt

This comment has been minimized.

Contributor

mreinhardt commented Nov 26, 2016

Will do!

@mreinhardt mreinhardt force-pushed the mreinhardt:keyboard-nav branch from 12a65f1 to 42e22aa Nov 26, 2016

@mreinhardt

This comment has been minimized.

Contributor

mreinhardt commented Nov 26, 2016

@waylan All done! (I hope.)

@waylan

waylan approved these changes Nov 26, 2016

@waylan

This comment has been minimized.

Member

waylan commented Nov 26, 2016

I'm happy with this. It just needs to be added to the Rease Notes. However, we haven't added a section for the next release yet. I expect we'll probably do a bug-fix release or two before our next point release. Actually the next point release will probably be 1.0.0, and this has been assigned to that milestone so we won't forget it.

@mreinhardt

This comment has been minimized.

Contributor

mreinhardt commented Nov 28, 2016

Excellent, cheers!

@waylan waylan self-assigned this Dec 2, 2016

@waylan

This comment has been minimized.

Member

waylan commented Oct 3, 2017

Hmm, looks like I forgot about this. @mreinhardt if you update this to the current state in master, I'll merge this in.

@waylan waylan added the Update needed label Oct 3, 2017

@mreinhardt

This comment has been minimized.

Contributor

mreinhardt commented Oct 3, 2017

@waylan waylan merged commit 9f5365a into mkdocs:master Oct 3, 2017

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
codecov/project 93.03% (target 90%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment