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 warnings for missing alt-text and auto-generated alt-text #1814

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

JimMadge
Copy link

This PR adds two warnings with rules allowing them to be disabled,

  • Warning for images missing alt-text (image-has-alt-text)
  • Warning for images where alt-text was auto-generated, by checking for the altTextIsAutoGenerated tag (image-alt-text-generated)

Example output,

> npx myst build -d
...
⚠️  index.md missing alt text for ./cat1.jpg
To suppress this message, add rule: "image-has-alt-text" to "error_rules" in your project config
⚠️  index.md alt text for ./cat2.jpg was auto-generated
   You can remove this warning by writing your own alt text
To suppress this message, add rule: "image-alt-text-generated" to "error_rules" in your project config
...

I have a example project to test this, but haven't added a test here.

Copy link

changeset-bot bot commented Jan 24, 2025

🦋 Changeset detected

Latest commit: be7097f

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

This PR includes changesets to release 5 packages
Name Type
myst-transforms Patch
myst-common Patch
myst-config Patch
myst-frontmatter Patch
myst-spec-ext Patch

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

@rowanc1
Copy link
Member

rowanc1 commented Jan 24, 2025

I am not sure how these will behave for notebooks, etc. Which has limited/no way to add alt text to for outputs at the moment, and so there would be many warnings you can't get rid of.

Thoughts on this being opt-in vs opt out?

@rowanc1 rowanc1 changed the title Add warnings for missing alt-text and auto-generated alt-text ⚠️ Add warnings for missing alt-text and auto-generated alt-text Jan 24, 2025
@agoose77
Copy link
Contributor

@rowanc1 that's an interesting point. Jim and I were thinking about hand authored markup. What about stopping at output nodes, and not recursing into their children? Then, as long as this happens before embeds, we're OK?

Incidentally, can't we use block labels (#| foo) to add alt? That might be useful (feature request I think).

@choldgraf
Copy link
Collaborator

I think a nice short term solution would be to not add this warning for output nodes, and to document that a good strategy is to use embed logic to get nicer things like captions, alt text, etc for notebook outputs

@agoose77
Copy link
Contributor

@JimMadge I've updated this PR to use a different unist utility called unist-util-visit. This lets us inspect the parents as we recurse, although I prefer to switch on output or image nodes, and explicitly stop recursion before we visit the leaves.

I've also added a test file. We didn't have time to add that when we paired on this PR. We use the vitest test framework, which is fairly self-explanatory (and there's good prior art in the wealth of tests that we already have). The .spec.ts files are picked up by the test runner automatically.

@rowanc1 are you comfortable merging? We could opt to loosen the strictness by disabling the generated alt-text (and/or splitting it into a separate transform) until such time as we can default opt-out of certain warnings.

@JimMadge
Copy link
Author

I don't have strong feelings about opt-in or opt-out. On one hand, alt text is important for accessibility and it would be good to promote using it. On the other, it could make output very noisy and push people to ignore the warning.

Thanks for the improvements 🚀.

@rowanc1
Copy link
Member

rowanc1 commented Jan 27, 2025

There is the capability to stop warnings for a certain image, if we pass the key through. (That is via addWarningForFile not fileWarn though (these are logged in logMessagesFromVFile), so we ned a way to pass the key through to that function on the VFileMessage.) Happy to jump in and make that change.

This would enable turning on/off on a per-image basis, for example, if you are using a decorative image that does not have alt text, but want to warn/error on the rest.

My preference is to change this to opt-in. I am more swayed by the it could make output very noisy and push people to ignore the warning part of the argument. We are also currently not enforcing any other best practices in warnings, only content errors and syntax-related requirements. If it moves to opt-in, we should add a section in the docs.

@JimMadge
Copy link
Author

That all sounds good to me. I would also be happy to work on those things if, perhaps, @agoose77 can spare some time to mentor me on it.

@agoose77
Copy link
Contributor

agoose77 commented Jan 27, 2025

Thanks for the helpful wider context, @rowanc1!

It looks like we have two separate enhancements:

  1. Support per-file selection of this rule
  2. Add machinery for disabling specific errors at the distribution level #1818

Both sound useful. @rowanc1, RE

Happy to jump in and make that change

I'm not 100% following the suggestion, but I understand that we have session-free and session-aware logging utilities. If you're happy to make that change at some point, that would be nice. I can put some cycles in this week to think about specifying a default set of error rules that merges with the user configuration.

There isn't in my mind a rush to get this in, besides not letting it stagnate!

@rowanc1
Copy link
Member

rowanc1 commented Jan 27, 2025

I will jump in and make it possible to add a key this afternoon!

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.

4 participants