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

Stop gobbling escape key #12813

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

gabalafou
Copy link

@gabalafou gabalafou commented Aug 22, 2024

Fixes #12811.

Subject: restore escape key functionality to HTML modal dialogs in Sphinx

Feature or Bugfix

  • Bugfix

Purpose

  • Restore default escape key functionality to modal dialogs, previously prevented by sphinx_highlight.js.

Detail

Relates

Copy link
Contributor

@jayaddison jayaddison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not convinced by this - sometimes the document.activeElement (focus) may be set to other document-level elements (body, for example) and it seems valid to clear the highlighting in those cases.

This is useful, for example, when searching for a term in some Sphinx-generated HTML documentation, clicking on a result, and then wanting to clear the highlighting without having taking any other action on the page.

@jayaddison jayaddison added html search javascript Pull requests that update Javascript code labels Sep 3, 2024
@gabalafou
Copy link
Author

@jayaddison, apologies, my wording isn't clear. When document.activeElement === document.body that essentially means that nothing on the page has focus. In other words, document.activeElement is never null, so if no other element is the active element, then the body is the active element. So, with the code changes introduced by this PR, if focus is on the body, the escape key should clear the search result highlights from the page.

I will update the PR description to match this more accurately.

That said, you raise a good question. In this PR, links (and document.body) are the only elements that while focused allow the escape key to clear the highlights. For any other focusable element, this PR assumes that it might have some default action for the escape key, so the escape key is ignored and the search highlights are not cleared. But might there be other exceptions? Are there other focusable elements that should allow the escape key to clear the highlights?

@gabalafou gabalafou changed the title Bail on escape key press if focus is on anything (other than a link) Bail on escape key if focus on something other than link or body Oct 21, 2024
@gabalafou gabalafou changed the title Bail on escape key if focus on something other than link or body Bail on escape key if focus on element other than link or body Oct 21, 2024
@gabalafou gabalafou changed the title Bail on escape key if focus on element other than link or body Only handle escape key on A and BODY Oct 21, 2024
CHANGES.rst Outdated Show resolved Hide resolved
@gabalafou gabalafou changed the title Only handle escape key on A and BODY Stop gobbling escape key Oct 21, 2024
CHANGES.rst Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
html search javascript Pull requests that update Javascript code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot press escape key to close modal dialog
2 participants