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.

Sign up for GitHub

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

Jump to bottom

Ensure code blocks (<pre>) are keyboard focusable #1636

Merged
merged 3 commits into from
Apr 5, 2024
Merged

Ensure code blocks (<pre>) are keyboard focusable #1636

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a0eafea
Ensure code blocks (<pre>) are keyboard focusable
gabalafou Jan 8, 2024
de64db1
Also cover <pre> tags not covered by starttag
gabalafou Jan 8, 2024
6b6faae
Update src/pydata_sphinx_theme/translator.py
trallard Apr 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 26 additions & 1 deletion src/pydata_sphinx_theme/translator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import types

import sphinx
from docutils import nodes
from packaging.version import Version
from sphinx.application import Sphinx
from sphinx.ext.autosummary import autosummary_table
Expand All @@ -23,11 +24,35 @@
self.settings.table_style = "table"

def starttag(self, *args, **kwargs):
"""Ensure an aria-level is set for any heading role."""
"""Perform small modifications to tags.

- ensure aria-level is set for any tag with heading role
- ensure <pre> tags have tabindex="0".
"""
if kwargs.get("ROLE") == "heading" and "ARIA-LEVEL" not in kwargs:
kwargs["ARIA-LEVEL"] = "2"

if "pre" in args:
kwargs["tabindex"] = "0"

Check warning on line 36 in src/pydata_sphinx_theme/translator.py

View check run for this annotation

Codecov / codecov/patch

src/pydata_sphinx_theme/translator.py#L36 

Added line #L36 was not covered by tests
trallard marked this conversation as resolved.
Show resolved Hide resolved

return super().starttag(*args, **kwargs)

def visit_literal_block(self, node):
"""Modify literal blocks.

- add tabindex="0" to <pre> tags within the HTML tree of the literal
block
"""
try:
super().visit_literal_block(node)
except nodes.SkipNode:
trallard marked this conversation as resolved.
Show resolved Hide resolved
# If the super method raises nodes.SkipNode, then we know it
# executed successfully and appended to self.body a string of HTML
# representing the code block, which we then modify.
Comment on lines +49 to +51
Copy link
Collaborator

Choose a reason for hiding this comment

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

so... docutils raises an exception when the method executes successfully? What happens when it fails? 😅

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it would raise a different exception but I'm not sure.

I think it has something to do with the way that docutils traverses the doc tree, see docutils/nodes.py::Node.walkabout()

html_string = self.body[-1]
self.body[-1] = html_string.replace("<pre", '<pre tabindex="0"')
Comment on lines +52 to +53
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is kinda dirty but... it seems to work 🤷

I'll leave it up to the reviewers to decide if it's too dirty

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am having a bit of trouble following here - is the goal of these to find and traverse any hidden/nested pretags then add the tabindex?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Exactly. When the parent class's visit_literal_block method is called, it appends an HTML string to self.body. So the very last item in the self.body list is just an HTML string containing a bunch of markup that represents the literal block. A lot of the tags in that markup are there to provide syntax highlighting, line numbers, captions, etc. The relevant tags inside that markup are the pre tags, although I would guess there's only ever one pre tag. However, I add tabindex="0" to any pre tag I find because the nature of a pre tag is that it preserves the whitespace, which means it might be scrollable (depending on how long the lines inside of it are) and should therefore be keyboard-focusable.

Copy link
Collaborator

Choose a reason for hiding this comment

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

this doesn't seem that "dirty" to me.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ditto not dirty just working with what we have

Copy link
Collaborator Author

@gabalafou gabalafou Apr 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I read something since working on this PR that makes me wonder if this is the right thing to do. The thing I read was about tables but the same could be said about pre-blocks:

Of course, we don't want to make the table container focusable unless its contents overflow. Otherwise we're adding a tab stop to the focus order which doesn't do anything. In my opinion, that would be a fail under 2.4.3 Focus Order. Giving keyboard users elements to focus which don't actually do anything is confusing and obstructive.
Source: Inclusive Components - Data Tables - Only Focusable Where Scrollable

Which makes me wonder if we should put a data attribute on the pre-tag and then check it with JavaScript after it loads to determine if it should really be a tab stop or not.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Follow-up:

raise nodes.SkipNode

def visit_table(self, node):
"""Custom visit table method.

Expand Down
Loading