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

Add swup-a11y-plugin #34

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add swup-a11y-plugin #34

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
"@astrojs/check": "^0.9.4",
"@astrojs/tailwind": "^5.1.2",
"@locomotivemtl/grid-helper": "^1.0.0",
"@swup/a11y-plugin": "^5.0.0",
"@swup/head-plugin": "^2.2.0",
"@swup/preload-plugin": "^3.2.10",
"@swup/scripts-plugin": "^2.1.0",
Expand Down
2 changes: 1 addition & 1 deletion src/pages/about.astro
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import Wysiwyg from '../components/Wysiwyg/Wysiwyg.astro';

<Layout title="About">
<header class="u-container">
<h1 class="u-heading-h1">About</h1>
<h1 id="page-heading" class="u-heading-h1">About</h1>
</header>

<div class="w-full u-mt-fluid-xl u-h-[20vh] u-bg-black"></div>
Expand Down
2 changes: 1 addition & 1 deletion src/pages/index.astro
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import Layout from '../layouts/Layout.astro';
<Layout title="Welcome to the Locomotive Astro Boilerplate.">
<header class="u-container">
<Icon name="logo" />
<h1 class="u-heading-h1">Welcome to the Locomotive Astro Boilerplate.</h1>
<h1 id="page-heading" class="u-heading-h1">Welcome to the Locomotive Astro Boilerplate.</h1>

<button class="shortcut-functions">Test</button>
</header>
Expand Down
2 changes: 1 addition & 1 deletion src/pages/post.astro
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ const blocks = [
<Layout title="Post title">
<header class="u-container">
<Icon name="logo" />
<h1 class="u-heading-h1">Post title</h1>
<h1 id="page-heading" class="u-heading-h1">Post title</h1>
</header>

<div class="u-container u-mt-fluid-xl">
Expand Down
15 changes: 13 additions & 2 deletions src/scripts/classes/Transitions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { toDash } from '@scripts/utils/string';
import SwupA11yPlugin from '@swup/a11y-plugin';
import SwupHeadPlugin from '@swup/head-plugin';
import SwupPreloadPlugin from '@swup/preload-plugin';
import SwupScriptsPlugin from '@swup/scripts-plugin';
Expand Down Expand Up @@ -55,7 +56,8 @@ export class Transitions {
preloadHoveredLinks: true,
preloadInitialPage: !import.meta.env.DEV
}),
new SwupScriptsPlugin()
new SwupScriptsPlugin(),
new SwupA11yPlugin()
]
});

Expand Down Expand Up @@ -105,9 +107,18 @@ export class Transitions {
* @see https://swup.js.org/hooks/#visit-start
* @param visit: VisitType
*/
onVisitStart() {
onVisitStart(visit: VisitType) {
document.documentElement.classList.add(Transitions.TRANSITION_CLASS);
document.documentElement.classList.remove(Transitions.READY_CLASS);

if (visit.a11y) {
visit.a11y.focus = 'h1#page-heading';
Copy link
Member Author

@Jerek0 Jerek0 Nov 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

visit.a11y.focus = 'h1#page-heading';

I'd like to have @mcaskill input on this.

I noticed by testing VoiceOver that a page change would either:

  • stay on the focused element (if it's out of #swup container)
  • move the focus to the body, but as if we reached the end of the document (moving VoiceOver's cursor forward wouldn't do anything and moving it backwards would read the end of the footer)

Considering those observations, specifying the focus target on the page heading seems pertinent! But it seems to prevent Swup's aria-live announcements to happen as a side effect... Not sure if that's a real issue though as the plugin announces Navigated to {title} by default (title value being decided as such).
We're basically only losing the Navigated to bit, but that bit increases clarity about what's happening.

Could you please try the navigation behavior with/without this line and give your recommendation?

Copy link
Member

Choose a reason for hiding this comment

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

So, Swup is the one at fault for the recent issue I noticed across some projects where the page does not scroll back to the top after clicking a hyperlink in the footer.

Swup created a problem and instead of fixing it introduces a non-standard solution.

But it seems to prevent Swup's aria-live announcements to happen as a side effect... Not sure if that's a real issue though as the plugin announces Navigated to {title} by default […]

If its announcing Navigated to {title} that means its aria-live element is working as intended. It's extracting the text node from the first <h1> which happens to be the target of your visit.a11y.focus.

Since the <h1> can't be guaranteed to have a relevant title, maybe the plugin's headingSelector could target <title> instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

If its announcing Navigated to {title} that means its aria-live element is working as intended. It's extracting the text node from the first <h1> which happens to be the target of your visit.a11y.focus.

Indeed. What I'm saying is that this doesn't happen anymore if I specify a focus target with visit.a11y.focus = 'h1#page-heading';. Seems like the element being focused steals the VoiceOver attention to the detriment of the aria-live element.

So, Swup is the one at fault for the recent issue I noticed across some projects where the page does not scroll back to the top after clicking a hyperlink in the footer.

Can you send me an example of a project where you noticed this on Slack? It's not necessarily related

Copy link
Member

Choose a reason for hiding this comment

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

In regards to visit.a11y.focus, that makes sense that it's stealing focus. Not necessarily the intended behavior. I noticed that Swup's documentation site still uses v4 whose behaviour is to focus the first h1.


// Best not to assume where to move the focus on fragment visits
if (visit.fragmentVisit) {
visit.a11y.focus = false;
}
}
Comment on lines +114 to +121
Copy link
Member

Choose a reason for hiding this comment

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

I would discourage this for most standard pages.

The body should be the default focused element.

On pages that use modals/drawers, that container or its main heading should receive focus.

If you insist on keeping this, the logic should be collapsed into a single statement:

Suggested change
if (visit.a11y) {
visit.a11y.focus = 'h1#page-heading';
// Best not to assume where to move the focus on fragment visits
if (visit.fragmentVisit) {
visit.a11y.focus = false;
}
}
// Best not to assume where to move the focus on fragment visits
if (visit.a11y && !visit.fragmentVisit) {
visit.a11y.focus = 'h1#page-heading';
}

Copy link
Member Author

@Jerek0 Jerek0 Nov 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Pages using modals/drawers are typically in a <dialog> which automatically focuses its first focusable element upon showModal() call.

The issue I see with other types of fragment visits (probably a single section inside the main of the page) resetting the focus to the body is it will require the user to travel again across the page to reach the updated content which could be irritating and potentially disorienting? But at the same time, it's how it would behave without an SPA system... so I'm not entirely sold on that

Also, I posted a comment asking for your advice regarding this bit of code but I didn't notice Github left it as "pending" which prevented you from seeing it.. my bad! It brings some explanations to my thought process there

Copy link
Member

Choose a reason for hiding this comment

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

Stick to Web standards and its default, established, behaviours.

If a project's stakeholders can afford to hire accessibility consultants and dedicate time to testing with real users, then the behaviour can be adjusted to accommodate their expectations.

}

/**
Expand Down
4 changes: 4 additions & 0 deletions types/swup.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,8 @@ type VisitType = {
to: {
html: string;
};
a11y?: {
announce: string;
focus: string | false;
};
};