Add HTML support for MD045: no-alt-text

[khiga8]

Issue: #992

This PR adds support to flag missing alt text on HTML images. I wasn't sure if I should hold off on opening the PR until there was more discussion on the issue so this might be premature... I am happy to continue discussing this on this PR or on the issue.

Approach taken

I am new to this repo and followed the approaches I saw in other rules.

I noticed there is parser that returns HTML tokens. I didn't see a way to access the attributes off the HTML. It may not be ideal but I ended up using regex to check if the HTML image token contains alt=.

[DavidAnson]

This looks pretty thorough and I like the test cases I see. Thank you! I will need to review this from a real computer, potentially this evening. I will have a set of nitpicks as always, so be warned. :)

A few things I noticed from a very quick scan on my phone:

• Element and attribute comparisons should be case insensitive, this suggests there are no test cases for uppercase
• I do not understand how range can be calculated after removing line breaks, this may mean there is something of a gap in the tests
• If you convert this to a real PR, we can see the result of CI, you can get close locally with "npm run ci"
• There are three copies of the documentation and you modified the correct one, but did not run the npm script to update the other two
• I include a space between rule names in test files {MD001} {MD002}
• I might choose to add the alternate text tag to the couple of unrelated test files instead of suppressing this rule as you did

[khiga8]

Hi @DavidAnson, I appreciate your feedback! I've addressed the following from your first set of feedback:

Element and attribute comparisons should be case insensitive, this suggests there are no test cases for uppercase

There are three copies of the documentation and you modified the correct one, but did not run the npm script to update the other two

I include a space between rule names in test files {MD001} {MD002}

I might choose to add the alternate text tag to the couple of unrelated test files instead of suppressing this rule as you did

With regards to:

I do not understand how range can be calculated after removing line breaks, this may mean there is something of a gap in the tests

I followed the approach taken in MD033.js which was one of the only other rules I could find that sets an error on an HTML tag. I believe this sets the error range on the first line of the tag and accounts for multiline tags. My understanding could be wrong and I am definitely open to your suggestions.

[DavidAnson]

Thanks for the quick activity! I will remind myself how the other range calculation works when I review this.

[DavidAnson]

FYI, GitHub is complaining this PR cannot be merged due to conflicts and I think that may be blocking me from approving a CI run?

[DavidAnson]

FYI now that tests are running, if anything new pops up in TestRepos, I typically verify all new issues are valid before updating those snapshots. There are npm scripts to clone and lint and update.

[DavidAnson]

Looks solid, thank you! Most of my comments are trivial nitpicks.

[khiga8]

Hi @DavidAnson! Apologies for the delayed follow-up.

Starting from commit 0165cf5, I've hopefully addressed all of your feedback, but let me know if I'm missing anything.

Re: #993 (comment)

Please let me know if you did not visually verify each one of these and I will.

I've visually verified the repo snapshot flags with the following steps:

1. In my VSCode editor, I did a regex search of <img and filtered to test-repos/**/*.md. This gives me 19 results across 5 files.
2. In the snapshots, there are 9 new image violations being flagged.
3. I've verified that each of the 9 flagged violations are legit violations. I saw that the remaining 10 images all have alt text on it.

[DavidAnson]

Thank you for verifying the new violations - that's exactly what I wanted.

[DavidAnson]

Looks awesome. I'm going to make the changes I suggest since I'm getting extra nit-picky at this point.

[DavidAnson]

GitHub is being silly, but maybe you can tell me how to work around it. If I make edits and push changes from a Codespace, it fails with HTTP 403 because I do not have write permission to your fork. However, if I use the web UI to make edits to files you have already edited, it works (that's what I did to implement my feedback), BUT it always adds a trailing newline, so the git diff check fails on the generated file. Could you please sync my changes to your branch, re-generate markdownlint-browser.js in your fork, and push it? Everything should go green and I'll approve the PR. (OR maybe you know how I can push edits from a Codespace?)

[khiga8]

Thanks, I've pushed up a commit(a1db76d) which re-runs the script!

GitHub is being silly, but maybe you can tell me how to work around it. If I make edits and push changes from a Codespace, it fails with HTTP 403 because I do not have write permission to your fork.

That sounds frustrating! I do have this checkbox option toggled on this PR:

This checkbox should give you permission to commit to my branch. (Related: Allowing changes to a pull request branch created from a fork)

Is your codespaces of my forked repo? This article has some pointers that might be helpful: Committing changes to a pull request branch created from a fork - GitHub Docs.

[DavidAnson]

Thank you! I will do a final review and merge the PR this evening.

Regarding permissions, I think all the preconditions are correct (and my ability to change individual files confirms my permission), but pushing from two different Codespaces created from that pull request failed. I have tried this in the past from the command line and that failed as well. It is possible I am making a mistake every time, but this has never worked for me and I am sure that I created the Codespace from the PR yesterday, so would have expected GitHub to set things up correctly. (I confirm that it created an "origin" and "upstream" remote.) I will try again tonight following the directions you sent exactly to double check myself.

[khiga8]

It is possible I am making a mistake every time, but this has never worked for me and I am sure that I created the Codespace from the PR yesterday,

@DavidAnson I was able to repro what you're seeing on my end too. After some investigation, I discovered this is a known issue in Codespaces currently.

Related community issues:

• Codespaces got error 403 when open and push to a fork · community · Discussion #36357
• git-push access denied if done from a codespace created from a pull request. · community · Discussion #36299

For now, you will have to stick with the traditional route of cloning via the steps in Committing changes to a pull request branch created from a fork - GitHub Docs. 😅

[DavidAnson]

Merged, thank you!

Regarding the Codespaces issue, thanks for tracking that down. :)