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 nbsphinx color outputs, suppress other notebook output Axe errors #1905

Merged
merged 2 commits into from
Jul 1, 2024

Conversation

gabalafou
Copy link
Collaborator

@gabalafou gabalafou commented Jun 27, 2024

Once this PR is merged, running the accessibility checks should execute successfully, with all known failures marked as expected. This should allow us to close #1428 and add the accessibility checks to CI.

@gabalafou
Copy link
Collaborator Author

gabalafou commented Jun 27, 2024

Submitting this as a draft until I can hunt down what seems to be some kind of race condition (I'm guessing race condition because sometimes the tests pass locally, sometimes not). Sometimes when running the tests locally, the "scollable-region-focusable" Axe check fails, which shouldn't happen because I am asking Playwright to wait for a sign from the script that adds tabindex=0.

@gabalafou
Copy link
Collaborator Author

Getting same confusing Axe failure in CI

@gabalafou gabalafou force-pushed the deal-with-notebook-outputs-axe branch from a66f1e3 to 7354230 Compare June 28, 2024 12:19
@gabalafou
Copy link
Collaborator Author

I think I found the cause of the race condition:

  1. page loads
  2. addTabStopsToScrollableElements() function runs
  3. ipywidgets load, triggering a debounced rerun of the addTabStopsToScrollableElements() function.
  • Note: at this point, the element containing the wide Pandas table in the ipywidget does NOT have tabindex="0"
  1. Playwright runs Axe
  2. The debounced call to addTabStopsToScrollableElements() finally runs and applies tabindex="0" after Axe has already run

With that order of operations, we get a failure for the scrollable-region-focusable Axe check. But if step 5 comes after 6, the check passes.

I rewrote the Playwright script to wait both for the ipywidget table's container to load and for tabindex="0" to be applied.

@gabalafou gabalafou marked this pull request as ready for review June 28, 2024 12:24
Copy link

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  src/pydata_sphinx_theme
  __init__.py
  logo.py
  short_link.py
  toctree.py
  translator.py
Project Total  

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

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.

self review

@@ -52,6 +52,28 @@ div.nblast.container {
margin-bottom: 1rem;
}

// Notebook cell input line number
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Suggested change
// Notebook cell input line number
// Notebook cell input prompt (execution count)
// Ref: https://nbformat.readthedocs.io/en/latest/format_description.html#code-cells

}
}

// Notebook cell output line number
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Suggested change
// Notebook cell output line number
// Notebook cell output prompt (execution count)
// Ref: https://nbformat.readthedocs.io/en/latest/format_description.html#code-cells

Comment on lines +209 to +213
# TODO: for color contrast issues with common notebook outputs
# (ipywidget tabbed panels, Xarray, etc.), should we override
# third-party CSS with our own CSS or/and work with NbSphinx, MyST-NB,
# ipywidgets, and other third parties to use higher contrast colors in
# their CSS?
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Larger conversation but longer term it would be best to have these fixed upstream vs in here.
Not sure how much of an effort this would be so will have to pin this for now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed

html[data-theme="dark"] & {
color: #ffa07a;
}
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

TODO: check MyST-NB notebook prompts for color contrast

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually MyST-NB does not render notebook cell prompts:

screenshot

@@ -38,8 +38,9 @@ def filter_ignored_violations(violations, url_pathname):
]:
filtered = []
for violation in violations:
# TODO: eventually fix this rule violation. See
# https://github.com/pydata/pydata-sphinx-theme/issues/1479.
# TODO: remove this exclusion once the following update to Axe is
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 change is unrelated to the rest of the PR, but I thought I would go ahead make this comment more up to date

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The CSS additions to this file override nbsphinx styles for the notebook cell prompts, which don't have sufficient color contrast against our light and dark backgrounds.

Nbsphinx uses a shade of blue for the input cell prompt and a shade of red for the output cell prompt. So I chose blueish and reddish colors from the following two accessible-pygments themes:

  1. a11y-high-contrast-light
  2. a11y-high-contrast-dark

TODO: add this as a code comment

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done, added comments to the code

@gabalafou gabalafou requested a review from trallard June 28, 2024 12:37
@trallard trallard added the tag: accessibility Issues related to accessibility issues or efforts label Jul 1, 2024
Copy link
Collaborator

@trallard trallard left a comment

Choose a reason for hiding this comment

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

Thanks @gabalafou this looks good, I checked through the axe/playwright tests and they look good so I will go ahead and merge

@@ -167,6 +168,13 @@ def test_axe_core(
# Wait for CSS transitions (Bootstrap's transitions are 300 ms)
page.wait_for_timeout(301)

# On the PyData Library Styles page, wait for ipywidget to load and for our
# JavaScript to apply tabindex="0" before running Axe checker (to avoid
# false positives for scrollable-region-focusable).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Interesting, I did not notice before that this was the case, i.e. the tabindex being applied later. I suppose it make sense since we are delegating this to JS.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, you might have seen my first tab stop implementation, which adds tabindex="0" at build time (via HTML created by Python), and might not have noticed my second tab stop implementation, which adds a zero tab index at run time (via DOM manipulation by JavaScript).

Fun fact. This change was driven by an article you shared (or maybe it was Smera) 🌻 Inclusive Components - Data Tables - Only Focusable Where Scrollable

Comment on lines +209 to +213
# TODO: for color contrast issues with common notebook outputs
# (ipywidget tabbed panels, Xarray, etc.), should we override
# third-party CSS with our own CSS or/and work with NbSphinx, MyST-NB,
# ipywidgets, and other third parties to use higher contrast colors in
# their CSS?
Copy link
Collaborator

Choose a reason for hiding this comment

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

Larger conversation but longer term it would be best to have these fixed upstream vs in here.
Not sure how much of an effort this would be so will have to pin this for now.

@trallard trallard merged commit 0acb754 into pydata:main Jul 1, 2024
29 checks passed
@gabalafou gabalafou deleted the deal-with-notebook-outputs-axe branch July 3, 2024 13:55
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.

Summary of Axe accessibility issues
2 participants