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

Creating Skip Navigation Links #29

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from
Draft

Creating Skip Navigation Links #29

wants to merge 12 commits into from

Conversation

arnvvd
Copy link
Contributor

@arnvvd arnvvd commented Oct 21, 2024

Description and Context

This PR introduces skip navigation links for improved accessibility.

A component is implemented to allow users to bypass repetitive navigation elements and jump directly to the main content. The skip link targets the #page-heading that should always be present on the h1 of the page, ensuring a smooth and intuitive navigation experience for keyboard and screen reader users.

It can also be used locally to skip a long section of content using the target property and making sure it's a children of a position: relative container.

Note

This PR has been put into draft until #34 is merged. This PR being dependent of @swup/a11y-plugin as stated in #29 (comment)

Type of Change:

  • New feature (non-breaking change which adds functionality)

Screenshots

Capture d’écran, le 2024-10-21 à 14 36 30

How to test

Navigate to any page and try to navigate using the keyboard, and then again with a screen reader. The first element in the taborder should be the SkipLink button and it should only reveal on focus. After "clicking" on it, it should focus to the h1 and scroll to it.

Note

Notice on the About page that the heading is only visible to screen-readers, but the scrollto feature remains as well as the focus move.

Also, go on the homepage and test the local SkipLink right before the accordion list. Clicking on it should teleport to a span right after the list and announce "End of accordions list" to screen reader users.

Checklist

  • I added tests to cover my changes
  • All new and existing tests are passing
  • I formatted and linted the code according to repo specs
  • I rebased changes with main so that they can be merged easily
  • I have updated the documentation accordingly (if applicable)

Copy link

vercel bot commented Oct 21, 2024

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

Name Status Preview Updated (UTC)
astro-boilerplate ✅ Ready (Inspect) Visit Preview Nov 7, 2024 3:42pm

@arnvvd arnvvd requested a review from devenini October 21, 2024 18:37
@arnvvd arnvvd marked this pull request as draft October 21, 2024 18:39
@arnvvd arnvvd marked this pull request as ready for review October 21, 2024 18:42
@arnvvd arnvvd requested a review from Jerek0 October 21, 2024 18:42
@devenini
Copy link
Member

LGTM. @mcaskill could you check if everything makes sense in terms of a11y and semantics?

@devenini devenini requested a review from mcaskill October 21, 2024 19:00
@Jerek0
Copy link
Member

Jerek0 commented Oct 21, 2024

Tip

Also note that this component can be used to skip any large portion of content: it's not limited to skipping navigation links to reach the main content of the page.

Let's say you have a large table of entries inside your page: you could use it to allow the user to skip that content by giving a custom selector with the target prop

Remove console.log
Remove console.log
Copy link
Member

@mcaskill mcaskill left a comment

Choose a reason for hiding this comment

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

Feels overly complicated for such primitive functionality.

The most basic version of a skip link is an anchor and a target:

<a href="#main">Skip to content</a>

<main id="main"></main>

No JavaScript needed.

If you insist on adding JavaScript, I would recommend using an anchor to make it a progressive enhancement. Intercept the anchor click to prevent a fragment being added to the current URL or replace the anchor with a button.

As far as I can tell (please confirm), the presence of the [tabindex="-1"] attribute was only necessary for MSIE and similarly older browsers. The default value of the tabIndex property is -1 for non-interactive elements. The attribute is essentially unnecessary in modern browsers.

If we feel obliged to include the attribute, it should only ever be added and removed dynamically. No need to define it manually in the HTML (as is the case in Layout.astro).

It should be documented with the component that it is preferable to target a heading element instead of a large container.

Finally, similar to manually adding [tabindex], you could remove the [role="main"] attribute from the <main> element. The <main> element comes with the implicit role main. It is allegedly supported by 83% of screen readers since ~2021.

onClick(e: Event) {
e.preventDefault();

const $mainContent = document.querySelector(
Copy link
Member

Choose a reason for hiding this comment

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

This variable should be renamed to $target to match corresponding custom target attribute.

e.preventDefault();

const $mainContent = document.querySelector(
this.parentElement?.getAttribute('target') ?? 'main[tabindex]'
Copy link
Member

Choose a reason for hiding this comment

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

I recommend renaming the target attribute to href to avoid repurposing the concept of the existing target attribute.

Also, the fallback should be main instead of main[tabindex] since eitherway, there should only be one <main>.

src/components/SkipLink/SkipLink.ts Outdated Show resolved Hide resolved
Copy link
Member

@mcaskill mcaskill left a comment

Choose a reason for hiding this comment

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

Is there a way with Astro for your SkipLink custom element to just extend the Button custom element?

This way the SkipLink is the anchor element.

Comment on lines 4 to 17
interface Props {
label?: string;
class?: string;
target?: string;
}

const { label = 'Skip to main content', target, class: propsClass, ...rest } = Astro.props;

const classes = ['c-skip-link', propsClass];
---

<c-skip-link class:list={classes} {...rest} data-no-swup>
<Button href="#main">{label}</Button>
</c-skip-link>
Copy link
Member

Choose a reason for hiding this comment

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

A Web site can have more than one skip link that target different locations in a document. We could reasonably assume that the first, and if only, skip link is one that targets a #main element.

Suggested change
interface Props {
label?: string;
class?: string;
target?: string;
}
const { label = 'Skip to main content', target, class: propsClass, ...rest } = Astro.props;
const classes = ['c-skip-link', propsClass];
---
<c-skip-link class:list={classes} {...rest} data-no-swup>
<Button href="#main">{label}</Button>
</c-skip-link>
interface Props {
label?: string;
class?: string;
href?: string;
}
const { label = 'Skip to main content', href = '#main', class: propsClass, ...rest } = Astro.props;
const classes = ['c-skip-link', propsClass];
---
<c-skip-link class:list={classes} {...rest} data-no-swup>
<Button href={href}>{label}</Button>
</c-skip-link>

src/components/SkipLink/SkipLink.scss Outdated Show resolved Hide resolved
Comment on lines 7 to 8
// Binding
this.onClick = this.onClick.bind(this);
Copy link
Member

Choose a reason for hiding this comment

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

Rebinding is unnecessary if you declare your method as an arrow function:

- onClick(e: Event) {
+ onClick: (e: Event) => {

Copy link
Member

Choose a reason for hiding this comment

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

To my understanding that syntax is not supported inside of a class. Unless i'm missing something?

This would work if we moved the function inside the constructor but I'm not found of the idea

Copy link
Member

@mcaskill mcaskill Oct 23, 2024

Choose a reason for hiding this comment

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

I'm not fond of moving it to the constructor either.

It does work within classes as a class field. It links this to the instance. I've used it in personal projects and works fine as far as I can tell.

Copy link
Member

Choose a reason for hiding this comment

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

Oh so you meant

- onClick: (e: Event) => {
+ onClick = (e: Event) => {

Indeed that works, thanks for the tip!

Copy link
Member

Choose a reason for hiding this comment

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

Oh 🤦 Yes. = and not :.

this.onClick = this.onClick.bind(this);

// UI
this.$link = this.firstElementChild as HTMLLinkElement;
Copy link
Member

Choose a reason for hiding this comment

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

I don't know how Astro handles custom elements but with native custom elements, you can't expect an element's children to be available during the constructor (unless maybe everything is executed after DOMContentLoaded).

Copy link
Member

Choose a reason for hiding this comment

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

Web components defined inside of an Astro component are loaded & executed after the main app.ts file, which I'm almost certain is executed after DOMContentLoaded as you suspected, so it shouldn't break

That being said, we could indeed move that assignation to the connectedCallback function instead just to be safe.

Comment on lines 45 to 47
const $target = document.querySelector(
this.$link?.getAttribute('href') ?? 'main'
) as HTMLElement;
Copy link
Member

Choose a reason for hiding this comment

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

For consistency with the default value of href in SkipLink.astro, this should also target #main. Using a comma to define multiple selectors allows the skip link to find the first available match.

Suggested change
const $target = document.querySelector(
this.$link?.getAttribute('href') ?? 'main'
) as HTMLElement;
const $target = document.querySelector(
this.$link?.getAttribute('href') ?? '#main,main'
) as HTMLElement;

@Jerek0
Copy link
Member

Jerek0 commented Oct 23, 2024

Feels overly complicated for such primitive functionality.
The most basic version of a skip link is an anchor and a target:

<a href="#main">Skip to content</a>
<main id="main"></main>

No JavaScript needed.

I tend to agree with this simple way of doing it. Testing it on the boilerplate as is doesn't work though because of swup breaking the focus behavior (as described in swup/swup#954). Doing it this way would require the addition of the Accessibility Plugin to the boilerplate, which would be a good idea IMO.

If you insist on adding JavaScript, I would recommend using an anchor to make it a progressive enhancement. Intercept the anchor click to prevent a fragment being added to the current URL or replace the anchor with a button.

I agree it would be the best way forward if we go forward with a Web Component

As far as I can tell (please confirm), the presence of the [tabindex="-1"] attribute was only necessary for MSIE and similarly older browsers. The default value of the tabIndex property is -1 for non-interactive elements. The attribute is essentially unnecessary in modern browsers.

A precision is stated in one of the sources your cited: (...) tabindex="-1" is still required on non-interactive elements to which focus is being moved programmatically via JavaScript.
That's why it's used in this PR, if we're going forward with the native solution described above though, we could get rid of the tabindex="-1"

Finally, similar to manually adding [tabindex], you could remove the [role="main"] attribute from the <main> element. The <main> element comes with the implicit role main. It is allegedly supported by 83% of screen readers since ~2021.

Fixed by c1f701c

Is there a way with Astro for your SkipLink custom element to just extend the Button custom element?
This way the SkipLink is the anchor element.

I wish this was possible, but it doesn't seem to be the case unfortunately. It may be possible for a web component to extend a native HTMLElement like <button> (tbc though, not an expert with Web Components), but an Astro component isn't extendable.


In the light of this discussion, I think we should:

  • simplify this Astro component by getting rid of the Web Component bit (hence why I didn't go forward with the suggested edits in the script file)
  • add Swup's Accessibility Plugin
  • rely on a h1#page-heading default target that should be present in all pages (will require caution & diligence) instead of #main

LMK your thoughts, if everyone agrees with this I can go forward and update this PR accordingly or submit a new one to merge here.

@mcaskill
Copy link
Member

A precision is stated in one of the sources your cited: (...) tabindex="-1" is still required on non-interactive elements to which focus is being moved programmatically via JavaScript. That's why it's used in this PR, if we're going forward with the native solution described above though, we could get rid of the tabindex="-1"

True. I should have clarified that declaring [tabindex="-1"] manually in the HTML is the unnecessary part. As its conceived SkipLink.onClickis handling the addition (and removal) the attribute for that programmatic focus move.

@Jerek0 Jerek0 mentioned this pull request Nov 6, 2024
4 tasks
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.

5 participants