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

Update a11y.ts #9567

Merged
merged 5 commits into from
Jan 17, 2024
Merged

Update a11y.ts #9567

merged 5 commits into from
Jan 17, 2024

Conversation

OliverSpeir
Copy link
Contributor

@OliverSpeir OliverSpeir commented Jan 2, 2024

Accidentally deleted PR #9507
Closes Issue #9551

Changes

  • Aligns missing content rule with WCAG and deque/axe-core ( used by lighthouse )
  • Allows for aria-label, aria-labelledby, img with alt, svg with title
  • Changes Error title and Message to be more clear and provide info how to solve the error

Testing

Manually tested with @examples/blog

Docs

  • No doc update needed

Copy link

changeset-bot bot commented Jan 2, 2024

🦋 Changeset detected

Latest commit: 41024d0

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

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

@github-actions github-actions bot added pkg: astro Related to the core `astro` package (scope) pr: docs A PR that includes documentation for review labels Jan 2, 2024
Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

The fix looks good to me, although it seems that the rule doesn't check for missing alt for input tag with type image, for example.

Should this be implemented in this PR or another? Here's a reference: https://biomejs.dev/linter/rules/use-alt-text/

@OliverSpeir
Copy link
Contributor Author

OliverSpeir commented Jan 2, 2024

The fix looks good to me, although it seems that the rule doesn't check for missing alt for input tag with type image, for example.

Should this be implemented in this PR or another? Here's a reference: https://biomejs.dev/linter/rules/use-alt-text/

Good catch didn't even think about inputs although I don't know why someone would have a <a> or <h1> input it is valid and accessible HTML and this also aligns with deque/axe-core

Copy link
Member

@natemoo-re natemoo-re left a comment

Choose a reason for hiding this comment

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

Logic looks good other than the querySelectorAll parts!

@OliverSpeir OliverSpeir requested a review from natemoo-re January 2, 2024 23:13
@ematipico ematipico added this to the 4.2.0 milestone Jan 4, 2024
Copy link
Member

@sarah11918 sarah11918 left a comment

Choose a reason for hiding this comment

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

Just a suggestion for your consideration!

.changeset/orange-trainers-learn.md Outdated Show resolved Hide resolved
message: 'Headings and anchors must have content to be accessible.',
title: 'Missing content',
message:
'Headings and anchors must have an accessible name, which can come from: inner text, aria-label, aria-labelledby, an img with alt property, or an svg with a tag <title></title>.',
Copy link
Member

Choose a reason for hiding this comment

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

Just checking because I don't think these error messages currently get pulled into docs, so this should be fine. But for docs, we would be using back ticks for inline code formatting on all values, but especially any components like <title> or else docs will break.

@Princesseuh - tangentially, do we want these dev tool error messages available in docs somewhere? Eventually? We don't really have any "using dev toolbar" docs at all. Is this something we want/care about?

Copy link
Member

@Princesseuh Princesseuh Jan 12, 2024

Choose a reason for hiding this comment

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

Sorry, missed this message (blame GitHub notifications not working properly since a few weeks 😠). Yes, eventually I would like to have the audits in the docs, similar to the error reference.

It would most likely use JSDoc comments like the error reference do, so the messages would get sanitized etc. (or we'd duplicate every error in the docs, that's ok too)

@sarah11918 sarah11918 removed the pr: docs A PR that includes documentation for review label Jan 8, 2024
Copy link
Member

@sarah11918 sarah11918 left a comment

Choose a reason for hiding this comment

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

Docs is happy with the changeset!

Copy link
Member

@natemoo-re natemoo-re left a comment

Choose a reason for hiding this comment

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

Looks great, thank you!

@ematipico ematipico merged commit 3a4d5ec into withastro:main Jan 17, 2024
13 of 14 checks passed
@astrobot-houston astrobot-houston mentioned this pull request Jan 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

False positive missing content audit on links with svg title
6 participants