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

BUG: fix highlighting of current version in switcher menu #1357

Merged
merged 11 commits into from
Jul 3, 2023

Conversation

drammock
Copy link
Collaborator

@drammock drammock commented Jun 20, 2023

closes #1128
(supersedes) closes #1305

#1344 ended up implementing most of #1305; this PR implements the remainder of #1305 (the new tests and some tweaks to the switcher ARIA roles). I gave co-author credit to @jpfeuffer here (really should have been done in #1344 but I failed to do it there, oops).

This PR also:

This PR doesn't do everything to fix accessibility of the version switcher; I've opened a separate issue to remind us to tackle that.

@drammock drammock added kind: bug Something isn't working tag: accessibility Issues related to accessibility issues or efforts kind: maintenance Improving maintainability and reducing technical debt impact: block-release Should block a release from happening. Only use if this is a critical problem we don't want to ship labels Jun 20, 2023
tests/test_build.py Outdated Show resolved Hide resolved
tests/test_build.py Outdated Show resolved Hide resolved
@drammock
Copy link
Collaborator Author

OK @12rambau CIs pass and this one should be ready to go. Highlighting of the active version works https://pydata-sphinx-theme--1357.org.readthedocs.build/en/1357/ and is being tested now (though note that our CIs currently don't "fail" when there are a11y test failures, but the CI log shows the switcher highlighting test passing (it's the last one in the a11y suite):

tests/test_a11y.py ..FFFFFF..FF........                                  [100%]

@drammock drammock changed the title Version switcher tests BUG: fix highlighting of current version in switcher menu Jun 23, 2023
@trallard
Copy link
Collaborator

I am away from my computer to do a thorough review. But at a quick glance I noticed this PR reverts this back to a menu (see 544f6ba where it was changed to a listbox).

Also from that previous PR (and I was reminded again this week as I was doing a quick keyboard navigation audit) that the version switcher focus is hidden right now - so need to swap the tabindex=-1 to 0

drammock and others added 2 commits June 26, 2023 10:38
@drammock
Copy link
Collaborator Author

I am away from my computer to do a thorough review. But at a quick glance I noticed this PR reverts this back to a menu (see 544f6ba where it was changed to a listbox).

Also from that previous PR (and I was reminded again this week as I was doing a quick keyboard navigation audit) that the version switcher focus is hidden right now - so need to swap the tabindex=-1 to 0

thanks for the notes @trallard, I must have been working from an outdated local copy of #1305. I've changed this PR to listbox and tab index 0 now.

@drammock
Copy link
Collaborator Author

this one is ready to go whenever someone has a moment to review/merge.

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 @drammock I finally had a chance to look into this PR.

Looks good except for a minor change needed. I tested on a separate branch w/main merged so that I could see the focus-visible when doing the tests, and all looks pretty good.

As a summary, this is how the keyboard navigation works now:

When the dropdown is collapsed

  • TAB: place focus on the version selector
  • Space and Enter opens the options list (it does not highlight the current list as this is handled separately as of now

With the dropdown expanded:

  • TAB move through options, changing selection at the same time (to move backwards Shift + TAB)
  • Escape closes the list, without applying the highlighted option
  • Enter closes the list and applies the highlighted option
  • Space does nothing

It would be nice to be able to use Up and Down arrow, but the current approach is usable through keyboard and voice control so we could merge as is

💡 I suggested removing the tabindex as I noticed this was selecting the whole list right after opening the option list which would require one to press Escape then start the navigation

@drammock drammock merged commit 5e6cf0b into pydata:main Jul 3, 2023
@drammock drammock deleted the version-switcher-tests branch July 3, 2023 15:57
QuLogic added a commit to matplotlib/matplotlib.github.com that referenced this pull request Aug 2, 2024
This was broken in older pydata-sphinx-theme, cf
pydata/pydata-sphinx-theme#1357
QuLogic added a commit to matplotlib/matplotlib.github.com that referenced this pull request Aug 2, 2024
This was broken in older pydata-sphinx-theme, cf
pydata/pydata-sphinx-theme#1357
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impact: block-release Should block a release from happening. Only use if this is a critical problem we don't want to ship kind: bug Something isn't working kind: maintenance Improving maintainability and reducing technical debt tag: accessibility Issues related to accessibility issues or efforts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Coloring the version switcher button does not work as expected
3 participants