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: accessibility of the mobile nav bar #7427

Merged
merged 9 commits into from
Feb 1, 2025

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Jan 26, 2025

Description

Validation

Navigate to the website with VoiceOver (or TalkBack, or another screen reader) enabled, try to use the navigation menu.

Related Issues

Fixes: #7423

Check List

  • I have read the Contributing Guidelines and made commit messages that follow the guideline.
  • I have run npm run format to ensure the code follows the style guide.
  • I have run npm run test to check if all tests are passing.
  • I have run npx turbo build to check if the website builds without errors.
  • I've covered new added functionality with unit tests if necessary.

@aduh95 aduh95 requested a review from a team as a code owner January 26, 2025 18:07
Copy link

vercel bot commented Jan 26, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
nodejs-org ✅ Ready (Inspect) Visit Preview Jan 28, 2025 9:47pm

packages/i18n/locales/en.json Outdated Show resolved Hide resolved
Copy link
Member

@AugustinMauroy AugustinMauroy left a comment

Choose a reason for hiding this comment

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

LGMT !

Copy link
Contributor

@shanpriyan shanpriyan left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@canerakdas canerakdas left a comment

Choose a reason for hiding this comment

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

When I try it on Safari with VoiceOver on my phone, the Node.js logo is not accessible. Applying the SVG pattern 5 found in the following link seems like a more suitable scenario; https://www.deque.com/blog/creating-accessible-svgs/

@aduh95
Copy link
Contributor Author

aduh95 commented Feb 1, 2025

When I try it on Safari with VoiceOver on my phone, the Node.js logo is not accessible. Applying the SVG pattern 5 found in the following link seems like a more suitable scenario; https://www.deque.com/blog/creating-accessible-svgs/

I've tried with iOS 18.3 and VoiceOver lets me select the logo, and enabling it does bring me back to the home page. Could you share more details please?

Copy link
Contributor

github-actions bot commented Feb 1, 2025

Lighthouse Results

URL Performance Accessibility Best Practices SEO Report
/en 🔴 67 🟢 100 🟢 100 🟢 91 🔗
/en/about 🟢 100 🟢 100 🟢 100 🟢 91 🔗
/en/about/previous-releases 🟢 99 🟢 100 🟢 100 🟢 92 🔗
/en/download 🟢 98 🟢 100 🟢 100 🟢 91 🔗
/en/blog 🟢 100 🟢 100 🟢 96 🟢 92 🔗

@ovflowd
Copy link
Member

ovflowd commented Feb 1, 2025

I believe we can proceed with this PR as is, and then iterate over if needed regarding the VoiceOver issue and possible Radix component from the Input element.

@ovflowd ovflowd enabled auto-merge February 1, 2025 20:13
Copy link
Contributor

github-actions bot commented Feb 1, 2025

Unit Test Coverage Report

Lines Statements Branches Functions
Coverage: 90%
88.71% (739/833) 75.94% (240/316) 87.65% (142/162)

Unit Test Report

Tests Skipped Failures Errors Time
182 0 💤 0 ❌ 0 🔥 5.889s ⏱️

@ovflowd ovflowd added this pull request to the merge queue Feb 1, 2025
Merged via the queue into nodejs:main with commit 317dce6 Feb 1, 2025
13 of 16 checks passed
@canerakdas
Copy link
Member

As far as I checked, VoiceOver tries to read all the svg's we use because we did not specify the role as image. I will open a PR to fix these 🤔

ScreenRecording_02-01-2025.23-18-25_1.mov

I learned something new, thank you for your contribution @aduh95

@aduh95 aduh95 deleted the fix-mobile-accessibility branch February 1, 2025 21:55
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.

Accessing the mobile menu is impossible when using VoiceOver
5 participants