Implement new search shortcuts. #9337

marxin · 2021-06-14T07:39:13Z

Implement new shortcuts:

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

Fixes #691.

mgeier · 2021-06-15T20:18:42Z

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:

sphinx/sphinx/themes/basic/static/doctools.js

Lines 157 to 159 in 355937a

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

marxin · 2021-06-16T10:31:33Z

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:

In Chrome p accesskey does not work for me for some reason
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 · 2021-06-17T11:06:53Z

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 · 2021-08-09T08:59:17Z

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 · 2021-08-09T22:10:52Z

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 · 2021-08-10T06:49:18Z

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 · 2021-08-10T17:09:59Z

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 · 2021-08-10T17:11:58Z

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

sphinx/themes/basic/static/doctools.js

mgeier · 2021-08-10T17:59:03Z

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 · 2021-08-10T18:10:50Z

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 · 2021-08-11T07:39:25Z

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 · 2021-08-11T07:41:15Z

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 · 2021-08-11T07:48:27Z

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 · 2021-08-11T08:06:01Z

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 · 2021-08-11T08:36:17Z

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.

sphinx/themes/basic/static/doctools.js

mgeier · 2021-08-11T18:15:17Z

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 · 2021-08-12T08:05:45Z

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.

marxin · 2022-01-16T10:19:58Z

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.

mgeier · 2022-01-17T19:34:24Z

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 · 2022-01-18T12:08:12Z

@mgeier Thank you for the testing effort!

mgeier · 2022-01-18T19:29:48Z

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 · 2022-01-22T12:58:01Z

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 <[email protected]>

marxin · 2022-01-22T15:18:14Z

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 · 2022-01-23T11:27:48Z

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

pradyunsg · 2022-01-25T00:48:09Z

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

marxin · 2022-01-25T08:55:26Z

Yes, please merge it.

tk0miya · 2022-01-30T15:11:41Z

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

marxin force-pushed the new-shortcuts branch from ab27cec to 8c2515a Compare August 10, 2021 06:46

tk0miya added builder:html type:proposal a feature suggestion labels Aug 10, 2021

tk0miya added this to the 4.2.0 milestone Aug 10, 2021

mgeier reviewed Aug 10, 2021

View reviewed changes

sphinx/themes/basic/static/doctools.js Outdated Show resolved Hide resolved

mgeier mentioned this pull request Aug 10, 2021

Proof of Concept: add "H" shortcut for hiding search matches #9539

Closed

marxin force-pushed the new-shortcuts branch from 8c2515a to 9f099e7 Compare August 11, 2021 07:38

marxin force-pushed the new-shortcuts branch from 9f099e7 to 7feb284 Compare August 11, 2021 07:45

marxin force-pushed the new-shortcuts branch from 7feb284 to e4e192a Compare August 11, 2021 08:33

tk0miya approved these changes Aug 11, 2021

View reviewed changes

mgeier reviewed Aug 11, 2021

View reviewed changes

sphinx/themes/basic/static/doctools.js Outdated Show resolved Hide resolved

marxin force-pushed the new-shortcuts branch from e4e192a to 98c3c16 Compare August 12, 2021 07:58

marxin force-pushed the new-shortcuts branch from 98c3c16 to 6f36876 Compare August 12, 2021 08:06

tk0miya modified the milestones: 4.4.0, 4.5.0 Jan 16, 2022

marxin force-pushed the new-shortcuts branch 2 times, most recently from 6c58f99 to b7eb90d Compare January 16, 2022 10:09

marxin marked this pull request as draft January 16, 2022 10:11

marxin force-pushed the new-shortcuts branch from b7eb90d to 160df9a Compare January 16, 2022 10:16

marxin force-pushed the new-shortcuts branch 3 times, most recently from 6e09439 to 38403ea Compare January 16, 2022 10:25

marxin changed the title ~~Implement new shortcuts:~~ Implement new search shortcuts. Jan 16, 2022

marxin marked this pull request as ready for review January 16, 2022 10:26

mgeier mentioned this pull request Jan 17, 2022

Call preventDefault() in "Esc" event listener readthedocs/readthedocs-sphinx-search#103

Open

marxin force-pushed the new-shortcuts branch from 38403ea to 69b9f51 Compare January 18, 2022 12:09

mgeier mentioned this pull request Jan 18, 2022

Add forward slash as hotkey for search UI readthedocs/readthedocs-sphinx-search#31

Merged

jakobandersen approved these changes Jan 22, 2022

View reviewed changes

Implement new shortcuts:

8cf3179

- "/" for Focus search bar - "ESC" - clear highlighted text Fixes sphinx-doc#691. Co-authored-by: Jakob Lykke Andersen <[email protected]>

marxin force-pushed the new-shortcuts branch from 69b9f51 to 8cf3179 Compare January 22, 2022 15:16

tk0miya merged commit 799385f into sphinx-doc:4.x Jan 30, 2022

github-actions bot locked as resolved and limited conversation to collaborators Mar 2, 2022

marxin deleted the new-shortcuts branch June 17, 2022 11:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement new search shortcuts. #9337

Implement new search shortcuts. #9337

marxin commented Jun 14, 2021 •

edited

Loading

mgeier commented Jun 15, 2021

marxin commented Jun 16, 2021

mgeier commented Jun 17, 2021

marxin commented Aug 9, 2021

TimKam commented Aug 9, 2021

marxin commented Aug 10, 2021

tk0miya commented Aug 10, 2021

tk0miya commented Aug 10, 2021

mgeier commented Aug 10, 2021

jakobandersen commented Aug 10, 2021 •

edited

Loading

marxin commented Aug 11, 2021

marxin commented Aug 11, 2021

marxin commented Aug 11, 2021

TimKam commented Aug 11, 2021

marxin commented Aug 11, 2021

mgeier commented Aug 11, 2021

marxin commented Aug 12, 2021

marxin commented Jan 16, 2022

mgeier commented Jan 17, 2022

marxin commented Jan 18, 2022

mgeier commented Jan 18, 2022

jakobandersen commented Jan 22, 2022

marxin commented Jan 22, 2022

tk0miya commented Jan 23, 2022

pradyunsg commented Jan 25, 2022

marxin commented Jan 25, 2022

tk0miya commented Jan 30, 2022

Implement new search shortcuts. #9337

Implement new search shortcuts. #9337

Conversation

marxin commented Jun 14, 2021 • edited Loading

mgeier commented Jun 15, 2021

marxin commented Jun 16, 2021

mgeier commented Jun 17, 2021

marxin commented Aug 9, 2021

TimKam commented Aug 9, 2021

marxin commented Aug 10, 2021

tk0miya commented Aug 10, 2021

tk0miya commented Aug 10, 2021

mgeier commented Aug 10, 2021

jakobandersen commented Aug 10, 2021 • edited Loading

marxin commented Aug 11, 2021

marxin commented Aug 11, 2021

marxin commented Aug 11, 2021

TimKam commented Aug 11, 2021

marxin commented Aug 11, 2021

mgeier commented Aug 11, 2021

marxin commented Aug 12, 2021

marxin commented Jan 16, 2022

mgeier commented Jan 17, 2022

marxin commented Jan 18, 2022

mgeier commented Jan 18, 2022

jakobandersen commented Jan 22, 2022

marxin commented Jan 22, 2022

tk0miya commented Jan 23, 2022

pradyunsg commented Jan 25, 2022

marxin commented Jan 25, 2022

tk0miya commented Jan 30, 2022

marxin commented Jun 14, 2021 •

edited

Loading

jakobandersen commented Aug 10, 2021 •

edited

Loading