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

Implement new search shortcuts. #9337

Merged
merged 1 commit into from Jan 30, 2022
Merged

Conversation

marxin
Copy link
Contributor

@marxin marxin commented Jun 14, 2021

Implement new shortcuts:

  • "/" for Focus search bar
  • "ESC" - clear highlighted text

Fixes #691.

@mgeier
Copy link
Contributor

mgeier commented Jun 15, 2021

There are already keyboard shortcuts in the basic theme using accesskey, for example using n for next and p for previous page. Some derived HTML themes have (I guess mostly inadvertently) disabled them though.

I think it would be confusing to add another mechanism.

Of course another mechanism has already been added long ago with the left and right arrow keys being mapped by some JavaScript (but this is disabled by default in most themes).

I think if shortcuts with "normal" letters are added, they should be added with the accesskey technology.

I guess an exception could be made for the "special" character /, but I'm not sure whether that's actually a good idea.

In general, I'm not a fan of running some JavaScript that grabs my keypresses, mostly because I'm already using a browser extension (in my case https://github.com/tridactyl/tridactyl, but there are several similar ones) that watches my keypresses and reacts to /. Checking for this key in Sphinx's JavaScript will likely break my current / key functionality.

In my own HTML theme I'm providing a s shortcut via accesskey and I've added a more prominent link to remove the highlighted text (see for example https://insipid-sphinx-theme.readthedocs.io/en/0.2.6/intro.html?highlight=accesskey#features). But I guess the suggested c shortcut could also be added with accesskey, right?

BTW, the suggested change in this PR will also be disabled in most themes unless the navigation_with_keys theme option is True:

if (DOCUMENTATION_OPTIONS.NAVIGATION_WITH_KEYS) {
this.initOnKeyListeners();
}

@marxin
Copy link
Contributor Author

marxin commented Jun 16, 2021

There are already keyboard shortcuts in the basic theme using accesskey, for example using n for next and p for previous page. Some derived HTML themes have (I guess mostly inadvertently) disabled them though.

Good to know. It seems like a pretty standard mechanism, however, I see some limitations:

  1. In Chrome p accesskey does not work for me for some reason
  2. In Firefox one needs to use ugly ALT + 'SHIFT' + 'key' and it only shows URL preview and one needs to click enter.

My inspiration was pages like Gmail, Trello or GitHub. Where shortcuts are pretty simple and easy to use.

I think it would be confusing to add another mechanism.

Of course another mechanism has already been added long ago with the left and right arrow keys being mapped by some JavaScript (but this is disabled by default in most themes).

Yes, one has to enable it explicitly.

I think if shortcuts with "normal" letters are added, they should be added with the accesskey technology.

I guess an exception could be made for the "special" character /, but I'm not sure whether that's actually a good idea.

In general, I'm not a fan of running some JavaScript that grabs my keypresses, mostly because I'm already using a browser extension (in my case https://github.com/tridactyl/tridactyl, but there are several similar ones) that watches my keypresses and reacts to /. Checking for this key in Sphinx's JavaScript will likely break my current / key functionality.

That's pretty uncommon use to me. I prefer native shortcuts provided by a web page.

In my own HTML theme I'm providing a s shortcut via accesskey and I've added a more prominent link to remove the highlighted text (see for example https://insipid-sphinx-theme.readthedocs.io/en/0.2.6/intro.html?highlight=accesskey#features). But I guess the suggested c shortcut could also be added with accesskey, right?

I like the other navigation shortcuts: Up, Index and I could also imagine going to Home.

BTW, the suggested change in this PR will also be disabled in most themes unless the navigation_with_keys theme option is True:

We can consider coming up with a more common name (shortcuts) and possibly enable it.

@mgeier
Copy link
Contributor

mgeier commented Jun 17, 2021

In Chrome p accesskey does not work for me for some reason

Hmmm, it works for me on Chromium.
Which theme are you using?

In Firefox one needs to use ugly ALT + 'SHIFT' + 'key'

I very quickly got used to this, but I agree that it's not very straightforward.

What's worse, it's hard to discover, it would be great if something could be done about this.

and it only shows URL preview and one needs to click enter.

I don't understand.
On Firefox, it also works fine for me, I don't have to hit Enter or click anywhere at any point.

I prefer native shortcuts provided by a web page.

Yeah, it's definitely a matter of preference.

The question is: is it worth how much it helps users (like you) compared to how much it hurts other users (like me)?

I think adding more shortcuts with accesskey wouldn't hurt anyone.

@marxin
Copy link
Contributor Author

marxin commented Aug 9, 2021

I would like to extend the group of people who discuss the functionality.
Adding @TimKam who added the shortcuts and others involved in the original PR: @shimizukawa, @lehmannro, @tk0miya.

@TimKam
Copy link
Member

TimKam commented Aug 9, 2021

Nit-pick: consistent use of quotes -> I'd say single quotes instead of double quotes

What are the remaining open questions?
I don't have a strong opinion on default vs. not default, but if we change the name of the config option, we could also consider an object: because on the long run, too many short cuts that are all activated by default can be confusing, and the best option could be an object that defaults to a good compromise.

@marxin
Copy link
Contributor Author

marxin commented Aug 10, 2021

Nit-pick: consistent use of quotes -> I'd say single quotes instead of double quotes

Sure, adjusted.

What are the remaining open questions?

Apparently, @mgeier didn't like the idea. However, my patch only extends the existing useful shortcuts.

I don't have a strong opinion on default vs. not default, but if we change the name of the config option, we could also consider an object: because on the long run, too many short cuts that are all activated by default can be confusing, and the best option could be an object that defaults to a good compromise.

Is it something that can be addressed in the future once this pull request is merged?
Thanks.

@tk0miya tk0miya added builder:html type:proposal a feature suggestion labels Aug 10, 2021
@tk0miya tk0miya added this to the 4.2.0 milestone Aug 10, 2021
@tk0miya
Copy link
Member

tk0miya commented Aug 10, 2021

IMO, / is a commonly used shortcut. So +1 for adding. On the other hand, clearing highlight is not common. So -0 for it.

Note: I don't have an opinion to implement them because I'm not familiar with HTML and JS.

@tk0miya
Copy link
Member

tk0miya commented Aug 10, 2021

Please merge the HEAD of 4.x branch to your PR to fix the CI error. It will be fixed soon.

@mgeier
Copy link
Contributor

mgeier commented Aug 10, 2021

For illustration purposes, I've created #9539, showing hot accesskey can be used to hide search matches. However, I don't think that any shortcut key is necessary for that functionality.

@jakobandersen
Copy link
Contributor

jakobandersen commented Aug 10, 2021

Whatever the shortcut mechanism is (didn't know about accessKey) I like the addition of both search an clearing highlight. Though, it would be nice if it would clear the actual highlight, clear the highlight part of the URL, and all while not refreshing the page.
I often search for something in order to find an entity to shared a link to, but the perma-link contains the highlight=...-part which then needs to manually be removed.

I would also suggest to add a ? shortcut which shows an overview of shortcuts, e.g., like Gmail has.

@marxin
Copy link
Contributor Author

marxin commented Aug 11, 2021

Please merge the HEAD of 4.x branch to your PR to fix the CI error. It will be fixed soon.

I've just done that.

@marxin
Copy link
Contributor Author

marxin commented Aug 11, 2021

For illustration purposes, I've created #9539, showing hot accesskey can be used to hide search matches. However, I don't think that any shortcut key is necessary for that functionality.

I think it's handy as e.g. sphinx_rtd_theme template does not show the Hide Search Matches button.

@marxin
Copy link
Contributor Author

marxin commented Aug 11, 2021

Whatever the shortcut mechanism is (didn't know about accessKey) I like the addition of both search an clearing highlight. Though, it would be nice if it would clear the actual highlight, clear the highlight part of the URL, and all while not refreshing the page.

I've just one that, browser history is updated accordingly.

I often search for something in order to find an entity to shared a link to, but the perma-link contains the highlight=...-part which then needs to manually be removed.

That's exactly my use case why I came up with the pull request.

I would also suggest to add a ? shortcut which shows an overview of shortcuts, e.g., like Gmail has.

Great idea. Does Sphinx have any Overlay that can be used similarly to what Gmail or Github provides?
One small note, I changed the shortcut c to h (Hide Search Matches), where I think using uppercase shortcuts is not a good practice.

@TimKam
Copy link
Member

TimKam commented Aug 11, 2021

Could you also update the documentation? See: https://www.sphinx-doc.org/en/master/usage/theming.html

I don't think Sphinx has any overlay feature, and I'd suggest that the ? feature may be addressed in a future PR.

@marxin
Copy link
Contributor Author

marxin commented Aug 11, 2021

Could you also update the documentation? See: https://www.sphinx-doc.org/en/master/usage/theming.html

I've just done that.

I don't think Sphinx has any overlay feature, and I'd suggest that the ? feature may be addressed in a future PR.

Sure, works for me.

@mgeier
Copy link
Contributor

mgeier commented Aug 11, 2021

I think it would be a bad idea to merge this as is.

I personally don't like it when JavaScript grabs my key presses (because I'm using them for something else), but this is only one part of the problem.

Even for people who maybe would like this exact implementation, they would still not get it in most cases, because navigation_with_keys is disabled by default.

Using accesskey would have none of those problems. And that's what Sphinx traditionally uses.

And of course theme authors are free to support additional JavaScript keyboard shortcuts as they see fit.


BTW, having this in the navigation_with_keys doesn't make too much sense, because neither focusing the search field nor removing search hits has anything to do with "navigation", right?


I do like the URL manipulation though, I think it would be great if this could be added to hideSearchWords() (see #9337 (comment)).

@marxin
Copy link
Contributor Author

marxin commented Aug 12, 2021

I think it would be a bad idea to merge this as is.

Heh.

I personally don't like it when JavaScript grabs my key presses (because I'm using them for something else), but this is only one part of the problem.

I have an opposite idea as accesskey seems like an interesting generic approach, but the shortcuts are complicated and I'm not aware of a webpage using them (Gmail, Trello, Github). The elegance of shortcuts is that they are short and quick to use.

Even for people who maybe would like this exact implementation, they would still not get it in most cases, because navigation_with_keys is disabled by default.

Using accesskey would have none of those problems. And that's what Sphinx traditionally uses.

But it has other problems (not being known, more complicated to use).

And of course theme authors are free to support additional JavaScript keyboard shortcuts as they see fit.

BTW, having this in the navigation_with_keys doesn't make too much sense, because neither focusing the search field nor removing search hits has anything to do with "navigation", right?

Sure, but it does not make sense to rename it or come up with a different one. I can read navigation_with_keys as a synonym for shortcuts or hotkeys.

@tk0miya tk0miya modified the milestones: 4.4.0, 4.5.0 Jan 16, 2022
@marxin marxin force-pushed the new-shortcuts branch 2 times, most recently from 6c58f99 to b7eb90d Compare January 16, 2022 10:09
@marxin marxin marked this pull request as draft January 16, 2022 10:11
@marxin
Copy link
Contributor Author

marxin commented Jan 16, 2022

I'm not sure if it's necessary to have two separate options for / and Esc. What do you think?

Yes, I agree that one option would be better.

And naming is of course hard. The name search_with_keys is a bit weird, because to search something, you'll have to type the search term, which you typically do with keys, right? So "search" is always "search with keys", isn't it?

What about using search_shortcuts or enable_search_shortcuts or something like that? Those would also allow to combine / and Esc, which are both shortcuts related to search.

Another (minor) problem with both search_with_keys and remove_highlight_with_keys is the use of the plural "keys", but it's only a single key in each case (except if Shift is needed to get /). In the future, more keys might be added, so using the singular "key" also won't be a good idea.

All fixed as I chose enable_search_shortcuts name now.

I've mentioned above (#9337 (comment)) that this feature is broken for me ... now it works! I guess this is because I'm also using a keyboard where Shift has to be pressed to get /. Thanks for finding and fixing this @jakobandersen!

Now it should work both on layouts where one needs Shift and one doesn't.

Finally, it might be interesting to check if this feature plays well with https://github.com/readthedocs/readthedocs-sphinx-search.

Can you please test it for me?

Anyway, thanks for useful comments.

@marxin marxin force-pushed the new-shortcuts branch 3 times, most recently from 6e09439 to 38403ea Compare January 16, 2022 10:25
@marxin marxin changed the title Implement new shortcuts: Implement new search shortcuts. Jan 16, 2022
@marxin marxin marked this pull request as ready for review January 16, 2022 10:26
@mgeier
Copy link
Contributor

mgeier commented Jan 17, 2022

Finally, it might be interesting to check if this feature plays well with https://github.com/readthedocs/readthedocs-sphinx-search.

Can you please test it for me?

Yeah, this is kinda hard to test, because it only works on RTD.

I've added it on a (temporary) branch on this dormant project of mine: https://magic-call.readthedocs.io/en/sphinx-search-with-shortcuts/

It seems to work fine, AFAICT.

The extension seems to not add the highlight thing when clicking on a search result, though (which is IMHO a bug on their side).

If you have a highlight link though (e.g. https://magic-call.readthedocs.io/en/sphinx-search-with-shortcuts/?highlight=package#the-python-package-magic-call) and then you hit / to search but then decide to cancel the search with Esc, the highlighting will also be cleared, which is probably not intentional.

I guess that's also a bug on their side, I've made a PR: readthedocs/readthedocs-sphinx-search#103. I couldn't test this though, because I couldn't manage to minify the thing.

@marxin
Copy link
Contributor Author

marxin commented Jan 18, 2022

@mgeier Thank you for the testing effort!

@mgeier
Copy link
Contributor

mgeier commented Jan 18, 2022

In the meantime I've found out that readthedocs-sphinx-search is already supposed to grab the / keypress, but it doesn't seem to work on keyboards which need Shift to get /, see readthedocs/readthedocs-sphinx-search#31.

@jakobandersen
Copy link
Contributor

Now that 4.4 has been released, can you rebase it on top of 4.x? And move the CHANGES entry correspondingly?

- "/" for Focus search bar
- "ESC" - clear highlighted text

Fixes sphinx-doc#691.

Co-authored-by: Jakob Lykke Andersen <jakobandersen@users.noreply.github.com>
@marxin
Copy link
Contributor Author

marxin commented Jan 22, 2022

Now that 4.4 has been released, can you rebase it on top of 4.x? And move the CHANGES entry correspondingly?

Done and moved CHANGES to the right place. Thanks for the approval.

@tk0miya
Copy link
Member

tk0miya commented Jan 23, 2022

Is it time to merge this? I feel the discussion becomes too long. So how about merging this if you're okay.

@pradyunsg
Copy link
Contributor

Given the thumbs-up reactions, looks like the answer is a yes. :)

@marxin
Copy link
Contributor Author

marxin commented Jan 25, 2022

Yes, please merge it.

@tk0miya
Copy link
Member

tk0miya commented Jan 30, 2022

Okay, merging now. Thank you for your contribution, all!

@tk0miya tk0miya merged commit 799385f into sphinx-doc:4.x Jan 30, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 2, 2022
@marxin marxin deleted the new-shortcuts branch June 17, 2022 11:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
builder:html type:proposal a feature suggestion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow keyboard shortcut / to focus on search
7 participants