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.

Sign up for GitHub

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

Jump to bottom

feat: Add "Skip to main content" link and update nav behavior #2253

Merged
merged 24 commits into from
Nov 30, 2023
Merged

feat: Add "Skip to main content" link and update nav behavior #2253

merged 24 commits into from
Nov 30, 2023

Conversation

jhildenbiddle
Copy link
Member

@jhildenbiddle jhildenbiddle commented Oct 7, 2023 •
edited
Loading

Summary

These updates align with accessibility best practices. The skip link and the updated navigation behavior allow for more efficient navigation via the keyboard and screen readers.

  1. Add a "Skip to main content" button as the first element in the DOM with options.
    • Includes configuration options to disable, modify the label, and specify localized labels based on the route path.
    • Dynamically updates label text on route path change (use "Translations" menu in preview for demo)
  2. Updated navigation behavior:
    • When loading a Docsify site:
      • If the URL does not contains a heading id, focus will remain on the <body> element. This is the browser's default behavior, which allows the skip link to appear when visitors press Tab one time.
      • If the URL contains a heading id, focus will be moved to the linked heading.
    • When navigating to a new page, focus will move to the first heading in the content area.
    • When navigating to a heading, focus will move to the linked heading.
  3. Updated documentation to include skipLink configuration options

This bug also addresses a separate issue where caused Docsify to treat successfully loaded but empty markdown files as 404 errors.

Related research

Screenshots

Safari 17 on macOS 14.0

CleanShot 2023-10-07 at 17 28 38@2x

CleanShot 2023-10-07 at 17 30 03@2x

Related issue, if any:

#494
#495
#2294 (Fixes loading of empty markdown files)

What kind of change does this PR introduce?

  • Feature
  • Docs
  • Tests

For any code change:

  • Related documentation has been updated, if needed
  • Related tests have been added or updated, if needed

Does this PR introduce a breaking change?

No

Tested in the following browsers:

  • Chrome
  • Firefox
  • Safari

@jhildenbiddle
Add skip link to main content
956bd1e 
Fix #494
@jhildenbiddle
Focus on content after navigation
d9ca92b
@jhildenbiddle
Allow config options & dynamic text
14c4641 
Allows skip link text to be updated dynamically based on the route path. See docisfy site’s “Translations” menu as an example of why this is necessary.
@jhildenbiddle
Update docs
7eff474
@vercel
Copy link

vercel bot commented Oct 7, 2023 •
edited
Loading

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

Name Status Preview Comments Updated (UTC)
docsify-preview ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 30, 2023 4:49am

@jhildenbiddle jhildenbiddle changed the title feat: Add "Skip to main content" link feat: Add "Skip to main content" link and update nav behavior Oct 8, 2023
@jhildenbiddle jhildenbiddle changed the title feat: Add "Skip to main content" link and update nav behavior Feat: Add "Skip to main content" link and update nav behavior Oct 16, 2023
@jhildenbiddle jhildenbiddle mentioned this pull request Oct 16, 2023
Closed
This was referenced Oct 17, 2023
Closed
Closed
trusktr
trusktr reviewed Oct 22, 2023
View reviewed changes
trusktr
trusktr reviewed Oct 22, 2023
View reviewed changes
trusktr
trusktr reviewed Oct 22, 2023
View reviewed changes
trusktr
trusktr previously approved these changes Oct 22, 2023
View reviewed changes
@jhildenbiddle
Merge branch 'develop' into 494-skip-link
d7101bb 
# Conflicts:
#	src/core/render/index.js
Koooooo-7
Koooooo-7 reviewed Nov 20, 2023
View reviewed changes
Copy link
Member

@Koooooo-7 Koooooo-7 left a comment

Choose a reason for hiding this comment

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

Can not show the btn locally or on the vercel Preview, although the dom is insert success.
Is there any style issue?

@jhildenbiddle
Copy link
Member Author

jhildenbiddle commented Nov 22, 2023 •
edited
Loading

@Koooooo-7 --

TL;DR: Press Tab after the initial site load.

  1. Click the preview link
  2. Press Tab

Or...

  1. Navigate directly to a preview page
  2. Press Tab

Can not show the btn locally or on the vercel Preview, although the dom is insert success. Is there any style issue?

The Vercel preview is working correctly.

The "Skip to main content" link is intended to be the first "focusable" element in the DOM. It is hidden by default and only visible when it receives focus. The intention is for it to be hidden for visitors navigating with a pointing device, but easily accessible to keyboard navigators and assistive device users by pressing Tab after the initial site load.

If you are wondering why pressing Tab does not result in the skip link being displayed after subsequent page loads, the following explanation taken from this comment explains:

For SPAs, there is no definitively "correct" behavior regarding where to move the focus after new content has loaded, although the majority opinion based on my research is that moving focus to the first heading in the content area is the most desirable behavior. The alternative is to set the focus back on the <body> to mimic the page load behavior of "standard" web site (i.e. non-SPA sites), but this is a far less practical behavior that requires users to manually navigate to the content area after every new page is loaded. Less than a minute testing with the keyboard or a screen reader makes choosing between these two options easy.

sy-records
sy-records previously approved these changes Nov 22, 2023
View reviewed changes
Copy link
Member

@sy-records sy-records left a comment

Choose a reason for hiding this comment

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

Good jobs!

@Koooooo-7
Copy link
Member

Hi @jhildenbiddle , sorry for the misunderstanding here.
Hit the Tab, I can see it now !

Based on our site here, about the multi langs support, I found that it only works when fresh the page ( as you mentioned, trigger after the initial Load), so every time fresh pages gonna trigger the skipLink by tab as well .
My concern is when we fresh the page, or in multi langs, we may already on the main content, it seems we don't need this btn.
Should we only make it works on the coverage showing?

@jhildenbiddle
Copy link
Member Author

Hi @jhildenbiddle , sorry for the misunderstanding here. Hit the Tab, I can see it now !

No worries, @Koooooo-7. Happy to hear it's working as expected for you!

My concern is when we fresh the page, or in multi langs, we may already on the main content, it seems we don't need this btn. Should we only make it works on the coverage showing?

👍 I've made a small update to the PR. Focus will now move to the linked heading when loading a URL with a linked heading id (e.g., ?id=foo). This behavior mimics the standard hash behavior in standard web pages (e.g, #foo).

A complete summary of the new navigation behavior is available in the summary and below:

  1. Updated navigation behavior:
    • When loading a Docsify site:
      • If the URL does not contains a heading id, focus will remain on the <body> element. This is the browser's default behavior, which allows the skip link to appear when visitors press Tab one time.
      • If the URL contains a heading id, focus will be moved to the linked heading.
    • When navigating to a new page, focus will move to the first heading in the content area.
    • When navigating to a heading, focus will move to the linked heading.

Given the above behavior, I believe your concerns are addressed:

  • If a user loads a Docsify URL that contains a heading id, focus will move to the linked heading.
  • If a user navigates to a page using the "Translations" menu on docsify.js.org, focus will move to the first heading in the content area.

In both scenarios above, pressing Tab will not display the skip link.

Koooooo-7
Koooooo-7 previously approved these changes Nov 23, 2023
View reviewed changes
Copy link
Member

@Koooooo-7 Koooooo-7 left a comment

Choose a reason for hiding this comment

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

LGTM. thx @jhildenbiddle for you ideas and contributions. 👍

@jhildenbiddle
Merge branch 'develop' into 494-skip-link
ce9198f 
# Conflicts:
#	src/core/render/index.js
#	src/core/render/tpl.js
@jhildenbiddle
Copy link
Member Author

@sy-records @Koooooo-7 --

Would you mind approving this PR again? I had to merge develop which cleared your previous approvals. Thx!

@sy-records sy-records changed the title Feat: Add "Skip to main content" link and update nav behavior feat: Add "Skip to main content" link and update nav behavior Nov 30, 2023
@sy-records sy-records merged commit 50b84f7 into develop Nov 30, 2023
@sy-records sy-records deleted the 494-skip-link branch November 30, 2023 05:11
This was referenced Dec 1, 2023
Closed
Closed
This was referenced Dec 4, 2023
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Koooooo-7 Koooooo-7 Koooooo-7 approved these changes

@sy-records sy-records sy-records approved these changes

@trusktr trusktr Awaiting requested review from trusktr

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@jhildenbiddle @Koooooo-7 @trusktr @sy-records