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

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

wants to merge 1 commit into from

Conversation

Jerek0
Copy link
Member

@Jerek0 Jerek0 commented Nov 6, 2024

Description and Context

Add @swup/a11y-plugin by default to improve... a11y!

Notable improvements:

  • Announces page changes with dedicated aria-live="assertive"
  • Allows to set a focus selector to determine targeted element on page changes (defaults to body)
  • As stated in Creating Skip Navigation Links #29, this also fixes the behavior of anchor links not properly focusing target and breaking expected taborder. (target was scrolled into view but wouldn't receive actual focus)

Note

It seems though that content:announce & content:focus may conflict with each other. See my comment below regarding specific focus setting

Type of Change:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Refactor / internal changes (changes do not affect functionality, but are an improvement to the codebase)

Copy link

vercel bot commented Nov 6, 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 6, 2024 11:25pm

@Jerek0 Jerek0 marked this pull request as ready for review November 7, 2024 15:42
@Jerek0 Jerek0 mentioned this pull request Nov 7, 2024
6 tasks
@devenini
Copy link
Member

devenini commented Nov 7, 2024

What is the expected behavior for focus on the first page load? When changing routes, I see that the plugin correctly focuses on the element specified by the focus option. However, on the first page load, it appears that focus is set to the body element instead

@Jerek0
Copy link
Member Author

Jerek0 commented Nov 7, 2024

Considering Swup is dedicated to page transitions handling, I don't believe it should impact the first page load.

The vanilla behavior of focus being set to body on first load therefore seems adequate to me and allows the first focusable element on your arrival to the website to be the skip-link (if present).

Then it seems reasonable that if you navigate to another page of the same site that you want to access the main content directly: the h1

@mcaskill
Copy link
Member

mcaskill commented Nov 7, 2024

What is the expected behavior for focus on the first page load? […] on the first page load, it appears that focus is set to the body element instead

The body is the default expected element and that should be maintained.

Announces page changes with dedicated aria-live="assertive"

VoiceOver already announces changes to document title. I have not been able to test with other screen readers such as JAWS and NVDA which require Windows (I need to clear up some disk space to install it).

Looking at how this Swup plugin determines what to announce, prioritizing the <h1> risks causing trouble unless one ensures each page's level 1 heading contains a relevant title. Not sure why this is not configurable, this should be the document title's job.

Note that an important aspect of Web accessibility is declaring what is being made accessible. The plugin has no Accessibility Statement nor Accessibility Compliance Report to explain its assumptions and logic beyond its documentation. The plugin claims to be have been tested in VoiceOver, JAWS, and NVDA, but provides no results of these tests or explanations of what their behaviour is.

Comment on lines +114 to +121
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;
}
}
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

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.

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

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.

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.

3 participants