Fix a11y CI workflow #2503

HiDeoo · 2024-10-22T14:28:26Z

This PR fixes the a11y CI workflow that has been silently failing on us for longer than we can even check 😓

After various fixes in the past to accommodate with pa11y-ci, this PR removes it in favour of directly running Axe through Playwright (which we're already using for our E2E tests). As we discussed the other day, my initial plan was to write yet again another Axe reporter but after researching for prior art, I found in the React Aria testing suite which uses Storybook, a package doing exactly what we need: axe-playwright with a built-in CLI reporter.

At the moment, the current test suite is running against the same rules that pa11y-ci was using plus the new wcag22aa (WCAG 2.2 Level AA).

With this kind of setup, we have way more control over the tests and even if at the moment, we are mostly mirroring the previous setup, we can easily extend it in the future with more tests, e.g. testing light/dark mode, testing after user interactions, etc.

Currently, the tests are failing due to a single error (landmark-unique) but I did not yet investigate it further to see if it's something we need to fix or if it's a rule to disable for w/e reason (that's why the PR is still a draft).

I'm also planning on experimenting with reducing the number of page tested: I don't think testing some pages provide much value (e.g. manual-setup & environmental-impact don't really provide much a11y value after having already tested the getting-started page).

changeset-bot · 2024-10-22T14:28:30Z

🦋 Changeset detected

Latest commit: 5f5eaeb

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@astrojs/starlight	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

netlify · 2024-10-22T14:28:45Z

✅ Deploy Preview for astro-starlight ready!

Name	Link
🔨 Latest commit	`5f5eaeb`
🔍 Latest deploy log	https://app.netlify.com/sites/astro-starlight/deploys/672e1c0a42b78a00089302ba
😎 Deploy Preview	https://deploy-preview-2503--astro-starlight.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
Lighthouse	1 paths audited Performance: 100 (no change from production) Accessibility: 100 (no change from production) Best Practices: 100 (no change from production) SEO: 100 (no change from production) PWA: - View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

HiDeoo · 2024-10-22T14:55:15Z

package.json

@@ -47,6 +47,7 @@
    "peerDependencyRules": {
      "ignoreMissing": [
        "@algolia/client-search",
+        "playwright",


This is needed as axe-playwright has a peer dependency of playwright which we explicitly don't use in favor of @playwright/test. Both official packages are different in the sense that playwright automatically installs browser binaries while @playwright/test does not. Running pnpm test:a11y in the packages/starlight/ directory will take care of installing the necessary dependencies in our case only when needed.

delucis · 2024-10-22T16:07:22Z

Amazing work @HiDeoo! Great to get these running again, and with the latest versions of stuff.

I took a quick look at the errors and suspect the error raised is technically correct, but a bit tricky to solve for.

It’s raising issues with both our aside and tab components.

Asides

Aside markup looks like this (simplified):

<aside aria-label="Note">
  <!-- ... -->
</aside>

<aside> is a landmark, but if you have multiple instances of the same aside variant on a page (without custom titles) you end up with multiple landmarks labelled the same via the aria-label attribute (e.g “Note” in this example).

Fixing this is tricky. Some of the options might be:

Structural/component-level approaches:
- Require setting a custom title always (not really practical I don’t think)
- Make the markup less semantic, so we’re not creating a landmark region in the first place (not sure about this either, but maybe there’s an argument for it)
Local, content-level approaches:
- Make sure we avoid multiple uses on the same page with the default title in the Starlight docs (works for us, but unlikely anyone else will follow it)

Probably needs some more research to understand the impact.

Tabs

Tab markup looks like this (simplified):

<starlight-tabs>
  <div class="tablist-wrapper">
    <ul role="tablist">
      <li role="presentation">
        <a role="tab" href="#tab-panel-378" id="tab-378">npm</a>
      </li>
      <!-- more tabs -->
    </ul>
  </div>
  <section id="tab-panel-378" aria-labelledby="tab-378" role="tabpanel">
    <!-- panel content -->
  </section>
  <!-- more panels -->
</starlight-tabs>

<section> is a landmark element and gets its label from the tablist item it is associated with via the aria-labelledby attribute. In this case, the section is labelled “npm”. Obviously, if you have multiple tabs of the same kind, e.g. npm/pnpm/yarn, on the same page that also results in multiple sections titled “npm” etc. just like the aside case.

So basically the same question for us to answer I guess: how important is it for these to be landmark regions, given that enforcing unique labelling may be impractical (in the aside case) or even impossible (in the tabs case, where the whole point is that they share a title).

HiDeoo · 2024-10-23T09:35:22Z

Thanks for the great write-up. After investigating the errors myself I came to the same conclusions and also the same questions and after some research, here are my current thoughts:

Asides

I think before jumping to the reported landmark-unique error, we could also take a look at the landmark-complementary-is-top-level which may be violated in our case with aside elements in the main landmark.

This best practice rule ensures that aside elements or elements with role=complementary are not an internal part of another ARIA landmark.

At the moment, this rule is not reported in our case, as even if it's not documented, an exception was added for allowing aside elements in the main landmark. By looking at the reasoning, it looks like the first iteration of the rule was implemented based on the ARIA standards, but they kept getting feedback on the main case and decided to add an exception for it.

As the initial rule was supposed to be based on the ARIA standards, I decided to dig a bit further and indeed the ARIA Authoring Practices Guide state for landmark regions the following:

banner, main, complementary and contentinfo landmarks should be top level landmarks.

Altho, it's quite confusing considering the next sentence is:

Landmark roles can be nested to identify parent/child relationships of the information being presented.

We can also easily find a lot of discussions like this one, or this one where the confusion is obvious even between members:

We do not have any nesting requirements in the ARIA specification for landmarks and only guidance in the ARIA practices.

Not sure what the benefit there is to screen reader users for an author to identify information that is complementary to the main content that is already part of the main content.

Landmarks are really about the screen reader experience to quickly identifty and navigate to the content areas of a website. Too many landmarks dilutes their utility.

complementary landmarks should be top level landmarks (e.g. not contained within any other landmarks).

I think the axe-core rule should stay. I think it's a good rule. And I think we can help authors by refining the mapping for the aside element, which I believe would make this better for users of AT.

Even tho some statement are contradictory, the rule was edited to make it more obvious that aside elements in the main landmark as it's a "non-normative 'restriction'".

For ARIA Practices, I think we need to clarify that complementary could be a child of main. I don't see why something like the following would be a problem, outside of it not meeting the non-normative 'restriction' that complementary needs to be a top-level landmark:

There are also stories about the W3C having this exact same issue themselves, e.g. when implementing the "Editor's note" note on this document:

The aside element was picked at the time as it was the only way for screen reader user to know where the Editor's Note ends and the actual Abstract text begins (this is supposedly now fixed in all screen readers).

Proposed change

Note that this proposal would require more testing.

Considering the existing confusion regarding this rule and as it was mentioned quite a bit of times in the links I shared for this kind of use case, we could move to the note role and remove the aside element:

A note role suggests a section whose content is parenthetic or ancillary to the main content.

This would remove the extra landmark and the original error while still providing a hint that is designed to help screen reader users understand the context and purpose.

It should be noted that this role is not reported in NVDA altho, this would be 1 specific reader issue, which could be solved at some point, instead of making navigation for screen reader users relying on landmarks navigation more difficult by having identical labels for multiple landmarks.

Tabs

I think this one is a bit more easy at first glance. Most tabs pattern example, even the one from the ARIA Authoring Practices Guide (APG) don't create an extra landmark.

Proposed change

Note that this proposal would require more testing.

We just replace the section element to avoid the extra landmark.

delucis · 2024-10-23T10:00:06Z

Haha, the history around the <aside> usage was very fun to read 😁

I think I’m in favour of both your proposed changes.

Additional motivation when I consider it a bit more is also that the usual usage of Starlight’s notes etc. is for that content to be locally complementary. It’s not really complementing <main> as a whole, so it’s not really helpful as far as I can see to be able to navigate directly to an <aside> or to move between them. To pick a random example, the note in this section of Starlight’s sidebar docs is relevant to that subheading, but jumping to it directly is not really a helpful experience. So that also supports avoiding the landmark role.

I’ll leave it up to you how you prefer to tackle this, whether to do the work directly in this PR or if you prefer to disable those audits in this PR, and we open an issue for these problems to PR the fixes & re-enabling the audits separately. Both would be fine approaches I think.

HiDeoo · 2024-10-23T13:49:32Z

Implemented some slug filtering to reduce the number of page tested that I mentioned in the PR description (open to discuss/ditch the idea if we decide it's not worth it or if we want to test all pages).

Here is a benchmark on my machine before the change (this include building the docs site):

❯ hyperfine --runs 5 -i 'pnpm test:a11y'
Benchmark 1: pnpm test:a11y
  Time (mean ± σ):     126.005 s ± 10.246 s    [User: 117.604 s, System: 7.940 s]
  Range (min … max):   113.814 s … 135.125 s    5 runs

And after the change:

❯ hyperfine --runs 5 -i 'pnpm test:a11y'
Benchmark 1: pnpm test:a11y
  Time (mean ± σ):     88.082 s ±  1.699 s    [User: 91.069 s, System: 6.393 s]
  Range (min … max):   86.485 s … 90.196 s    5 runs

For comparison, on CI, this change reduced the time from 3m 44s to 3m 2s.

Here is the pages I decided to keep and the reason why (these are tested for the default locale and ja):

Slug	Reason
`''` (index)	`splash` template
`components/*`	All dedicated component pages
`environmental-impact`	Include a Markdown table
`guides/authoring-content`	A huge mixed bag of content
`guides/authoring-content`	The theme editor
`guides/sidebar`	The sidebar preview component
`reference/configuration`	The largest reference page
`reference/icons`	Unique page content with a grid of icons
`resources/community`	Unique page content
`resources/plugins`	Unique page content
`resources/showcase`	Unique page content

All other pages are skipped as they don't really contain any unique content that is not already tested in the other pages. The skipped pages are explicitly defined which means that adding new pages will automatically be tested until we decide to opt-out of them.

HiDeoo · 2024-10-24T11:30:33Z

packages/starlight/__tests__/remark-rehype/rehype-tabs.test.ts

+const extractTabPanels = (html: string) => {
+	const tree = fromHtml(html, { fragment: true });
+	const tabPanels = selectAll('div[role="tabpanel"]', tree);
+	return tabPanels.map((tabPanel) => processor.stringify({ type: 'root', children: [tabPanel] }));
+};


Considering we're now using a <div> element and the tests contain nested <div> elements, I didn't want to rely and maintain a regular expression to extract the panel HTML 😅

HiDeoo · 2024-10-24T11:31:04Z

packages/starlight/integrations/asides.ts

@@ -202,7 +203,7 @@ function remarkAsides(options: AsidesOptions): Plugin<[], Root> {
 						),
 						...titleNode,
 					]),
-					h('section', { class: 'starlight-aside__content' }, node.children),
+					h('div', { class: 'starlight-aside__content' }, node.children),


I also removed the inner region landmark too as it doesn't bring any value imo. I think this landmark definition sums it up well:

Landmarks are meant to identify key areas of a page a user would most likely be interested in and would want to navigate to.

HiDeoo · 2024-10-24T11:33:01Z

Implemented the changes discussed above.

For the asides, here is the accessibility tree and a VoiceOver recording of the asides before and after the changes:

	Before	After
Accessibility tree
VoiceOver	https://github.com/user-attachments/assets/71c72bae-8146-4f2a-be4b-df5342fad23f	https://github.com/user-attachments/assets/2b7dcf7b-8bfe-4791-b9e9-bed8cddaa585

For the tabs, the changes don't affect the screen readers output.

HiDeoo · 2024-11-01T15:15:02Z

As we discussed, I moved the a11y tests to the docs/ directory.

I also tested the new changes on Windows using NVDA and JAWS. For tabs, everything is working as expected for both screen readers. For the asides, unfortunately, JAWS is working fine, but NVDA does not only not announce the new note role as I expected but also now entirely skips the aside title.

Back to the drawing board for asides 😢

HiDeoo · 2024-11-02T14:47:34Z

Following on my previous message, I continued research for the asides.

One interesting thing I found is this proposal by Lea Verou for a new <callout> element for callouts/alerts/admonitions which definitely highlights some of our issues.

This comment from Matthew Tylee Atkinson, an accessibility consultant, pinged by Lea Verou in the issue as an a11y expert, also confirms some previous points:

However, a document structuring role (such as note or article) that doesn't create a landmark region would be best. Because the callout is an integral part of the text in which it lives, it shouldn't be a separate landmark region[...]

If a document had many notes, and they used landmark roles, that would put a lot of noise into the landmark structure and navigation

I also found this interesting comparison between various callouts, e.g. AsciiDoc vs reStructuredText vs Obsidian vs MDN vs Docusaurus vs W3C. It basically highlights the same issues and conclusions that we have been discussing.

Then I decided to take a look at some implementations of asides. I used The Component Gallery altho it's a bit tricky as asides don't have a dedicated category and usually bundled with alerts.

From my research, I think there are mostly 3 types of implementations:

No special markup
List
Looks like asides but use a status or error role (not really a good idea in our case)
List
Use a <section> (which is the same as the initial issue we're trying to solve)
List

So here are the various possibilities I see so far:

Have the role="note" and an aria-label on the outer <div>:

NVDA will not announce the role and also skips the label as NVDA only honors aria-label for certain roles. I think this is too much of an issue to just ignore it and wait for NVDA to fix it (the issue is opened since late 2019).
Have the role="note" with no aria-label on the outer <div>, move the aria-hidden to the <Icon>:

The role is not announced by any screen reader, but all the content is announced. Even if not used right now, the note role is in the accessibility tree but a bit useless.
No role on the outer <div> and an aria-label:

No role is announced by any screen reader, but all the content is announced. Screen reader users don't really know where the aside ends.
Keep using an <aside> and have extra landmarks:

We now know this is an issue, and kinda invalid. A role is announced by all screen readers. This would require disabling the associated rule.

So far, in the current state of things, I'm leaning towards the last option. It's not the best but maybe the "more accessible" version across all screen readers. As we also tend to avoid asides, I think this goes in the same direction, but of course we have no control over the user's content.

When nvaccess/nvda#10439 is fixed (I'm subscribed to it), we can revisit the first option which would be the best until, if ever, a <callout> element is introduced.

The second best option would be the option 3 I guess, which is widely used.

Open to any feedback or suggestions.

* main: (82 commits) i18n(ja): Update site-search.mdx (withastro#2577) i18n(ja): Update pages.mdx (withastro#2576) i18n(fr): Update `reference/plugins.mdx` from withastro#2549 (withastro#2574) [ci] format docs: update showcase-sites.astro (withastro#2562) Throw an error if a showcase image does not have the required dimensions (withastro#2573) i18n(ja): Update frontmatter.md (withastro#2566) Fixes import name typo in Customize icons example (withastro#2572) [ci] release (withastro#2570) Prettier ignore a malformed YAML test file (withastro#2571) Add YAML support to the FS translation system (withastro#2565) i18n(ja): Update reference/icons.mdx (withastro#2564) i18n(ja): Update overrides.md (withastro#2567) i18n(zh-cn): Update `plugins.mdx` (withastro#2568) i18n(ja): Update customization.mdx (withastro#2560) i18n(ja): Update css-and-tailwind.mdx (withastro#2559) i18n(ru): update `icons.mdx` and `overrides.md` (withastro#2558) i18n(ru): update `i18n.mdx` and `overriding-components.mdx` (withastro#2557) i18n(ko-KR): update `plugins.mdx` (withastro#2556) i18n(ru): update `plugins.mdx` (withastro#2555) ...

delucis · 2024-11-06T10:46:39Z

Just noting here before I forget that we chatted on Monday and agreed that probably the best (if still imperfect) approach is to maintain use of the <aside> landmark given the bugs with alternative implementation options.

Basically the trade-off is:

Either use the landmark, somewhat polluting the page’s list of landmark elements, and potentially causing issues with duplicate landmark labels when there are multiple asides of the same type using the default label.
Or use regular elements (with no indication of semantics like role="note"), which reduces the landmark clutter, but makes it impossible for AT to communicate where an aside ends and regular page content resumes.

On balance, we agreed the first option is probably the best compromise for now and we can always revisit this base on future user feedback.

HiDeoo · 2024-11-06T10:55:16Z

Updated the PR with the following changes:

Partially reverted the aside changes (the inner region landmark (<section>) is still removed)
Added an ignore mechanism to the a11y tests
- A rule can be entirely ignored (unused but ready for future use if needed)
- A rule can be partially ignored to exclude certain elements for a specific rule
The landmark-unique rule is now partially ignored and does not apply to asides. Other elements are still checked against this rule.

delucis

Looks good to me! Ready to merge when we release the next minor.

Thanks again for the careful work researching this behaviour with different screenreaders to help us arrive at the best approach we can for now 💖

* main: (27 commits) i18n(ko-KR): update `site-search.mdx` [ci] release (withastro#2590) Support passing more options to the DocSearch component (withastro#2589) [ci] release (withastro#2587) Add changeset for withastro#2252 (withastro#2588) [ci] format Update dev dependencies (withastro#2582) Build performance optimizations for projects with large sidebars (withastro#2252) Fix a11y CI workflow (withastro#2503) Update `astro-expressive-code` to v0.38 (withastro#2551) Added social icon for Nostr (withastro#2579) [ci] format docs(showcase): add docs.reactbricks.com to showcase (withastro#2586) i18n(ja): Update site-search.mdx (withastro#2577) i18n(ja): Update pages.mdx (withastro#2576) i18n(fr): Update `reference/plugins.mdx` from withastro#2549 (withastro#2574) [ci] format docs: update showcase-sites.astro (withastro#2562) Throw an error if a showcase image does not have the required dimensions (withastro#2573) i18n(ja): Update frontmatter.md (withastro#2566) ...

HiDeoo added 2 commits October 22, 2024 16:04

ci: refactor a11y tests

21b68ad

ci: remove pa11y-ci

757b1ed

github-actions bot added 🚨 action Changes to GitHub Action workflows 📚 docs Documentation website changes 🌟 core Changes to Starlight’s main package labels Oct 22, 2024

HiDeoo added 3 commits October 22, 2024 16:33

ci: increase a11y build step timeout

e0f2870

test: exclude a11y tests from coverage

dc43808

test: tweak a11y test output

398aa45

HiDeoo commented Oct 22, 2024

View reviewed changes

test: reduce the number of pages testeed for a11y issues

c853ccb

HiDeoo added 4 commits October 24, 2024 11:20

feat: remove aside landmarks

480fd3d

feat: remove tab landmarks

0076726

ci: add feedback during a11y tests

c8e2df4

chore: add changeset

033be42

HiDeoo commented Oct 24, 2024

View reviewed changes

test: move a11y tests to docs/

98562a3

HiDeoo added 2 commits November 6, 2024 10:04

fix: restore <aside> element

976005d

test: add a11y ignore system and ignore landmark-unique for asides

8d614df

HiDeoo marked this pull request as ready for review November 6, 2024 10:55

delucis approved these changes Nov 6, 2024

View reviewed changes

delucis added the 🌟 minor Change that triggers a minor release label Nov 8, 2024

Merge branch 'main' into pr/2503

5f5eaeb

delucis merged commit a4c8edd into withastro:main Nov 8, 2024
16 checks passed

astrobot-houston mentioned this pull request Nov 8, 2024

[ci] release #2587

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix a11y CI workflow #2503

Fix a11y CI workflow #2503

HiDeoo commented Oct 22, 2024 •

edited

Loading

changeset-bot bot commented Oct 22, 2024 •

edited

Loading

netlify bot commented Oct 22, 2024 •

edited

Loading

HiDeoo Oct 22, 2024

delucis commented Oct 22, 2024 •

edited

Loading

HiDeoo commented Oct 23, 2024 •

edited

Loading

delucis commented Oct 23, 2024

HiDeoo commented Oct 23, 2024 •

edited

Loading

HiDeoo Oct 24, 2024

HiDeoo Oct 24, 2024

HiDeoo commented Oct 24, 2024

HiDeoo commented Nov 1, 2024

HiDeoo commented Nov 2, 2024 •

edited

Loading

delucis commented Nov 6, 2024

HiDeoo commented Nov 6, 2024

delucis left a comment

Fix a11y CI workflow #2503

Fix a11y CI workflow #2503

Conversation

HiDeoo commented Oct 22, 2024 • edited Loading

changeset-bot bot commented Oct 22, 2024 • edited Loading

🦋 Changeset detected

netlify bot commented Oct 22, 2024 • edited Loading

✅ Deploy Preview for astro-starlight ready!

HiDeoo Oct 22, 2024

Choose a reason for hiding this comment

delucis commented Oct 22, 2024 • edited Loading

Asides

Tabs

HiDeoo commented Oct 23, 2024 • edited Loading

Asides

Proposed change

Tabs

Proposed change

delucis commented Oct 23, 2024

HiDeoo commented Oct 23, 2024 • edited Loading

HiDeoo Oct 24, 2024

Choose a reason for hiding this comment

HiDeoo Oct 24, 2024

Choose a reason for hiding this comment

HiDeoo commented Oct 24, 2024

HiDeoo commented Nov 1, 2024

HiDeoo commented Nov 2, 2024 • edited Loading

delucis commented Nov 6, 2024

HiDeoo commented Nov 6, 2024

delucis left a comment

Choose a reason for hiding this comment

HiDeoo commented Oct 22, 2024 •

edited

Loading

changeset-bot bot commented Oct 22, 2024 •

edited

Loading

netlify bot commented Oct 22, 2024 •

edited

Loading

delucis commented Oct 22, 2024 •

edited

Loading

HiDeoo commented Oct 23, 2024 •

edited

Loading

HiDeoo commented Oct 23, 2024 •

edited

Loading

HiDeoo commented Nov 2, 2024 •

edited

Loading