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

Improve accessibility of navbar and footer #44

Merged
merged 17 commits into from
Oct 12, 2023

Conversation

Mandrenkov
Copy link
Collaborator

@Mandrenkov Mandrenkov commented Oct 6, 2023

Context:

There were a number of accessibility issues introduced in v0.4.0 of the XST, including:

  • Missing alt text for navbar images.
  • Missing ARIA labels for <a> and <button> elements.
  • Missing ENTER and ECS controls for menu buttons.
  • Focus is not trapped within the expanded navbar menu on mobile.

Description of the Change:

This PR addresses all of the issues in the context. It also introduces:

  1. An optional navbar_logo_alt theme option to control the alternative text in the navbar.
  2. A mandatory name field to the social icons in the footer to populate ARIA labels.

The version number is also bumped to v0.5.0 to account for (2) above.

Benefits:

  • The XST footer and navbar are more accessible to those using keyboard navigation or screen readers.

Possible Drawbacks:

None.

Related GitHub Issues:

None.


Previews:

Copy link
Contributor

@LucasSilbernagel LucasSilbernagel left a comment

Choose a reason for hiding this comment

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

Excellent work @Mandrenkov ! The accessibility of the navbar and footer is much improved. I only noticed two remaining screen reader issues:

  • For the toggle buttons to open/close the navbar submenus on desktop, the screen reader correctly reads the aria-label and then says "To click this button, press CONTROL OPTION SPACE", which is normal behaviour. However, pressing CONTROL OPTION SPACE in this case does nothing for some reason. Pressing ENTER does work to toggle these buttons, but they should also work the way the screen reader expects.
  • Nice working trapping keyboard focus in the mobile menu! Unfortunately screen reader focus is not trapped, so that should be fixed if possible.

@josh146
Copy link
Member

josh146 commented Oct 10, 2023

Hey @Mandrenkov, the navbar no longer works for me on Firefox with this change (can't access the dropdown menus)

@Mandrenkov
Copy link
Collaborator Author

Thanks for letting me know, @josh146!

Hey @Mandrenkov, the navbar no longer works for me on Firefox with this change (can't access the dropdown menus)

Can you clarify what OS you're using and whether the dropdown menu issue is specific to desktop or mobile?

@josh146
Copy link
Member

josh146 commented Oct 10, 2023

This is on a Mac M1, and with desktop 🤔 Hovering over the navbar, and clicking on the down arrows, isn't doing anything. I tested on Chrome, and everything works

@Mandrenkov
Copy link
Collaborator Author

The issue should be fixed now, @josh146! It turns out ariaLabel is not yet supported in Firefox.

@Mandrenkov
Copy link
Collaborator Author

Thanks for the review, @LucasSilbernagel !

  • For the toggle buttons to open/close the navbar submenus on desktop, the screen reader correctly reads the aria-label and then says "To click this button, press CONTROL OPTION SPACE", which is normal behaviour. However, pressing CONTROL OPTION SPACE in this case does nothing for some reason. Pressing ENTER does work to toggle these buttons, but they should also work the way the screen reader expects.

Nice catch! I was able to fix this by adding a click event listener to the navbar dropdown buttons.

  • Nice working trapping keyboard focus in the mobile menu! Unfortunately screen reader focus is not trapped, so that should be fixed if possible.

I'm not sure how to reproduce this issue: using TAB and SHIFT + TAB with VoiceOver enabled seems to work as expected. Do you mind providing more specific instructions for reproducing the problem?

@LucasSilbernagel
Copy link
Contributor

  • Nice working trapping keyboard focus in the mobile menu! Unfortunately screen reader focus is not trapped, so that should be fixed if possible.

I'm not sure how to reproduce this issue: using TAB and SHIFT + TAB with VoiceOver enabled seems to work as expected. Do you mind providing more specific instructions for reproducing the problem?

@Mandrenkov I don't know which screen reader you're using, but with VoiceOver on Mac, moving between elements is usually done by holding down Ctrl + Option and then navigating with the left and right arrow keys. The reason for this is that Tab navigation only focuses on interactive elements like buttons and links, so it skips over readable content like paragraphs.

@Mandrenkov
Copy link
Collaborator Author

Thanks for the clarification, @LucasSilbernagel! I was able to achieve the desired result by adding arria-hidden attributes to the content which is covered by the mobile navbar dropdown.

Copy link
Contributor

@LucasSilbernagel LucasSilbernagel left a comment

Choose a reason for hiding this comment

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

Thanks for making those changes @Mandrenkov ! I just noticed one new issue (in the Xanadu Sphinx Theme preview): when viewing the navbar on desktop and navigating with my keyboard, if I focus on a submenu toggle button and press the Enter key, the menu opens and then closes right away.

@Mandrenkov
Copy link
Collaborator Author

Amazing catch, @LucasSilbernagel! I'm not sure how I didn't notice that it broke. 😅 Should be fixed now!

Copy link
Contributor

@LucasSilbernagel LucasSilbernagel left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @Mandrenkov !

@Mandrenkov Mandrenkov merged commit 1f7d81e into master Oct 12, 2023
2 checks passed
@Mandrenkov Mandrenkov deleted the sc-46517-accessibility-improvements branch October 12, 2023 14:18
Mandrenkov added a commit to PennyLaneAI/pennylane-sphinx-theme that referenced this pull request Oct 13, 2023
**Context:**

A recent [XST PR](XanaduAI/xanadu-sphinx-theme#44) introduced a breaking change to the theme configuration.

**Description of the Change:**

This PR upgrades the XST version to v0.5.0 and patches any breaking changes. It also adds a **Discord** link to the footer.

**Benefits:**

* See the [v0.5.0](https://github.com/XanaduAI/xanadu-sphinx-theme/releases/tag/v0.5.0) release notes of the XST.

**Possible Drawbacks:**

None.

**Related GitHub Issues:**

None.

---

**Preview:**
https://xanaduai-pennylane--40.com.readthedocs.build/projects/sphinx-theme/en/40/
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants