-
Notifications
You must be signed in to change notification settings - Fork 324
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
Fix keyboard access for scrollable regions created by notebook outputs #1787
Conversation
d3f6357
to
221d4ec
Compare
TODO:
|
- 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 | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this 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
if (document.readyState === "complete") { | ||
addTabStopsToScrollableElements(); | ||
} else { |
There was a problem hiding this comment.
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.
The failing check has to do with tokenless CodeCov rate limiting, so it's not related to this PR per se |
Coverage reportClick to see where and how coverage changed
This report was generated by python-coverage-comment-action |
tests/test_a11y.py
Outdated
@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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
resolved some conflicts. |
This addresses one of the issues in pydata#1740: missing horizontal scrollbar. - Add CSS rule to allow scrolling - Add ipywidgets example to the examples/pydata page
The "defusedxml" error means we can't actually see in the CI that the tests I added actually work, since I tagged them as |
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. |
That worked! Happy to see that CI PR merged (#1759) :) |
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
#1827 introduced an [Axe accessibility regression](https://dequeuniversity.com/rules/axe/4.0/scrollable-region-focusable). See also: - #1787 - #1428
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.
This also addresses one of the issues in #1740: missing horizontal scrollbar by: