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

Only make scrollable code blocks into tab stops #1777

Merged
merged 3 commits into from
Apr 24, 2024

Conversation

gabalafou
Copy link
Collaborator

@gabalafou gabalafou commented Apr 19, 2024

Context:

PR #1636 puts tabindex="0" on all code blocks. This PR limits it to code blocks that actually have overflowing, scrollable content.

How to test

This PR includes an automated test, which should run and pass with pytest -m "not a11y".

To manually test this PR, go to the preview build of the Kitchen Sink blocks page. With your browser wide (> 1440px), check that you can use the tab key on your keyboard to focus on the code blocks that have content that overflows (scrollable). It should be easy to tell that you are focused on the code block because it should have a purple focus ring and you should be able to use the left and right arrows to scroll the content. Now, make your browser window narrow so that some code blocks which previously did not have overflow now do. Double check that before you resized the window, you could not use the tab key to focus on the block, but now you can.

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

function setupLiteralBlockTabStops() {
const updateTabStops = () => {
document.querySelectorAll('[data-tabindex="0"]').forEach((el) => {
el.tabIndex = el.scrollWidth > el.clientWidth ? 0 : -1;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right now, I'm only checking for scrollable content in the x, or horizontal, direction because that's all I've ever seen. But do we know if we ever have code blocks that scroll in the y direction?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we do, or I don't see any reason to have some. The only thing I ce see that would be vertically scrollable would be inclusions like widgets/iframe from things like jupyter-lite-sphinx.

Copy link
Collaborator Author

@gabalafou gabalafou Apr 24, 2024

Choose a reason for hiding this comment

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

I decided to extend this line to check for y-overflow too. My reasoning is that the theme should work robustly—it would be fairly easy for some documentation site to create a class like .height-10em that they apply to code blocks.

@gabalafou gabalafou marked this pull request as ready for review April 19, 2024 16:20
Copy link
Collaborator

@Carreau Carreau left a comment

Choose a reason for hiding this comment

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

Looks good to me, I was wondering if resize event would also trigger when the windows is out of focus, but it does looks like they do.

function setupLiteralBlockTabStops() {
const updateTabStops = () => {
document.querySelectorAll('[data-tabindex="0"]').forEach((el) => {
el.tabIndex = el.scrollWidth > el.clientWidth ? 0 : -1;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we do, or I don't see any reason to have some. The only thing I ce see that would be vertically scrollable would be inclusions like widgets/iframe from things like jupyter-lite-sphinx.

@Carreau
Copy link
Collaborator

Carreau commented Apr 23, 2024

Side question for general reviewing of the theme/debugging.

Would it be good to have a mode/css where all items that can be keyboard focused have a border ? I tried [tabindex="0"] {border: solid red;} but it's not sufficient. Any thoughts.

@gabalafou
Copy link
Collaborator Author

Would it be good to have a mode/css where all items that can be keyboard focused have a border ? I tried [tabindex="0"] {border: solid red;} but it's not sufficient. Any thoughts.

Yeah, that won't work because, depending on your browser and OS configuration, some elements are automatically treated as tab stops—buttons, for example. And I don't know of any CSS pseudo-selectors for tab stops. You'd have to create some kind of long heuristic selector, like:

button, a, [tabindex]:not([tabindex="-1"]), input, textarea...

(Which wouldn't be truly accurate because some browsers—Safari—don't treat links as tab stops by default.)

You'd probably have to develop a browser extension, where you simulate pressing the tab key after the page is loaded, record all of the elements that were visited, then apply the border to those elements.

That might be what the Wave browser extension does under the hood. You can use it to show you the tab navigation order of the page. (It doesn't outline the elements, though; it overlays a number.)

Copy link
Collaborator

@drammock drammock left a comment

Choose a reason for hiding this comment

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

Comment on lines +711 to +719
function debounce(callback, wait) {
let timeoutId = null;
return (...args) => {
clearTimeout(timeoutId);
timeoutId = setTimeout(() => {
callback(...args);
}, wait);
};
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is so general-purpose I'm surprised there isn't a JS built-in for doing it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Heh, I had the same thought!

@drammock drammock merged commit a4eaf77 into pydata:main Apr 24, 2024
18 of 19 checks passed
@gabalafou gabalafou deleted the pre-data-attr-tabindex branch April 24, 2024 15:08
@gabalafou gabalafou added the tag: accessibility Issues related to accessibility issues or efforts label Apr 25, 2024
gabalafou added a commit to gabalafou/pydata-sphinx-theme that referenced this pull request Apr 29, 2024
* Only make scrollable code blocks into tab stops

* Check for y-overflow too
Carreau pushed a commit that referenced this pull request May 27, 2024
#1787)

One of many fixes for the failing accessibility tests (see #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 #1636 and #1777. 

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
ivanov pushed a commit to ivanov/pydata-sphinx-theme that referenced this pull request Jun 5, 2024
* Only make scrollable code blocks into tab stops

* Check for y-overflow too
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
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.

3 participants