Skip to content

Comments

fix: swev-id: sphinx-doc__sphinx-8621 handle :kbd: separator splits#63

Open
casey-brooks wants to merge 2 commits intosphinx-doc__sphinx-8621from
work/kbd-separator-fix-8621
Open

fix: swev-id: sphinx-doc__sphinx-8621 handle :kbd: separator splits#63
casey-brooks wants to merge 2 commits intosphinx-doc__sphinx-8621from
work/kbd-separator-fix-8621

Conversation

@casey-brooks
Copy link

@casey-brooks casey-brooks commented Dec 26, 2025

Request: Fix :kbd: role rendering for separator keys in HTML

Related Issue: #62

Summary

  • Correct handling of '-', '+', '^' when used as literal keys and within compound keystrokes.
  • Updated HTML KeyboardTransform tokenization to only split on true separators between non-whitespace characters; added escape-aware masking.

Observed failure (before fix)
Examples:

  1. :kbd:-
    Produced:
    -
  2. :kbd:+
    Produced:
    +
  3. :kbd:Shift-+
    Produced:
    Shift-+

Stack trace

  • None. This is a rendering correctness issue; no exceptions are raised.

Reproduction steps

  1. Create a minimal Sphinx project with index.rst containing:

.. code-block:: rst

Examples:

(1) :kbd:-
(2) :kbd:+
(3) :kbd:Shift-+

  1. Build HTML:

.. code-block:: bash

sphinx-build -b html

  1. Inspect the generated HTML for the above lines. Prior to this fix, the output shows empty children around separator characters as in “Observed failure”.

Verification (after fix)

  • The above examples now render as:
    • :kbd:--
    • :kbd:++
    • :kbd:^^
    • :kbd:Shift-+Shift-+

Local test summary

  • Pytest selection in tests/test_markup.py for kbd cases: 13 passed.
  • Manual build of demo page confirmed expected HTML structure.

Notes

  • CI was not run per task instructions.
  • Base branch: sphinx-doc__sphinx-8621; this PR does not modify that branch directly.

@casey-brooks casey-brooks requested a review from a team December 26, 2025 17:59
@casey-brooks
Copy link
Author

Tests & Lint Summary

  • PYTHONPATH=/workspace/sphinx .venv/bin/pytest tests/test_markup.py -k ':kbd:' -q → 12 passed, 30 deselected
  • PYTHONPATH=/workspace/sphinx .venv/bin/sphinx-build -b html tmp/kbd-demo tmp/kbd-demo/_build/html → build succeeded (emits existing 'meta' registration warning)

Copy link

@noa-lucent noa-lucent left a comment

Choose a reason for hiding this comment

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

Thanks for the updates! I found an issue with the escape handling in that causes escaped separators to start splitting again as soon as a different escape appears earlier in the literal (see inline). Please address that and add coverage for the scenario before we merge.

@casey-brooks
Copy link
Author

Updated Tests & Lint Summary

  • ............. [100%]
    =============================== warnings summary ===============================
    sphinx/util/docutils.py:45
    /workspace/sphinx/sphinx/util/docutils.py:45: DeprecationWarning: distutils Version classes are deprecated. Use packaging.version instead.
    version_info = tuple(LooseVersion(docutils.version).version)

sphinx/registry.py:22
/workspace/sphinx/sphinx/registry.py:22: DeprecationWarning: pkg_resources is deprecated as an API. See https://setuptools.pypa.io/en/latest/pkg_resources.html
from pkg_resources import iter_entry_points

.venv/lib/python3.11/site-packages/pkg_resources/init.py:3147
.venv/lib/python3.11/site-packages/pkg_resources/init.py:3147
.venv/lib/python3.11/site-packages/pkg_resources/init.py:3147
.venv/lib/python3.11/site-packages/pkg_resources/init.py:3147
.venv/lib/python3.11/site-packages/pkg_resources/init.py:3147
/workspace/sphinx/.venv/lib/python3.11/site-packages/pkg_resources/init.py:3147: DeprecationWarning: Deprecated call to pkg_resources.declare_namespace('sphinxcontrib').
Implementing implicit namespace packages (as specified in PEP 420) is preferred to pkg_resources.declare_namespace. See https://setuptools.pypa.io/en/latest/references/keywords.html#keyword-namespace-packages
declare_namespace(pkg)

sphinx/directives/patches.py:14
/workspace/sphinx/sphinx/directives/patches.py:14: DeprecationWarning: The docutils.parsers.rst.directive.html module will be removed in Docutils 2.0. Since Docutils 0.18, the "Meta" node is defined in docutils.parsers.rst.directives.misc.
from docutils.parsers.rst.directives import html, images, tables

sphinx/util/rst.py:55
/workspace/sphinx/sphinx/util/rst.py:55: DeprecationWarning: 'environmentfilter' is renamed to 'pass_environment', the old name will be removed in Jinja 3.1.
@environmentfilter

sphinx/util/images.py:12
/workspace/sphinx/sphinx/util/images.py:12: DeprecationWarning: 'imghdr' is deprecated and slated for removal in Python 3.13
import imghdr

sphinx/jinja2glue.py:105
/workspace/sphinx/sphinx/jinja2glue.py:105: DeprecationWarning: 'contextfunction' is renamed to 'pass_context', the old name will be removed in Jinja 3.1.
@contextfunction

tests/test_markup.py: 13 warnings
/workspace/sphinx/sphinx/jinja2glue.py:184: DeprecationWarning: 'contextfunction' is renamed to 'pass_context', the old name will be removed in Jinja 3.1.
self.environment.globals['debug'] = contextfunction(pformat)

tests/test_markup.py: 13 warnings
/workspace/sphinx/sphinx/jinja2glue.py:186: DeprecationWarning: 'contextfunction' is renamed to 'pass_context', the old name will be removed in Jinja 3.1.
self.environment.globals['accesskey'] = contextfunction(accesskey)

tests/test_markup.py: 13 warnings
/workspace/sphinx/tests/test_markup.py:32: DeprecationWarning: The frontend.OptionParser class will be replaced by a subclass of argparse.ArgumentParser in Docutils 2.0 or later.
To get default settings, use frontend.get_default_settings().
optparser = frontend.OptionParser(

tests/test_markup.py: 1235 warnings
/root/.nix-profile/lib/python3.11/optparse.py:1000: DeprecationWarning: The frontend.Option class will be removed in Docutils 2.0 or later.
option = self.option_class(*args, **kwargs)

tests/test_markup.py: 16 warnings
/workspace/sphinx/tests/test_markup.py:70: DeprecationWarning: nodes.Node.traverse() is obsoleted by Node.findall().
for msg in document.traverse(nodes.system_message):

tests/test_markup.py: 13 warnings
/workspace/sphinx/sphinx/builders/html/transforms.py:44: DeprecationWarning: nodes.Node.traverse() is obsoleted by Node.findall().
for node in self.document.traverse(matcher): # type: nodes.literal

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
13 passed, 30 deselected, 1314 warnings in 0.19s → 13 passed, 30 deselected

  • → build succeeded (expected existing 'meta' registration warning)

Copy link

@noa-lucent noa-lucent left a comment

Choose a reason for hiding this comment

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

Looks great—thanks for tightening up the escape handling and covering it with tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants