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

Fix keyboard access for scrollable regions created by notebook outputs #1787

Merged
merged 20 commits into from
May 27, 2024

Conversation

gabalafou
Copy link
Collaborator

@gabalafou gabalafou commented Apr 25, 2024

One of many fixes for the failing accessibility tests (see #1428).

The accessibility tests were still reporting some violations of:

These were due to Jupyter notebook outputs that have scrollable content. This PR extends the functionality of PRs #1636 and #1777 to such outputs.

  • Adds a test for tabindex = 0 on notebook outputs after page load

This also addresses one of the issues in #1740: missing horizontal scrollbar by:

  • Adding CSS rule to allow scrolling
  • Add ipywidgets example to the examples/pydata page

@gabalafou gabalafou force-pushed the fix-scrollable-notebook-outputs branch from d3f6357 to 221d4ec Compare April 25, 2024 16:48
@gabalafou
Copy link
Collaborator Author

gabalafou commented Apr 25, 2024

TODO:

  • add test for tabindex = 0 on notebook outputs after page load

Comment on lines -30 to -55
- 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["data-tabindex"] = "0"

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:
# 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.
html_string = self.body[-1]
self.body[-1] = html_string.replace("<pre", '<pre data-tabindex="0"')
raise nodes.SkipNode

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 realized as I was working on this that this deleted code was essentially trying to put a data-tabindex attribute on every <pre> tag. So, in the JavaScript file, instead of iterating through elements with data-tabindex, which is either the same or a sub set of all the pre elements on the page, we can simplify and iterate through all the pre elements instead.

In other words, it only makes sense to keep this code if we add the tabindex attribute at build time. But since we're not doing that anymore (as of #1777), we might as well delete this code, and just check all pre elements on page load.

Copy link
Collaborator Author

@gabalafou gabalafou left a comment

Choose a reason for hiding this comment

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

done self-reviewing, going to mark this as ready for review

Comment on lines 748 to 750
if (document.readyState === "complete") {
addTabStopsToScrollableElements();
} else {
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 had to add the document.readyState check in order for the Playwright tests to pass. It may or may not be necessary in other browsers, but it makes sense to keep it as a bit of defensive programming.

@gabalafou gabalafou marked this pull request as ready for review April 26, 2024 12:15
@gabalafou
Copy link
Collaborator Author

gabalafou commented Apr 26, 2024

The failing check has to do with tokenless CodeCov rate limiting, so it's not related to this PR per se

@trallard trallard added the tag: accessibility Issues related to accessibility issues or efforts label May 8, 2024
Copy link

github-actions bot commented May 21, 2024

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  src/pydata_sphinx_theme
  translator.py
Project Total  

This report was generated by python-coverage-comment-action

Comment on lines 284 to 303
@pytest.mark.fail(reason="fail until #1760 is merged")
def test_notebook_output_tab_stop_1760(page: Page, url_base: str) -> None:
"""# TODO: this was part of test_notebook_output_tab_stop.

I is now separated into it's own failing test until #1760 is merged.
"""
page.goto(urljoin(url_base, "/examples/pydata.html"))

# An ipywidget notebook output
ipywidget = page.locator("css=.jp-RenderedHTMLCommon").first

# As soon as the ipywidget is attached to the page it should trigger the
# mutation observer, which has a 300 ms debounce
ipywidget.wait_for(state="attached")
page.wait_for_timeout(301)

# At the default viewport size (1280 x 720) the data table inside the
# ipywidget has overflow
assert ipywidget.evaluate("el => el.scrollWidth > el.clientWidth") is True
assert ipywidget.evaluate("el => el.tabIndex") == 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

I made this a separate test with @pytest.mark.fail in order to not forget to uncomment it when working on #1760

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not familiar with fail mark, is it an alias for xfail? I would have expected maybe pytest.mark.xfail(strict=True) to make sure we remember to remove it after #1760 is in

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah, let's do xfail(strict=True), it's clearer.

tests/test_a11y.py Outdated Show resolved Hide resolved
tests/test_a11y.py Outdated Show resolved Hide resolved
tests/test_a11y.py Outdated Show resolved Hide resolved
tests/test_a11y.py Outdated Show resolved Hide resolved
tests/test_a11y.py Show resolved Hide resolved
tests/test_a11y.py Outdated Show resolved Hide resolved
@Carreau
Copy link
Collaborator

Carreau commented May 23, 2024

resolved some conflicts.

@gabalafou
Copy link
Collaborator Author

The "defusedxml" error means we can't actually see in the CI that the tests I added actually work, since I tagged them as a11y and for some reason that test path is triggering that error (but I think there's some setting on the step that makes the failure not appear at first glance in the workflow run report)

@trallard
Copy link
Collaborator

Hmmm this error should not be present anymore. But since we have the CI to pass even if we have a11y errors this means this is getting obscured.

Can you merge main on your branch to see if the recently merged CI changes fixes it? If not I can add a fix.

@gabalafou
Copy link
Collaborator Author

That worked! Happy to see that CI PR merged (#1759) :)

@Carreau Carreau merged commit c839490 into pydata:main May 27, 2024
30 checks passed
@gabalafou gabalafou deleted the fix-scrollable-notebook-outputs branch May 27, 2024 08:46
ivanov pushed a commit to ivanov/pydata-sphinx-theme that referenced this pull request Jun 5, 2024
pydata#1787)

One of many fixes for the failing accessibility tests (see pydata#1428).

The accessibility tests were still reporting some violations of: 
 - Scrollable region must have keyboard access (https://dequeuniversity.com/rules/axe/4.8/scrollable-region-focusable) even after merging pydata#1636 and pydata#1777. 

These were due to Jupyter notebook outputs that have scrollable content. This PR extends the functionality of PRs pydata#1636 and pydata#1777 to such outputs.

- Adds a test for tabindex = 0 on notebook outputs after page load

This also addresses one of the issues in pydata#1740: missing horizontal scrollbar by:

- Adding CSS rule to allow scrolling
- Add ipywidgets example to the examples/pydata page
Carreau pushed a commit that referenced this pull request Jun 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tag: accessibility Issues related to accessibility issues or efforts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants