init: add skip-to-main-content shortcut #7390

yaten2302 · 2025-01-07T06:57:02Z

Description

This PR adds a skip to main content button (initially hidden), when the user presses Tab(for windows), the button is visible and the user can then move directly to the main content of the page.

Related Issues

Fixes #7220

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.

Signed-off-by: Yaten Dhingra <yaten598@gmail.com>

vercel · 2025-01-07T06:57:07Z

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

Name	Status	Preview	Updated (UTC)
nodejs-org	✅ Ready (Inspect)	Visit Preview	Jan 24, 2025 3:12pm

yaten2302 · 2025-01-07T07:38:23Z

I think currently on pressing Tab, the button is shown, but it's not getting pressed if the user presses Enter, I forgot take into account the Enter button, I'll push a commit for this too 👍

Edit: Sorry, will not ping 👍

AugustinMauroy · 2025-01-07T08:33:13Z

WOW stop pinging everyone ! we (website team) receive notification when there are changes on repo.

apps/site/components/withNavBar.tsx

Signed-off-by: Yaten Dhingra <yaten598@gmail.com>

apps/site/components/withNavBar.tsx

Signed-off-by: Yaten Dhingra <yaten598@gmail.com>

araujogui · 2025-01-08T15:09:40Z

apps/site/components/withNavBar.tsx


  const toggleCurrentTheme = () =>
    setTheme(resolvedTheme === 'dark' ? 'light' : 'dark');

  return (
    <div>
+      <a className={classNameOnTabPress} href="#main">


Anchor href is not working, main element is missing the id attribute.

This href won't work as of now, because I just set it randomly to #main, I wanted to clarify that to what section the user must be directed to on clicking this button?

I believe It should redirect to main indeed

For this, is there a main class present in the pages? Ig not?
There is this <main/> tag present in the about page, just for example.

Sorry, I don't understand what you saying

I was trying to say that on any page, since we're redirecting to #main, there is no such class as #main on any page, so where should the user be redirected?

User should be "redirected" to main, layouts lives on apps/site/layouts.

apps/site/components/withNavBar.tsx

Signed-off-by: Yaten Dhingra <yaten598@gmail.com>

yaten2302 · 2025-01-10T14:24:38Z

I've pushed a minor change(typo). Just wanted to ask, that could anyone please guide regarding these points, I was having a few doubts:

Signed-off-by: Yaten Dhingra <yaten598@gmail.com>

yaten2302 · 2025-01-16T17:46:03Z

Hi, I've pushed a commit, and I think that this addresses all the changes mentioned and required! Could anyone please have a look at it and LMK if any change has to be made in this?

yaten2302 · 2025-01-19T14:32:48Z

Hey, can anyone please have a look and LMK if any more changes are required?

AugustinMauroy · 2025-01-19T14:54:40Z

I don't really know what to do with this button, but at the moment it degrades the UI.

apps/site/components/withNavBar.tsx

Signed-off-by: Yaten Dhingra <yaten598@gmail.com>

aduh95 · 2025-01-22T18:16:06Z

I've tried to pull that PR on top of main and run npm run dev but couldn't get the button to show up, am I missing something?

yaten2302 · 2025-01-23T05:44:48Z

2025-01-23.11-09-38.mp4

I've shared a screen recording for the same, is this behavior not being shown on your side? I just checked that in the deployment also it's showing this.

aduh95 · 2025-01-23T10:19:07Z

No even running from the DevTools Array.from(document.all).find(a => a.textContent.trim() === 'Skip to content') returns undefined 🤔

My steps are:

git fetch https://github.com/nodejs/nodejs.org.git HEAD
git reset FETCH_HEAD --hard
curl -L https://github.com/nodejs/nodejs.org/pull/7390.patch | git am
npm ci
npm run dev

yaten2302 · 2025-01-23T11:42:16Z

I used deployment dev tools:

canerakdas · 2025-01-23T16:51:49Z

apps/site/components/withLayout.tsx

@@ -19,6 +21,7 @@ const layouts = {
  'blog-category': BlogLayout,
  download: DownloadLayout,
  article: ArticlePageLayout,
+  skipToContent: WithNavBar,


I don't think skipToContent is a page layout, IMO it is a feature in layouts

Someone mentioned about this in previous suggestions, that it needs to be added in withLayout.tsx, should I remove this?

I think you are talking about this comment; #7390 (comment)

@araujogui As far as I understand, the suggestion is to move the skip to content button here, not add it as a layout right? 🤔

Yes, saying about this 👍

apps/site/components/withNavBar.tsx

canerakdas · 2025-01-23T17:20:47Z

apps/site/types/layouts.ts

@@ -6,4 +6,5 @@ export type Layouts =
  | 'blog-category'
  | 'blog-post'
  | 'download'
-  | 'article';
+  | 'article'
+  | 'skipToContent';


Similarly, IMO there should be no skip to content layout

Signed-off-by: Yaten Dhingra <yaten598@gmail.com>

aduh95 · 2025-01-24T17:45:48Z

It looks like we don't have any element with a main id, so the button is moot.

I've also tried to use the website using VoiceOver, it seems completely impossible to access the menu from there, which is pretty bad, but I guess I'll open a separate issue for that.

yaten2302 · 2025-01-24T18:04:50Z

Yes, I think, the desired behavior is such that on clicking skip to content button, the user should be redirected to the section with id - #main, right?
And I think it's not present as of now, so once all the ID's are added, then it would be helpful, am I right?

aduh95 · 2025-01-24T21:17:36Z

As you can read in https://www.w3.org/TR/WCAG20-TECHS/G1.html#G1-tests, in order to fix the linked issue the following checks need to pass:

[…]
4. Check that activating the link moves the focus to the main content.
5. Check that after activating the link, the keyboard focus has moved to the main content.

Currently those check fail; adding the ids might fix that yes

yaten2302 · 2025-01-25T05:43:52Z

May I create an issue for the same?
Something like - Add #main id to navigate to main section of pages?

yaten2302 · 2025-01-25T05:45:20Z

By the time this issue gets fixed, I would like to work upon some other issues too! And this PR can be kept on hold for a while till the time the main id is added in all pages.

ovflowd

I'd ask to first apply the suggestions made from other users. I appreciate that you're iterating over our feedback and appreciate your time here.

Having that said, I don't think this can land as it is right now. The button is simply hiding other content and overlapping with it in a non-productive manner.

I'd recommend using the small variant of our Button, and making it floating on the bottom right (position: fixed) or something of sort.

That said, I don't really think our pages need a "skip to content" Button -- Our pages are already designed in a way that the content is always already present even when scroll is zeroed. It is a very responsive design and that does not require a button to skip to content, since again, the content is always visible.

I'm -1 on this change.

aduh95 · 2025-01-26T17:15:07Z

For example the download page could definitely benefit form it, it takes a long while to get to the download button using VoiceOver – but ~~that could be solved by adding a low tabindex attribute to the download button(s)~~ EDIT: I was wrong, messing with tabindex is listed as an anti-pattern

ovflowd · 2025-01-26T17:18:51Z

For example the download page could definitely benefit form it, it takes a long while to get to the download button using VoiceOver – but that could be solved by adding a low tabindex attribute to the download button(s).

Which download page? The download page renders all in one screen without even needing to scroll

ovflowd · 2025-01-26T17:21:40Z

Also the whole downloads page is the main content; A skip to main content there just does not make any sense here.

aduh95 · 2025-01-26T17:25:24Z

For example the download page could definitely benefit form it, it takes a long while to get to the download button using VoiceOver – but that could be solved by adding a low tabindex attribute to the download button(s).

Which download page? The download page renders all in one screen without even needing to scroll

VoiceOver is a screen reader built into Apple OSes (macOS, iOS, tvOS, watchOS, iPadOS) designed to increase accessibility for blind and low-vision users, as well as for users with dyslexia. The fact that all the content is visible without scrolling is irrelevant for folks who can't see

ovflowd · 2025-01-26T21:24:49Z

For example the download page could definitely benefit form it, it takes a long while to get to the download button using VoiceOver – but that could be solved by adding a low tabindex attribute to the download button(s).

Which download page? The download page renders all in one screen without even needing to scroll

VoiceOver is a screen reader built into Apple OSes (macOS, iOS, tvOS, watchOS, iPadOS) designed to increase accessibility for blind and low-vision users, as well as for users with dyslexia. The fact that all the content is visible without scrolling is irrelevant for folks who can't see

Are we talking about the same thing? I'm talking about a skip to content button. That doesn't make sense to be rendered nor added. We can detect by CSS queries if the user is using a VoiceOver tool, in those situations if you believe any extra accessibility accessory needs to be added here, I'd like to follow standards proposed by a11y.

aduh95 · 2025-01-26T21:36:48Z

As you can read in #7220 description (or directly in https://www.w3.org/TR/WCAG20-TECHS/G1.html#G1-tests), the "Skip to content" button is an a11y best practice. Anyway, your point of "we may actually not need it" is not invalid, I'm not disputing it – though I'd encourage you to try browsing the Download page with a screen reader, getting to the important links takes a frustrating long time.

ovflowd · 2025-01-26T22:06:07Z

As you can read in #7220 description (or directly in w3.org/TR/WCAG20-TECHS/G1.html#G1-tests), the "Skip to content" button is an a11y best practice. Anyway, your point of "we may actually not need it" is not invalid, I'm not disputing it – though I'd encourage you to try browsing the Download page with a screen reader, getting to the important links takes a frustrating long time.

Noted. I believe this is something tricky. I would consider closing this PR and open an discussion first so we can properly discuss about how to approach this. Maybe for key/essential pages we can have invisible elements that are rendered for screen readers, that's also a common practice.

yaten2302 · 2025-01-27T06:23:52Z

Should I close this PR for now? Maybe after some discussion, a new PR can be created?

init: add skip-to-main-content shortcut

bfd4bbd

Signed-off-by: Yaten Dhingra <yaten598@gmail.com>

yaten2302 requested a review from a team as a code owner January 7, 2025 06:57

yaten2302 mentioned this pull request Jan 7, 2025

Adding a "Skip to Content" Link #7220

Open

vercel bot deployed to Preview January 7, 2025 06:58 View deployment

AugustinMauroy requested changes Jan 7, 2025

View reviewed changes

apps/site/components/withNavBar.tsx Show resolved Hide resolved

apps/site/components/withNavBar.tsx Outdated Show resolved Hide resolved

apps/site/components/withNavBar.tsx Outdated Show resolved Hide resolved

fix imports in withNavBar.tsx

85e3e14

Signed-off-by: Yaten Dhingra <yaten598@gmail.com>

vercel bot deployed to Preview January 7, 2025 08:50 View deployment

added classNames lib

af3799a

Signed-off-by: Yaten Dhingra <yaten598@gmail.com>

vercel bot deployed to Preview January 7, 2025 09:01 View deployment

aduh95 requested changes Jan 7, 2025

View reviewed changes

apps/site/components/withNavBar.tsx Outdated Show resolved Hide resolved

canerakdas reviewed Jan 7, 2025

View reviewed changes

apps/site/components/withNavBar.tsx Outdated Show resolved Hide resolved

made skipToContent button CSS-only

0c9c1b5

Signed-off-by: Yaten Dhingra <yaten598@gmail.com>

vercel bot deployed to Preview January 7, 2025 17:28 View deployment

araujogui requested changes Jan 8, 2025

View reviewed changes

Merge branch 'nodejs:main' into main

9f14df0

vercel bot had a problem deploying to Preview January 10, 2025 14:14 Failure

changed classNameOnTabPress to skipToContent

0b24e44

Signed-off-by: Yaten Dhingra <yaten598@gmail.com>

vercel bot deployed to Preview January 10, 2025 14:21 View deployment

Merge branch 'nodejs:main' into main

c05f0b9

vercel bot deployed to Preview January 11, 2025 17:42 View deployment

Merge branch 'nodejs:main' into main

b5f2322

vercel bot deployed to Preview January 15, 2025 18:13 View deployment

added skipToContent in withLayout.tsx

536bfd7

Signed-off-by: Yaten Dhingra <yaten598@gmail.com>

vercel bot deployed to Preview January 15, 2025 18:18 View deployment

Merge branch 'nodejs:main' into main

61d34c8

vercel bot deployed to Preview January 16, 2025 17:34 View deployment

added appropriate class to <a/> tag

db8543f

Signed-off-by: Yaten Dhingra <yaten598@gmail.com>

vercel bot deployed to Preview January 16, 2025 17:45 View deployment

AugustinMauroy reviewed Jan 19, 2025

View reviewed changes

apps/site/components/withNavBar.tsx Outdated Show resolved Hide resolved

updated skitToContent classname

2a465e3

Signed-off-by: Yaten Dhingra <yaten598@gmail.com>

vercel bot deployed to Preview January 22, 2025 17:53 View deployment

canerakdas reviewed Jan 23, 2025

View reviewed changes

canerakdas mentioned this pull request Jan 24, 2025

fix: Button accessibility #7420

Merged

5 tasks

minor: remove custom className skipToContent

2233341

Signed-off-by: Yaten Dhingra <yaten598@gmail.com>

vercel bot deployed to Preview January 24, 2025 15:12 View deployment

ovflowd requested changes Jan 26, 2025

View reviewed changes

init: add skip-to-main-content shortcut #7390

Are you sure you want to change the base?

init: add skip-to-main-content shortcut #7390

Conversation

yaten2302 commented Jan 7, 2025

Description

Related Issues

Check List

vercel bot commented Jan 7, 2025 • edited Loading

yaten2302 commented Jan 7, 2025 • edited Loading

AugustinMauroy commented Jan 7, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaten2302 commented Jan 10, 2025

yaten2302 commented Jan 16, 2025

yaten2302 commented Jan 19, 2025

AugustinMauroy commented Jan 19, 2025 • edited Loading

aduh95 commented Jan 22, 2025

yaten2302 commented Jan 23, 2025

aduh95 commented Jan 23, 2025

yaten2302 commented Jan 23, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduh95 commented Jan 24, 2025

yaten2302 commented Jan 24, 2025

aduh95 commented Jan 24, 2025

yaten2302 commented Jan 25, 2025

yaten2302 commented Jan 25, 2025

ovflowd left a comment

Choose a reason for hiding this comment

aduh95 commented Jan 26, 2025 • edited Loading

ovflowd commented Jan 26, 2025

ovflowd commented Jan 26, 2025

aduh95 commented Jan 26, 2025

ovflowd commented Jan 26, 2025

aduh95 commented Jan 26, 2025

ovflowd commented Jan 26, 2025

yaten2302 commented Jan 27, 2025

vercel bot commented Jan 7, 2025 •

edited

Loading

yaten2302 commented Jan 7, 2025 •

edited

Loading

AugustinMauroy commented Jan 19, 2025 •

edited

Loading

aduh95 commented Jan 26, 2025 •

edited

Loading