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

Accessibility: new check for image alt text #1651

Closed
beckykd opened this issue Jul 3, 2024 · 5 comments · Fixed by #2349
Closed

Accessibility: new check for image alt text #1651

beckykd opened this issue Jul 3, 2024 · 5 comments · Fixed by #2349
Assignees

Comments

@beckykd
Copy link
Collaborator

beckykd commented Jul 3, 2024

All images need to have alt="some alt text" , like in the following example

Stratified circuit illustration. There are arbitrary single-qubit gates between each `layer`. Each layer is defined by a block that crosses multiple qubit wires.

@Eric-Arellano Eric-Arellano changed the title Suggestion: New check to make sure alt text has been added for images New check for image alt text Jul 3, 2024
@shraddha-aangiras
Copy link
Contributor

Hi @Eric-Arellano, is this issue similar to this one where simply alt text is to be added, or is a new check to be implemented?
If a check is to be created, could you give me an outline on where it should be done? Would love to work on this issue either way :)

@Eric-Arellano
Copy link
Collaborator

Thanks for offering, @shraddha-aangiras! That would be an enormous help. Alt text is crucial for accessibility.

This issue is about adding a generic check, whereas #1659 is fixing all the current violations we have. This issue is an infra task, whereas 1659 is a content task that the technical writers can help with.

Here are some thoughts on how you could add this check:

  1. Add a new files scripts/commands/checkAltText.ts, scripts/lib/extractAltText.ts, and scripts/lib/extractAltText.ts
    • For now, they should only have the copyright notice with the year set to 2024
  2. Update package.json, .github/workflows/main.yml, and README.md to define and use a new command check:alt-text. Look at check:metadata for an example. Make sure that check runs the new command.
  3. Implement extractAltText.ts and extractAltText.test.ts by defining a new async function extractAltText(markdown: string): Promise<AltText>. AltText can be an interface that has something like {imageName: string; altText: string | null}.
    • Note that the function intentionally takes a string as the arg rather than a file path. That makes it easier to test.
    • Your test should have some valid examples with alt text and some without
    • See this example. I think yours will look the same but you only have the image visit, and you need to get the alt text
      export async function parseLinks(
      markdown: string,
      ): Promise<[Set<string>, Set<string>]> {
      const internalLinks = new Set<string>();
      const externalLinks = new Set<string>();
      const addLink = (link: string): void => {
      if (link.startsWith("http")) {
      externalLinks.add(link);
      } else {
      internalLinks.add(link);
      }
      };
      await unified()
      .use(rehypeParse)
      .use(remarkGfm)
      .use(rehypeRemark)
      .use(() => (tree: Root) => {
      visit(tree, "text", (TreeNode) => {
      markdownLinkExtractor(String(TreeNode.value)).links.forEach((url) =>
      addLink(url),
      );
      });
      visit(tree, "link", (TreeNode) => addLink(TreeNode.url));
      visit(tree, "image", (TreeNode) => addLink(TreeNode.url));
      })
      .use(remarkStringify)
      .process(markdown);
      return [internalLinks, externalLinks];
      }
  4. Implement checkAltText.ts
    • It should run on all files in guides/ and api/migration-guides. (Not the rest of api/). You can use globby for this
    • It should call readMarkdown() from https://github.com/Qiskit/documentation/pull/1666/files and extractAltText from the prior step. Do this for every source file
    • It should have an allowlist mechanism
    • See the other files in scripts/commands for inspiration, like checkMetadata.ts

How does this sound @shraddha-aangiras? Would you like me to assign you?

@shraddha-aangiras
Copy link
Contributor

Thank you so very much for the comprehensive breakdown @Eric-Arellano! It helped me understand how to implement this and also how the checks like these work. I would love to work on this, please assign this to me! Thanks again!

@Eric-Arellano
Copy link
Collaborator

Great! Let me know if you have questions, and also feel free to open a draft PR with [wip] in the PR title if you want feedback on your progress.

@shraddha-aangiras
Copy link
Contributor

@Eric-Arellano Thank you so much!

github-merge-queue bot pushed a commit that referenced this issue Jul 10, 2024
We're going to reuse this logic in
#1651, so it's helpful to
have in a common helper file.
frankharkins pushed a commit to frankharkins/documentation that referenced this issue Jul 22, 2024
We're going to reuse this logic in
Qiskit#1651, so it's helpful to
have in a common helper file.
@Eric-Arellano Eric-Arellano moved this to In Progress in Docs Planning Aug 5, 2024
@Eric-Arellano Eric-Arellano changed the title New check for image alt text Accessibility: new check for image alt text Nov 4, 2024
@Eric-Arellano Eric-Arellano moved this from In Progress to Blocked in Docs Planning Nov 18, 2024
@javabster javabster removed the status in Docs Planning Nov 18, 2024
github-merge-queue bot pushed a commit that referenced this issue Nov 21, 2024
This pull request adds an image checker to ensure all images have an alt
text defined, which is crucial for accessibility, and that we don't have
any `<img>` HTML tag. The output of the check shows the file name that
contains invalid images and the images' names. Ex:
```
Error in file 'docs/guides/custom-transpiler-pass.ipynb':
	- The image '/images/guides/custom-transpiler-pass/DAG.png' does not have alt text.

Invalid images found 💔 See https://github.com/Qiskit/documentation#images for instructions.
```
The PR builds on the work done by @shraddha-aangiras on
#1800. I have made some
changes to simplify the code and incorporated @Eric-Arellano's feedback.
Thank you both for the work you've done!

Closes #1651

---------

Co-authored-by: Eric Arellano <[email protected]>
Co-authored-by: Shraddha Aangiras <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment