feat(image): throw if alt text is missing #4511

DerYeger · 2022-08-27T10:51:48Z

Changes

@astrojs/image should throw an error if alt text is missing #4291
<Image> and <Picture> now throw if alt prop is missing
Prop types have been updated to require alt

Testing

This change was tested by adding fixtures that have images without alt text.
The tests assert that an error is thrown.

Docs

PR: withastro/docs#1421

changeset-bot · 2022-08-28T06:52:06Z

🦋 Changeset detected

Latest commit: 618834e

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

This PR includes changesets to release 1 package

Name	Type
@astrojs/image	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

sarah11918 · 2022-08-29T17:43:06Z

packages/integrations/image/README.md

+Astro's `<Image />` and `<Picture />` components will throw an error if no `alt` text is provided. 
+
+If the image is merely decorative (i.e. doesn't contribute to the understanding of the page), set `alt=""` so it is ignored by screen readers.
+


LOVE this new feature, @DerYeger! I have installed some Chrome extensions myself to make sure I'm not allowed to Tweet without alt-text on my images. 😄

Please ALSO check the "Usage" heading further down the page where the attributes, and whether or not they are required are listed for both components. There should be an alt heading and description for each one of them. It looks like it's currently not there for <Image /> but is there for <Picture /> though only saying "if it's provided."

These should both prominently display the new info, and double check all the examples to make sure they all show alt= on every component.

Lastly, @DerYeger & @tony-navillus, what exactly is the error message that is thrown? (Sorry, hard to look through everything on my phone here) If it's not, can it be an explicit message to add alt text rather than a generic error?

Thanks!

Hi, I used the suggestion of #4291 (comment) as the error message, replacing image with picture for the respective component.

Great! I've merged the PR for the docs "Images Guide" page just now. Just let me know if you're up for making the above edits under "Usage" (as an attribute, like all the other aspectRatio, src etc.) in addition to your note here, or whether you'd like me to take care of that! But, it DOES need to be there as an attribute for each component.

These changes don't appear to have been made, so I'm making them now here!

tony-sull

This is excellent, thanks for the thorough test coverage!

Consider the feature work here 🎉 @sarah11918 I'll leave the docs approval up to you if you don't mind, once it has your approval as well I'll get this merged in and ready for release 🚀

sarah11918 · 2022-08-29T21:30:20Z

Yay! Will look closely when I am back at a laptop!

tony-sull · 2022-08-30T21:15:20Z

@DerYeger Let me know if you have any issues with the merge conflict!

tony-sull · 2022-08-31T15:52:20Z

I'll get those tests passing, looks like the tests in main are too aggressive with hard-coded filename hashes

sarah11918 · 2022-08-31T17:36:02Z

@tony-sull Ooh, good! I need to make a fix to this README anyway. Please do not merge this until I've updated the docs here. 💜

sarah11918 · 2022-08-31T17:47:51Z

OK @tony-sull PLEASE check all my alt-text stuff carefully! Code samples! etc! Please make sure it's actually correct and no typos. 💜 Otherwise, I'm happy.

DerYeger · 2022-08-31T18:46:12Z

I'll get those tests passing, looks like the tests in main are too aggressive with hard-coded filename hashes

Thanks! Let me know I can help out anywhere else.

tony-sull · 2022-08-31T21:08:44Z

Just opened #4579 with a big cleanup of the image tests. This PR was a perfect kick in the pants for me to stop dragging my feet on refactoring all the test code 😄

FredKSchott

Looks great! I had a few comments, mainly around documentation. Thanks for pushing us to add this, I'm a big +1 for this feature.

packages/integrations/image/README.md

FredKSchott · 2022-09-01T13:16:32Z

packages/integrations/image/components/Image.astro

@@ -21,6 +23,10 @@ export type Props = LocalImageProps | RemoteImageProps;

 const { loading = 'lazy', decoding = 'async', ...props } = Astro.props as Props;

+if (props.alt === undefined || props.alt === null) {
+ throw new Error('The <Image> component requires you provide alt text. If this image does not require an accessible label, set alt="".')


link to more reading in case this is a new concept for the reader! https://developer.mozilla.org/en-US/docs/Web/HTML/Element/img#attr-alt would probably be good.

FYI no need to worry too much about the final formatting here for this error message, as long as all of the info is correct. @tony-sull or myself will plan to finalize this ourselves with correct formatting before merging the PR, to match other errors in our codebase.

FredKSchott · 2022-09-01T13:18:17Z

packages/integrations/image/components/Image.astro

 }

 interface RemoteImageProps extends TransformOptions, astroHTML.JSX.ImgHTMLAttributes {
 src: string;
+ alt: string;


Can we add a helpful warning here as JSDoc as well? Something like:

/** Defines an alternative text description of the image. Set to an empty string (alt="") if the image is not a key part of the content (it's decoration or a tracking pixel). **/ alt: string;

Ditto for the LocalImageProps type above.

FredKSchott · 2022-09-01T13:37:24Z

@tony-sull @DerYeger I'm realizing that this will break most existing users who didn't add alt text, and my guess is that this is a lot of them. Even if we make this error helpful, that's still going to be a real pain to upgrade given that we won't even render the page anymore if alt is missing.

Thoughts on making this a TS-only change first, and then adding the runtime error in a future minor release? This will still provide a very visible error in astro check and visible inline errors in their code editor, but it will also give them some time to update before we escalate.

FYI to implement this change, everything about this PR can stay as-is except for removing the ~3 lines that throw the error at runtime.

matthewp · 2022-09-01T13:46:32Z

@FredKSchott are you proposing essentially we warn in 1 "major" (minor in this case because < 1.0) release and then break in the next? If so I think that's fine and in line with some other changes we've done.

tony-sull · 2022-09-01T15:21:31Z

@FredKSchott That's a good callout, I like the idea of warning for now and throwing the error in a later release (either a future minor or 1.0)

@DerYeger The tests should be almost there now, I refactored all the image tests to avoid using hard-coded filename hashes but it looks like I missed an alt tag in the with-mdx test suite 🤦

tony-sull · 2022-09-01T17:36:15Z

@FredKSchott @sarah11918 I pushed a few updates here to include the comments above. Any additional feedback before merging this in?

For the warning, I wrapped it in a helper to make sure it's only logged once per astro dev or astro build. Logging a warning for every component could get very frustrating 😅

DerYeger · 2022-09-01T17:43:00Z

@FredKSchott @sarah11918 I pushed a few updates here to include the comments above. Any additional feedback before merging this in?

For the warning, I wrapped it in a helper to make sure it's only logged once per astro dev or astro build. Logging a warning for every component could get very frustrating 😅

That’s a great change, thanks for cleaning up this PR!

tony-sull · 2022-09-01T21:24:12Z

Thanks for this PR, @DerYeger! Excellent test coverage and docs updates 🎉

Very excited to see this flipped over to a build error in the future, I'm always a fan of build tools making sure I don't accidentally create a11y issues!

FredKSchott · 2022-09-02T01:48:53Z

@FredKSchott are you proposing essentially we warn in 1 "major" (minor in this case because < 1.0) release and then break in the next? If so I think that's fine and in line with some other changes we've done.

Yup, pretty much! Glad everyone was on board for this, sgtm and excited to see this merged!

DerYeger added 2 commits August 27, 2022 12:38

feat(image): throw if no alt is provided

1e26c6f

chore: add changeset

2001c4b

github-actions bot added the pkg: integration Related to any renderer integration (scope) label Aug 27, 2022

DerYeger mentioned this pull request Aug 27, 2022

docs(image): add note about error if alt is missing withastro/docs#1421

Merged

DerYeger marked this pull request as ready for review August 27, 2022 11:01

docs(image): update README

dfc38d9

bluwy linked an issue Aug 29, 2022 that may be closed by this pull request

@astrojs/image should throw an error if alt text is missing #4291

Closed

1 task

sarah11918 reviewed Aug 29, 2022

View reviewed changes

tony-sull approved these changes Aug 29, 2022

View reviewed changes

DerYeger and others added 2 commits August 31, 2022 15:18

Merge branch 'main' into main

4d4ea72

Merge branch 'main' into main

f697cf9

updated alt text stuff throughout

f1d1113

FredKSchott reviewed Sep 1, 2022

View reviewed changes

Merge branch 'main' into main

0f33492

Tony Sullivan added 3 commits September 1, 2022 10:55

fixing with-mdx test suite

e6c1bb0

warn for missing alt text, will throw an error in a future release

a393af9

final README tweaks

0f01097

merging latest astro main

618834e

tony-sull merged commit 72c760e into withastro:main Sep 1, 2022

astrobot-houston mentioned this pull request Sep 1, 2022

[ci] release #4589

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(image): throw if alt text is missing #4511

feat(image): throw if alt text is missing #4511

DerYeger commented Aug 27, 2022 •

edited

Loading

changeset-bot bot commented Aug 28, 2022 •

edited

Loading

sarah11918 Aug 29, 2022

DerYeger Aug 29, 2022

sarah11918 Aug 29, 2022

sarah11918 Aug 31, 2022

tony-sull left a comment

sarah11918 commented Aug 29, 2022

tony-sull commented Aug 30, 2022

tony-sull commented Aug 31, 2022

sarah11918 commented Aug 31, 2022

sarah11918 commented Aug 31, 2022

DerYeger commented Aug 31, 2022

tony-sull commented Aug 31, 2022 •

edited

Loading

FredKSchott left a comment

FredKSchott Sep 1, 2022

FredKSchott Sep 1, 2022

FredKSchott commented Sep 1, 2022

matthewp commented Sep 1, 2022

tony-sull commented Sep 1, 2022

tony-sull commented Sep 1, 2022

DerYeger commented Sep 1, 2022

tony-sull commented Sep 1, 2022

FredKSchott commented Sep 2, 2022

		Astro's `<Image />` and `<Picture />` components will throw an error if no `alt` text is provided.

		If the image is merely decorative (i.e. doesn't contribute to the understanding of the page), set `alt=""` so it is ignored by screen readers.

feat(image): throw if alt text is missing #4511

feat(image): throw if alt text is missing #4511

Conversation

DerYeger commented Aug 27, 2022 • edited Loading

Changes

Testing

Docs

changeset-bot bot commented Aug 28, 2022 • edited Loading

🦋 Changeset detected

sarah11918 Aug 29, 2022

Choose a reason for hiding this comment

DerYeger Aug 29, 2022

Choose a reason for hiding this comment

sarah11918 Aug 29, 2022

Choose a reason for hiding this comment

sarah11918 Aug 31, 2022

Choose a reason for hiding this comment

tony-sull left a comment

Choose a reason for hiding this comment

sarah11918 commented Aug 29, 2022

tony-sull commented Aug 30, 2022

tony-sull commented Aug 31, 2022

sarah11918 commented Aug 31, 2022

sarah11918 commented Aug 31, 2022

DerYeger commented Aug 31, 2022

tony-sull commented Aug 31, 2022 • edited Loading

FredKSchott left a comment

Choose a reason for hiding this comment

FredKSchott Sep 1, 2022

Choose a reason for hiding this comment

FredKSchott Sep 1, 2022

Choose a reason for hiding this comment

FredKSchott commented Sep 1, 2022

matthewp commented Sep 1, 2022

tony-sull commented Sep 1, 2022

tony-sull commented Sep 1, 2022

DerYeger commented Sep 1, 2022

tony-sull commented Sep 1, 2022

FredKSchott commented Sep 2, 2022

DerYeger commented Aug 27, 2022 •

edited

Loading

changeset-bot bot commented Aug 28, 2022 •

edited

Loading

tony-sull commented Aug 31, 2022 •

edited

Loading