sphinx-doc / sphinx

The Sphinx documentation generator
https://www.sphinx-doc.org/
Other
6.56k stars 2.12k forks source link

Cannot press escape key to close modal dialog #12811

Open gabalafou opened 2 months ago

gabalafou commented 2 months ago

Describe the bug

If you add an HTML modal dialog to a website built with Sphinx, and you have search enabled, then when the modal is shown, the escape key is broken, meaning you should be able to close the modal dialog by pressing the escape key, but you can't

This is because sphinx_highlight.js calls event.preventDefault() on line 142 when the escape key is pressed.

Steps to verify this explanation. Follow the steps to reproduce (below). Then open Chrome Dev Tools. Go to the Sources tab, expand Event Listener Breakpoints, expand Keyboard, and check the keydown checkbox. Then click on the modal dialog and press Escape. Use the debugger to step through the code until you get to the event.preventDefault() on line 142 of sphinx_highlight.js.

The easy fix would be to add "DIALOG" to the BLACKLISTED_KEY_CONTROL_ELEMENTS set. However, I think the appearance of this bug suggests that the current implementation could be made more robust.

I'm not quite sure I totally understand the logic of not handling the escape key when focus is on a textarea, input, select, or button, which is the current set of elements covered by BLACKLISTED_KEY_CONTROL_ELEMENTS. I get that, with a select, the escape key is meant to close it. But with a button, is there any expected default behavior for the escape key? In other words, if buttons are excluded, why not also links or any other kind of focusable element?

Which makes me wonder if a safer and just as effective solution would be to bail if document.activeElement is anything other than document.body or null.

How to Reproduce

Add a native HTML modal dialog to a Sphinx site. You do this by adding a <dialog> tag and opening it with .showModal(). For example, you could extend layout.html, by creating the following file at docs/source/_templates/layout.html:

{% extends "!layout.html" %}
{% block document %}
    <dialog id="dialog">Hello, world</dialog>
    <script defer>
        const dialog = document.getElementById("dialog");
        dialog.showModal();
    </script>
    {{ super() }}
{% endblock %}

Steps:

  1. Load the index page, the modal dialog should open automatically, centered over the page.
  2. Press the escape key.
  3. Observe that nothing happens, even though it should close the modal dialog.

Environment Information

Platform:              darwin; (macOS-14.6.1-arm64-arm-64bit)
Python version:        3.10.10 | packaged by conda-forge | (main, Mar 24 2023, 20:12:31) [Clang 14.0.6 ])
Python implementation: CPython
Sphinx version:        7.2.6
Docutils version:      0.20.1
Jinja2 version:        3.1.2
Pygments version:      2.17.2

Sphinx extensions

extensions = []

Additional context

This bug surfaced while working on a pull request in another GitHub repo (https://github.com/pydata/pydata-sphinx-theme/pull/1942). To get around this bug, we had to manually reconnect the escape key to the modal dialog.

jayaddison commented 2 months ago

I'm not quite sure I totally understand the logic of not handling the escape key when focus is on a textarea, input, select, or button, which is the current set of elements covered by BLACKLISTED_KEY_CONTROL_ELEMENTS. I get that, with a select, the escape key is meant to close it. But with a button, is there any expected default behavior for the escape key? In other words, if buttons are excluded, why not also links or any other kind of focusable element?

This seems likely to be accessibility-related: a mechanism to reduce conflict with input assistance technologies that might use the escape key as a modifier or menu control. That said, I don't know for certain that that is why it was implemented.